7
\$\begingroup\$

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 who was specified on control creation
            throw new Exception("Logged in member is not the member the player is using for reporting data");
        }


        if(memberId != null && videoId != null) {

            List<MemberLicense> licenses = LicenseAccess.getInstance().GetMemberLicenses(Convert.ToInt32(memberId)
                , Convert.ToInt32(videoId));

            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;
            }


            // From here on out we will use memberId and videoId explicitly

            // Get the video with the member's information attached
            FullVideo = videoAccess.GetVideo((int) memberId, (int) videoId);
            if(!(FullVideo.Solution && !SubscriptionModel.HasAnnualSubscription((int) memberId))
                && SubscriptionModel.MemberCanAccessVideo((int) memberId, (int) videoId)) {
                // If this is a solution, the member has access to it. Otherwise, it is a
                // regular video and the member has access to it

                // Set the page title
                Page.Title = FullVideo.ProductName + ": " + FullVideo.Title;

                if(SessionUtils.IsValidSession() && (Context.User.Identity.IsAuthenticated || SessionUtils.isCookieUser())) {
                    // If a next video was specified, let's set it for display on video completion
                    if(nextVideo != null) {
                        panels.SpecifiedVideo = (int) nextVideo;
                        panels.NextVideoSpecified = true;
                    }
                    panels.PlayingPlaylistVideo = isPlayingPlaylist;

                    //Get Learner Info
                    Tuple<int?, ContactType?> contactInfo = learnerAccess.GetPrimaryGroupContactInfoForMember((int) memberId);
                    if(contactInfo.Item1 != null) {
                        int PrimaryGroupId = (int) contactInfo.Item1;
                        lp = learnerAccess.GetPrimaryGroupPlanInfo((int) PrimaryGroupId);
                    }

                    if (hasFreeLicense || learnerAccess.IsLearnerMember((int)memberId)) 
                    {
                        initializeVideoHero();
                        if (SeriesId != null && SeriesId != 0)
                        {
                            List<Instructor> surveyLinks = new List<Instructor>();

                            foreach (Instructor i in VideoProduct.Instructors)
                            {
                                surveyLinks.Add(i);
                            }

                            if (surveyLinks.Count > 0)
                            {
                                ratingform1.SurveyLinks = surveyLinks;
                            }
                            initializeRatingForm();
                        }
                        videoFooter.Visible = true;

                        // Member has access to view video
                        if(SessionUtils.getSubDaysLeft() > 0) {
                            panels.Trial = false;
                        } else if(SessionUtils.getHasTrial() != null && (bool) SessionUtils.getHasTrial()) {
                            panels.Trial = true;
                        }
                        panels.ShowPopover(FullVideo);

                        // Display analytics codes for Google Analytics
                        if(showAnalytics) {
                            gaqEventCode.Text = "<script>_gaq.push(['_trackEvent', 'Video', 'Viewed', '"
                                + HttpUtility.HtmlEncode(FullVideo.ProductName) + " - "
                                + HttpUtility.HtmlEncode(FullVideo.Title) + "']);</script>";
                        }

                        int? lastPosition = videoAccess.GetLastVideoPosition((int)memberId, (int)videoId);
                        if (lastPosition != 0)
                        {
                            lposition = lastPosition;
                        }

                        playerElement.Visible = true;
                        panels.Visible = true;
                    } else if (!hasFreeLicense && license != null) {
                        // Member would have access to this video, but there has been a license issue.
                        // This most likely is indicitive of a legacy account.
                        uclicensesinuse.setShowAddLicensesButtonVisibility(false);
                        videofailure.Visible = true;

                        innerPlayerWrapper.Visible = false;

                        licensesinuse.Visible = true;
                        uclicensesinuse.Visible = true;

                        uclicensesinuse.license = license.license_id;

                        // Store denied license info to database
                        videoAccess.DenyLicense((int)memberId, license.license_id, (int)FullVideo.Product, 
                            (int)FullVideo.Id, (int)RequestedStatus.AllLicensesInUse, SessionUtils.getVisitId(), String.Empty);
                        // Determine if this is an admin. If so, display a button to purchase new licenses
                        if (videoAccess.GetLicenseAdmins((int)license.license_id).Any(m => m.Id == memberId)) {
                            uclicensesinuse.setShowAddLicensesButtonVisibility(true);
                        }

                    } else if (!hasFreeLicense && license == null) {
                        // No longer has an active subscription
                        videofailure.Visible = true;
                        subscriptionexpired.Visible = true;
                        innerPlayerWrapper.Visible = false;
                        if (SessionUtils.IsLearner()) {
                            // This is a learner so they can either renew or start a free trial
                            if (lp.IsFreeTrial) {
                                learnertype.InnerHtml = "free trial";
                                learnerrenewtype.InnerHtml = "Subscribe";
                            } else {
                                learnertype.InnerHtml = "subscription";
                                learnerrenewtype.InnerHtml = "Renew";
                            }
                            learneraccount.Visible = true;
                        } else {
                            // This is a legacy subscription
                            legacyaccount.Visible = true;
                        }
                    } else {
                        videofailure.Visible = true;
                        noaccesspanel.Visible = true;
                        videoerror.Visible = true;
                    }
                } else {
                    // Member is not logged in and should be prompted to start a free trial
                    trialpanel.Visible = true;
                    playerWrapper.Visible = false;
                    videohero.Visible = false;
                }
            } else {

                videofailure.Visible = false;
                noaccesspanel.Visible = true;
                videoerror.Visible = true;
                innerPlayerWrapper.Visible = false;
                videohero.Visible = false;
            }
        } else {
            // Member is not logged in and should be prompted to start a free trial
            trialpanel.Visible = true;
            playerWrapper.Visible = false;
            videohero.Visible = false;
        }
    }

