Website development and design blog, tutorials and inspiration

Go On... Have a chuckle at this poorly written code

The comedy of errors

By , 24th September 2008 in C#

A collection of poorly written code written by an under-qualified contractor. It would be funny if it wasn't in a production system!

In my permanent place of work, we had an external contractor in to write an eCommerce website while I learned ASP.Net and C#. Now the website is up and running and I have completed my training, it's now my job to look through the code and maintain it. The contractor long since gone, but his legacy lives on; every day we curse his code.

I have pulled out various snippets of this poorly written code to show everyone how not to program C#, and highlight the fact that the most expensive contractors are not necessarily the best. Hope you find it as amusing as we did!

Here is a property that exposes a private Boolean, _isLocked. Inside the property he checks if the private field is true, then returns true, otherwise he returns false! Why not just return _isLocked?

  1. private static bool _isLocked = false;
  2. public bool IsLocked
  3. {
  4. get
  5. {
  6. if (_isLocked == true)
  7. {
  8. return true;
  9. }
  10. else
  11. {
  12. return false;
  13. }
  14. }
  15. }

This one is brilliant. All throughout his code the not equals code is very abstract, he negates the equality as shown below. This style of coding makes it VERY difficult to debug or understand the logic.

  1. if (!(Session["sNavText"] == null))
  2. {
  3. }

Let's check if a session value is null, if it is we'll add it, otherwise add it. Session["catVisit"] = Request.RawUrl; will do the job just as well with much less code and without any of the checking.

  1. if (Session["catVisit"] == null)
  2. {
  3. Session.Add("catVisit", Request.RawUrl);
  4. }
  5. else
  6. {
  7. Session["catVisit"] = Request.RawUrl;
  8. }

Related to the if statement above, there are numerous while loops like the one shown below. Not only is the logic abstract, but comparing a counter with an absolute value is very dangerous - what happens if numTries somehow gets set to 4 in the spaghetti code of the while loop?

  1. int numTries = 0;
  2.  
  3. while (!(numTries == 3))
  4. {
  5. ...
  6. numTries++;
  7. }

Classic! Normally when you compare two strings you make them both lowercase or both uppercase, that's good. But in this case, he runs .ToLower() on a static string! Why use extra processing to call ToLower when you can just change the string???

  1. if (text2.ToLower() == "Default.aspx".ToLower())
  2. {
  3. }

Rather than changing the PostBackURL property of the form to searchResults.aspx, we'll post it back to default.aspx, add the value to the session cache, then redirect the user to the correct page and read in the session cache.

default.aspx:

  1. if ((!(searchText.Text == "")) & (!(searchText.Text == staticText[24].ToString())))
  2. {
  3. if (IsPostBack)
  4. {
  5. Session.Add("search", searchText.Text);
  6. Response.Redirect("~/searchResults.aspx");
  7. }
  8. }

searchResults.aspx:

  1. mySearch = (string)Session["search"];

We've never heard of Server.UrlEncode or Server.UrlDecode.

  1. if (lNavText.Contains("%20"))
  2. {
  3. lNavText = lNavText.Replace("%20", " ");
  4. }
  5.  
  6. if (lNavText.Contains("%26"))
  7. {
  8. lNavText = lNavText.Replace("%26", "&");
  9. }
  10.  
  11. and so on...

We've not heard of server controls either.

  1. <img src="images/<asp:Literal ID="litURL1" runat="server"></asp:Literal>.jpg" alt=" " align="left" title=" " />

We won't rewrite the if statement logic to become a not equals; instead we will use an empty if true block and use the else block for our code. Makes sense, after all its more efficient this way.

  1. if (colorBox == null)
  2. {
  3. }
  4. else
  5. {
  6. int c = colorBox.SelectedIndex;
  7. }

When a block of code falls over for no reason, don't worry that's what a try...catch block is for! The code will no longer fall over and sizeHash is now guaranteed to contain valid data! NOT!

  1. try
  2. {
  3. sizeHash = myProd.GetSize(myProd.GetStyle, myProd.GetFitSelect, colorBoxSelect, myConn);
  4. }
  5. catch (Exception ex)
  6. {
  7. }
  8.  
  9. if (sizeHash.IsAvailable("small"))
  10. {
  11. }

Saving the best for last! The method will split a string into a string array, a very useful method so you may wish to save it. Basically, when you create an instance of a string array you need to know its size, so this method will calculate the number of elements in the string allowing you to create the array before calling the actual split method.

  1. public string[] parseMsg(string msg)
  2. {
  3. int myInt = 0;
  4. int pos = 0;
  5. int count = 0;
  6.  
  7. while (!(myInt == -1))
  8. {
  9. if (!(pos == 0))
  10. {
  11. pos += 1;
  12. }
  13.  
  14. myInt = msg.IndexOf('|', pos);
  15. pos = myInt;
  16. if (!(myInt == -1))
  17. {
  18. count += 1;
  19. }
  20. }
  21.  
  22. string[] Splitter = new string[count];
  23. Splitter = msg.Split(new Char[] { '|' });
  24.  
  25. return Splitter;
  26. }

When using this code snippet, who will be continuing to use the one-liner below anymore?

  1. string[] Splitter = msg.Split('|');

Anyway, hope this made you chuckle as much as it did us.

Comments
  1. Andrew
    Andrew

    Looks like that 'Consultant' was paid per line of code, or plateau'd in skill and settled.

    Sad either way.

    A good collection :)

  2. Bala
    Bala

    Looks like a old School Programmer... learned some stuff in old language.. apply the same concepts in C#.. even though there are better and simpler way to do things... but good collection though.

  3. lol nice job to that highly paid contractor!

Leave a Reply

Your email address will not be published.