Displaying a video player to eligible users

I posted this on Stack Overflow, but it was suggested that I move it over to Code Review.

I would like some feedback on the way I decided to clean up code from the in a .NET project that had Page_Load functions that had been added to over many years to the point that they were a complete mess.

While the code cleanup is the context of the question, this is really a question about if I should or should not be passing by reference to "underscore functions". Underscore functions are functions that are not meant to be used for encapsulation (although I made all the underscore functions protected) nor "reusability", but instead to wrap code in a human friendly way. The "clean up" version below is not the final product, but was instead a first stab a cleaning up the code. The cleaned up code is functioning correctly.

Just to be clear (the code below is a bad example for this), in general I have needed to do mass mutation in the "underscore" functions on other pages. Meaning I have had to change/set the value for multiple variables that are being passed in and then used outside the "underscore" function's scope.

ORIGINAL PAGE_LOAD

```
public void Page_Load() {
// Set player to have been initialized
this.initialized = true;

if(!this.playerOverride) {
String vendorValue = Resolver.Resolve("PlayerVender");
if(vendorValue.Equals("jwplayer")) {
this.UseVideoRxPlayer = false;
} else {
this.UseVideoRxPlayer = true;
}
}

// Get instances for use
VideoAccess videoAccess = VideoAccess.getInstance();
LearnerAccess learnerAccess = LearnerAccess.getInstance();

// Member should be logged in
if(memberId == null) {
this.memberId = SessionUtils.getMemberId();
} else if(SessionUtils.getMemberId() != null && SessionUtils.getMemberId() != memberId) {
// A member is logged in and it is not the member w

What you call "underscore functions" are actually the result of a few principles including separation of concerns, single responsibility and sure, there is some encapsulation involved as well but that's not really prevalent.

So in that regard: it's definitely a good thing that you are splitting this in multiple methods because it really makes it easier to follow the flow of your application.

That being said, let's start critiquing!

You're setting initialized to true before you've even started initializing anything. I would do this at the very end to indicate that everything is finished and succesful (I don't assume you want to mark things initialized when in reality the program crashed).

I'm noticing explicit this prefixes. Typically I only do this when I have to remove ambiguity between two variables with the same name in the same scope (or when it is required to compile, of course). Adding this clutters the code a little bit more.

I prefer to define (almost) all of my variables as var and when it's a string, I use the lowercase string instead of String. It just feels nicer and in the case of var: keeps the focus on what's important.

C# is awesome! For strings, we can just use == instead of .Equals.

I find vendorValue == "jwplayer" cleaner than vendorValue.Equals("jwplayer").

Boolean assignments like this:

if(vendorValue.Equals("jwplayer")) {
    this.UseVideoRxPlayer = false;
} else {
    this.UseVideoRxPlayer = true;
}

can be combined to this:

UseVideoRxPlayer = vendorValue != "jwplayer";

I'd avoid throwing Exception because it will make it impossible later on to distinguish between an exception you've thrown and an unknown exception caused by code, should you wrap all this around a general catch(Exception) to make sure nothing slips through the cracks.

Often this is worked around by creating a custom MyApplicationException class and throwing your selfdefined errors with that.

Keep naming in mind: protected members are UpperCamelCase. Underscores are only considered best practice as private fields.

In _IfMemberAndVideoExist you use two variables memberId and videoId which you convert to integers. This implies that these Ids are stored as a type they shouldn't be outside of your method.

This poses 2 issues:

• You'll have to convert it to the appropriate type everywhere it will be used

• You can get exceptions if the fields contain inappropriate values

Both issues can be solved by storing the fields in the correct datatype.

licenses.FirstOrDefault(x => x.available != null && x.available > 0);

This line seems to indicate you have a Nullable property called available.

First of all: the naming convention for properties is that they're UpperCamelCase.
Secondly: available in itself doesn't tell me what it is (I know it has something to do with a license, but what exactly..)

I prefer licenses.Any() over licenses.Count > 0

Take a closer look at this code snippet:

MemberLicense license = licenses.FirstOrDefault(x => x.available != null && x.available > 0);
if (license == null && licenses != null && licenses.Count > 0)
{
    license = licenses[0];
}
bool hasFreeLicense = false;

if (license != null)
{
    licensenum = license.license_id;
    hasFreeLicense = license.available > 0;
    groupId = license.group_id;
}

As far as I can tell from first sight: if you have a situation where license is initially null and licenses has items but the first item in that list (aka: index 0) is null then licensenum, hasFreeLicense and groupId will not be set.

This might not have been a problem but license is never reassigned so it will remain null. This will crash your program if a flow is followed where eventually _legacyLicenseConflict will be called: there the license.license_id field is used. This will result in a NullReferenceException.

Your code is cluttered with casts of memberId and videoId to integers. I would strongly encourage you to change the backing field. If that is not possible then at least use a temporary variable in method scope.

FullVideo.Solution is not an appropriate boolean naming. It should IsSolution, HasSolution or something similar which indicates what exactly that solution means.

nextVideo also seems to be a private field which is the wrong datatype.

You are now manually setting NextVideoSpecified to true, although this is actually a computed value based on whether or not SpecifiedVideo is null or not.

It isn't unthinkable that someone might forget to do this someday so I would make this implicit:

public bool IsNextVideoSpecified { get { return SpecifiedVideo != null; } }

int PrimaryGroupId = (int)contactInfo.Item1;
lp = learnerAccess.GetPrimaryGroupPlanInfo((int)PrimaryGroupId);

PrimaryGroupId is already an integer, calm down!

initializeVideoHero()

should be

InitializeVideoHero()

Methods are UpperCamelCas

StackExchange Code Review Q#72278, answer score: 6