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.

 image

 

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

 

My main problem here is the combination of

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?

 

 ap--check   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

 

image 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.

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

 

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

3 comments:

Unknown said...

Fully agree! But I beat you: I already came across code (also in Page_Load) with CC > 30 :O

That's a lot of fun adding new features there, believe me...
Unfortunately many devs still write such code which I often just cannot understand. Extracting/refactoring wouldn't be too difficult.

But a question: what do you mean with CC=23 "Not too bad"??
Normally I consider code with CC > 10 (15) to be quite complex already. Of course just if I speak of single-method complexity.

Peter Gfader said...

Sorry this was my mistake!
Cyclomatic Complexity = 23 = Bad.
I edited that part...

Unknown said...

Nice article. I also agree with you; I'm working currently on a project where there is a method with a cyclomatic complexity of 44. The other ones are around 30.
It's a pain to understand what the program does...

Post a Comment

Latest Posts

Popular Posts