Thursday, August 15, 2013

Beware the literal string!

One of the most frustrating bugs in programming (for me anyway) is caused by mistyping a literal string. It is just so hard to catch because of the way the brain works. When you mean to type 'Denied.aspx", but you ended up typing "Denied.asp", you see "Denied.aspx" when you read it over. That's because it is what you expect to see. This is why it is sometimes necessary to have another set of eyes to look over your code. If someone spots a mistake like this in your code, don't worry they aren't smarter than you and you're not a bad coder. What I do is when I finish writing some code, especially code that involves literal strings, I let it sit for a day or too before going back and looking it over. Fresh eyes are more likely to spot this mistakes because what is SUPPOSED to be there isn't at the forefront of your brain.

The literal string gets really tricky when you are using the same exact string in more than one location. At this point it is makes sense to create a constant string variable and put it in there. I think that even if it is used in only one place it should still be put in a constant variable (the story I will share in a bit will show why I feel this way). The variable name could even be the string that variable holds. As long as you are using a modern IDE (I'm working on Visual Studio now) then your IDE can become a sort of spellchecker when using the constant.

Here is my latest literal string story:

My code needed a way to redirect a user to a access denied page if they weren't authorized to access the page. Pretty simple, a simple Server.Transfer("Denied.aspx") worked fine. (I could not use Response.Redirect for reasons beyond the scope of this blog entry). However Server.Transfer throws a ThreadAbortException, which can be a bit confusing since we log all exceptions and treat them as errors. While Servier.Transfer was probably the most correct way of coding this, we REALLY didn't want that exception showing up in logs. The solution I can up with was using Server.Execute instead.
     
     Server.Execute("Denied.aspx");
     Response.Flush();
     Response.Close();

What this does (as best as my knowledge goes..I  don't really want to be considered an expert on this) is 1) Server.Execute() = run the page and add the output (the rendered html that the client recieves) to the output buffer, 2) Response.Flush() = send whatever data is in the output buffer to the browser (execution of the current page request still continues and more can still be added to the buffer, and 3) Response.Close() = this closes the connection the browser used to make the page request to the server, meaning that the server can no longer use that connection to send anything else back to the browser (I'm a bit effy on this part, but it is how I understand it). Now I'm not sure if this is the proper way of doing this, but it doesn't throw an exception and seems to work. So I had

     if (conditions not met for access)
     {
          try {
               Server.Execute("Denied.aspx");
          }
          catch (Exception e)
               LogException(e);
          }
          finally {
               Response.Flush();
               Response.Close();
         {
    }

I then decided that this would be useful as a standalone function in a class built for general-use functions. The class was called GeneralFunctions and the function I decided to name Deny(); So now I had

     if (conditions not met for access)
     {
          Deny("Denied.asp");
     }

and my Deny function looked like

     internal static void Deny(string DenyPage)
     {
          try {
               Server.Execute(DenyPage);
          }
          catch (Exception e)
               LogException(e);
          }
          finally {
               Response.Flush();
               Response.Close();
         {
    }

Oops. Seemed like I left the 'x' off of "Denied.aspx" when calling my function. I was now getting an HttpException saying that the httphandle could not execute on the child request. I'm not going to lie, I had very little clue what this meant (in this case, it means that the file doesn't exist!). Now i haven't noticed my mistype yet,.So I reverted back to the old way. I did this via comments because I don't want to forget what I had typed before.

     if (conditions not met for access)
    {
         Server.Execute("Denied.aspx");
         Response.Flush();
         Response.Close();
         // Deny("Denied.asp");
    }

Ok, no exception there. I kept switching between having those three lines in the if statement and moving them into the beginning and end of Deny() and commenting and uncommenting Deny(). Finally I commented out the whole try..catch block in the Deny function while I had the trio of lines from the if statement in there and saw no exception. But even then I STILL couldn't see my typing mistake. I didn't see it until another coworker showed up and distraction me for a bit. When I went back to look at my problem...blam! All of a sudden I could see it clear as day.

It took about an hour, maybe more for me to see it. When I did see it, the first thing I did was put that bad boy into a constant and used it instead.

     constant string ACCESS_DENIED_PAGE = "Denied.aspx";

      if (conditions not met for access)
     {
         Deny(ACCESS_DENIED_PAGE);
     }

While I know that this small problem doesn't mean that I'm bad at coding, but its nice to know I'm not alone. So if you have a story about a typo messing up something you were working on, feel free to share below, and we'll all have a good laugh together.