CLEANED UP PAGE_LOAD VERSION

        protected void Page_Load(object sender, EventArgs e) {
        if(!initialized) {
            PreparePlayer();
        }
    }

    public void PreparePlayer() {
        // 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 who was specified on control creation
            throw new Exception("Logged in member is not the member the player is using for reporting data");
        }


        if(memberId != null && videoId != null) {
            _IfMemberAndVideoExist(ref learnerAccess, ref videoAccess);

        } else {
            // Member is not logged in and should be prompted to start a free trial
            trialpanel.Visible = true;
            playerWrapper.Visible = false;
            videohero.Visible = false;
        }
    }

    protected void _IfMemberAndVideoExist(ref LearnerAccess learnerAccess, ref VideoAccess videoAccess)
    {
        List<MemberLicense> licenses = LicenseAccess.getInstance().GetMemberLicenses(Convert.ToInt32(memberId), Convert.ToInt32(videoId));

        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;
        }


        // From here on out we will use memberId and videoId explicitly

        // Get the video with the member's information attached
        FullVideo = videoAccess.GetVideo((int)memberId, (int)videoId);
        if (!(FullVideo.Solution && !SubscriptionModel.HasAnnualSubscription((int)memberId))
            && SubscriptionModel.MemberCanAccessVideo((int)memberId, (int)videoId))
        {

            _IfViewableVideo(ref learnerAccess, ref hasFreeLicense, ref videoAccess, ref license);
        }
        else
        {
            if (!hasFreeLicense && license == null)
            {
                //@TODO this was unreachable due to added check SubscriptionModel.MemberCanAccessVideo((int) memberId, (int) videoId).  Moved this logic here outside of _IfViewableVideo.
                _subscriptionExpired(ref learnerAccess);
            }
            else
            {
                // This is a solution that they don't have access to
                videofailure.Visible = false;
                noaccesspanel.Visible = true;
                videoerror.Visible = true;
                innerPlayerWrapper.Visible = false;
                videohero.Visible = false;
            }
        }
    }


    /*
     * 
     * If this is a solution, the member has to have access to it (Annual Sub). Otherwise, it is a regular video and the member has access to it
     */
    protected void _IfViewableVideo(ref LearnerAccess learnerAccess, ref bool hasFreeLicense, ref VideoAccess videoAccess, ref MemberLicense license)
    {

        // Set the page title
        Page.Title = FullVideo.ProductName + ": " + FullVideo.Title;

        if (SessionUtils.IsValidSession() && (Context.User.Identity.IsAuthenticated || SessionUtils.isCookieUser()))
        {
            _IfIsValidUser(ref learnerAccess, ref hasFreeLicense, ref videoAccess, ref license);
        }
        else
        {
            // Member is not logged in and should be prompted to start a free trial
            trialpanel.Visible = true;
            playerWrapper.Visible = false;
            videohero.Visible = false;
        }
    }

    protected void _IfIsValidUser(ref LearnerAccess learnerAccess, ref bool hasFreeLicense, ref VideoAccess videoAccess, ref MemberLicense license)
    {
        // If a next video was specified, let's set it for display on video completion
        if (nextVideo != null)
        {
            panels.SpecifiedVideo = (int)nextVideo;
            panels.NextVideoSpecified = true;
        }
        panels.PlayingPlaylistVideo = isPlayingPlaylist;

        //Get Learner Info
        Tuple<int?, ContactType?> contactInfo = learnerAccess.GetPrimaryGroupContactInfoForMember((int)memberId);
        if (contactInfo.Item1 != null)
        {
            int PrimaryGroupId = (int)contactInfo.Item1;
            lp = learnerAccess.GetPrimaryGroupPlanInfo((int)PrimaryGroupId);
        }

        if (hasFreeLicense || learnerAccess.IsLearnerMember((int)memberId))
        {
            _IfLearnerWithAccess(ref videoAccess);
        }
        else if (!hasFreeLicense && license != null)
        {
            _legacyLicenseConflict(ref videoAccess, ref license);
        }
        else if (!hasFreeLicense && license == null)
        {
            //@TODO unreachable due to added check SubscriptionModel.MemberCanAccessVideo((int) memberId, (int) videoId).  This code is still being used.  Moved this logic to outside the IfViewableVideo.
            _subscriptionExpired(ref learnerAccess);
        }
        else
        {
            videofailure.Visible = true;
            noaccesspanel.Visible = true;
            videoerror.Visible = true;
        }
    }

    protected void _IfLearnerWithAccess(ref VideoAccess videoAccess)
    {
        initializeVideoHero();
        _populateInstructorRatings();
        videoFooter.Visible = true;

        // Member has access to view video
        if (SessionUtils.getSubDaysLeft() > 0)
        {
            panels.Trial = false;
        }
        else if (SessionUtils.getHasTrial() != null && (bool)SessionUtils.getHasTrial())
        {
            panels.Trial = true;
        }
        panels.ShowPopover(FullVideo);

        // Display analytics codes for Google Analytics
        if (showAnalytics)
        {
            gaqEventCode.Text = "<script>_gaq.push(['_trackEvent', 'Video', 'Viewed', '"
                + HttpUtility.HtmlEncode(FullVideo.ProductName) + " - "
                + HttpUtility.HtmlEncode(FullVideo.Title) + "']);</script>";
        }

        int? lastPosition = videoAccess.GetLastVideoPosition((int)memberId, (int)videoId);
        if (lastPosition != 0)
        {
            lposition = lastPosition;
        }

        playerElement.Visible = true;
        panels.Visible = true;
    }

    /*
    * @TODO investigate if the trainer raitings can be removed (except initializeRaitingForm)
    */
    protected void _populateInstructorRatings()
    {
        if (SeriesId != null && SeriesId != 0)
        {
            List<Instructor> surveyLinks = new List<Instructor>();

            foreach (Instructor i in VideoProduct.Instructors)
            {
                surveyLinks.Add(i);
            }

            if (surveyLinks.Count > 0)
            {
                ratingform1.SurveyLinks = surveyLinks;
            }
            initializeRatingForm();
        }
    }

    /*
     * Member would have access to this video, but there has been a license issue
     * This most likely is indicitive of a legacy account.
     * 
     */
    protected void _legacyLicenseConflict(ref VideoAccess videoAccess, ref MemberLicense license)
    {
        uclicensesinuse.setShowAddLicensesButtonVisibility(false);
        videofailure.Visible = true;

        innerPlayerWrapper.Visible = false;

        licensesinuse.Visible = true;
        uclicensesinuse.Visible = true;

        uclicensesinuse.license = license.license_id;

        // Store denied license info to database
        videoAccess.DenyLicense((int)memberId, license.license_id, (int)FullVideo.Product,
            (int)FullVideo.Id, (int)RequestedStatus.AllLicensesInUse, SessionUtils.getVisitId(), String.Empty);
        // Determine if this is an admin. If so, display a button to purchase new licenses
        if (videoAccess.GetLicenseAdmins((int)license.license_id).Any(m => m.Id == memberId))
        {
            uclicensesinuse.setShowAddLicensesButtonVisibility(true);
        }

    }

    /*
     * Member no longer has an active subscription.
     * 
     * 
     */
    protected void _subscriptionExpired(ref LearnerAccess learnerAccess)
    {

        //Get Learner Info
        Tuple<int?, ContactType?> contactInfo = learnerAccess.GetPrimaryGroupContactInfoForMember((int)memberId);
        if (contactInfo.Item1 != null)
        {
            int PrimaryGroupId = (int)contactInfo.Item1;
            lp = learnerAccess.GetPrimaryGroupPlanInfo((int)PrimaryGroupId);
        }

        // No longer has an active subscription
        videofailure.Visible = true;
        subscriptionexpired.Visible = true;
        innerPlayerWrapper.Visible = false;
        if (SessionUtils.IsLearner())
        {
            // This is a learner so they can either renew or start a free trial
            if (lp.IsFreeTrial)
            {
                learnertype.InnerHtml = "free trial";
                learnerrenewtype.InnerHtml = "Subscribe";
            }
            else
            {
                learnertype.InnerHtml = "subscription";
                learnerrenewtype.InnerHtml = "Renew";
            }
            learneraccount.Visible = true;
        }
        else
        {
            // This is a legacy subscription
            legacyaccount.Visible = true;
        }
    }
