ASP.NET – WTF Code smell – Request.Path.Contains

1. Read the following code and try to understand what it tries to achieve

 protected void Page_Load(object sender, EventArgs e)
 {

       if (HttpContext.Current.User.IsInRole("User "))
       {
           FormView1.FindControl("noteRow").Visible = true;
           FormView1.FindControl("RequiredFieldValidator4").Visible = false;
           // TODO: Fix this - What if Action is 2nd parameter in URL
           if (Request.Path.Contains(@"ClientProfile.aspx?Action=Add"))
           {
               FormView1.FindControl("thLoginID").Visible = true;
               FormView1.FindControl("tdLoginID").Visible = true;
               FormView1.FindControl("txtLoginID").Visible = true;
           }

       }
       else
       {
           FormView1.FindControl("noteRow").Visible = false;
       }


     if (HttpContext.Current.User.IsInRole("Administrator"))
     {

         if (!Request.Path.Contains(@"/AdministratorProfile.aspx"))
         {
             if (!Request.Path.Contains(@"/Profile.aspx"))
             {
                 FormView1.FindControl("txtLoginID").Visible = false;
                 FormView1.FindControl("lblLoginID").Visible = false;
                 -- Snip snip snip – more controls are hidden or shown -- --

                 if (Request.Path.Contains(@"/UserProfile.aspx") && HttpContext.Current.User.IsInRole("User") && (Request.QueryString["Action"] == null || Request.QueryString["Action"].Equals("New")))
                 {

                     FormView1.FindControl("thLoginID").Visible = true;
                     FormView1.FindControl("lblLoginID").Visible = true;
                     -- Snip snip snip – more controls are hidden or shown -- --

                 }
                 else
                 {
                     FormView1.FindControl("thLoginID").Visible = false;
                     FormView1.FindControl("lblLoginID").Visible = false;
                     -- Snip snip snip – more controls are hidden or shown -- --


                 }

             }
             else
             {
                 FormView1.FindControl("thDateRegistered").Visible = false;
                 FormView1.FindControl("tdDateRegistered").Visible = false;
                 -- Snip snip snip – more controls are hidden or shown -- --

             }
         }
         else if (Request.Path.Contains(@"/ReaderProfile.aspx"))
         {
             FormView1.FindControl("tdAdviser").Visible = false;
             FormView1.FindControl("thAdviser").Visible = false;
             -- Snip snip snip – more controls are hidden or shown -- --
 
         }
     }
     else if (HttpContext.Current.User.IsInRole("Administrator"))
     {
        -- Snip snip snip – this code is 130 lines long... -- --

     Figure: Code from a ASP.NET usercontrol. The usercontrol determines on which page it is used

2. Now try to image you have to fix something in this mess.

3. Or try to add a feature for a certain user.

 

 

The only way this code is developed, is

1. debugging (called manual testing by Roy)
2. adding some code,
3. debugging again,
4. adding some code
5. …

 

My main problem here is the combination of

• HttpContext.Current.User.IsInRole
• Request.Path.Contains
• Request.QueryString

In particular

• The check on URL path and query string parameters!!!
• -->  Request.Path.Contains(@"ClientProfile.aspx?Action=Add")
  • What if Action is 2nd parameter in URL (and not the 1st one)
  • This comes from: debugging and seeing that this comes in to the control…

How can the next dev work out when we enter in which branch?

 

    This code (control) has been fixed now and uses now properties that are set from the user of the control

My suggestions:

• Don’t touch Request.Path.Contains method at all
• Don’t use debugging to decide which part of the method to insert new code
• Try to think before code :-)
• Don’t use the debugger to much

BTW: This code broke at least two principles

1. “Single Responsibility Principle”
2. “tell don’t ask principle”

 

I am curious so I paste this code to VS 2010 and run “Analyze”, “Code Metrics”

• Cyclomatic Complexity = 23. Not too bad.  EDIT: Cyclomatic Complexity = 23 = Bad.
• Maintainability Index = 25. Not very good

 

 Figure: Cyclomatic Complexity of 23 for the Page_Load method

 

As a quick test I extract a piece of code from the Page_Load to another method and expect the Cyclomatic Complexity to go down and the Maintainability to go up.

Figure: Extracting 1 method out of Page_Load, “Maintainability Index” goes up - NICE

 

Extract til you drop, but with meaningful names!!!