\$\endgroup\$
  • 4
    \$\begingroup\$ Welcome to Code Review! I suggest you only include the post-cleanup code; reviewers won't make a diff of the two and review your changes - they'll more likely tell you what you've missed and what's left to do with the last version of the code ;) \$\endgroup\$ – Mathieu Guindon Dec 10 '14 at 22:40
6
\$\begingroup\$

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:

  1. You'll have to convert it to the appropriate type everywhere it will be used
  2. 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<int> 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 UpperCamelCase.


I would rename getSubDaysLeft() to GetSubscriptionDaysLeft(). There is no reason to sacrifice readability to save 10 pixels.


if (SessionUtils.getSubDaysLeft() > 0)
{
    panels.Trial = false;
}
else if (SessionUtils.getHasTrial() != null && (bool)SessionUtils.getHasTrial())
{
    panels.Trial = true;
}

This implies that if neither of the two conditions are true, panels.Trial will have its current value. I can't know from the code whether that is intended but if you never set Trial elsewhere, this means it will have the default value of false. In that case I would incorporate this in your code by making it explicit:

panels.Trial = SessionUtils.getHasTrial() != null && (bool)SessionUtils.getHasTrial()

Once more: getHasTrial() shouldn't have to be casted.


lposition is not a good name. I assume it means lastPosition but that should be written out entirely and follow naming conventions.


You can shorten this:

List<Instructor> surveyLinks = new List<Instructor>();

foreach (Instructor i in VideoProduct.Instructors)
{
    surveyLinks.Add(i);
}

to this:

List<Instructor> surveyLinks = new List<Instructor>();
surveyLinks.AddRange(VideoProduct.Instructors);

or even

List<Instructor> surveyLinks = new List<Instructor>(VideoProduct.Instructors);

uclicensesinuse and all other variables in this neighbourhood should be using lowerCamelCase styling.


Here too: I prefer string.Empty over String.Empty. Takes away some of the black color in your IDE and it feels slimmer. All the cool kids use it.


Conclusion

Overall the code is very well written and easy to follow. Some styling remarks that can be solved by following the conventions and some code that can be written less verbose but I couldn't find anything that struck me as potentially performance hindering so you're good on that aspect.

It's fine to use the ref parameters, nothing wrong with it. The naming of your methods are all in the form of [Condition] whereas convention dictates [Action]. This results in "IfSubscriptionExpired" vs "DisplayRenewSubscriptionDialog".

I can see where you're coming from with the [Condition] scheme since you are basically building a flowchart so I would be okay with it if it makes things easier in your case.

\$\endgroup\$
  • \$\begingroup\$ List<T>.Count isn't too bad - it's IEnumerable<T>.Count() that's much better off as IEnumerable<T>.Any(), to avoid iterating the entire collection just to see if there's 1 item. +1 anyway, great, great review as always :) \$\endgroup\$ – Mathieu Guindon Dec 11 '14 at 1:46
  • \$\begingroup\$ Thank you for taking the time to really look at the code! Most of it is code that was written before me, but I really appreciate your comments. I actually plan on implementing some of your suggestions. I appricate the conclusion as well, because the real concern I have here is another developer coming along and thinking I was crazy and ruining performance. I've gone a little overboard with the ALWAYS passing by reference into these "underscore" function. If I mark this as an answer will there still be some discussion about it? I would like to see if there are any detractors. \$\endgroup\$ – tophstar Dec 11 '14 at 4:26
  • \$\begingroup\$ Great review, but I'd use "PascalCase" instead of "UpperCamelCase" and simply "CamelCase" instead of "lowerCamelCase". \$\endgroup\$ – BCdotWEB Dec 11 '14 at 7:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.