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

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 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 is 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?

private static bool _isLocked = false;
public bool IsLocked
{
  get
  {
    if (_isLocked == true)
    {
      return true;
    }
    else
    {
      return false;
    }
  }
}

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

if (!(Session["sNavText"] == null))
{
}

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.

if (Session["catVisit"] == null)
{
  Session.Add("catVisit", Request.RawUrl);
}
else
{
  Session.Add("catVisit", Request.RawUrl);
}

Related to the if statement above, there are numerous while loops like the one shown below. Not only is the logic convoluted, 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?

int numTries = 0;

while (!(numTries == 3))
{
  ...
  numTries++;
}

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

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

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:

if ((!(searchText.Text == "")) & (!(searchText.Text == staticText[24].ToString())))
{
  if (IsPostBack)
  {
    Session.Add("search", searchText.Text);
    Response.Redirect("~/searchResults.aspx");
  }
}

searchResults.aspx:

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

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

if (lNavText.Contains("%20"))
{
  lNavText = lNavText.Replace("%20", " ");
}

if (lNavText.Contains("%26"))
{
  lNavText = lNavText.Replace("%26", "&");
}

and so on...

We've not heard of server controls either.

<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 it's more efficient this way.

if (colorBox == null)
{
}
else
{
  int c = colorBox.SelectedIndex;
}

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!

 try
{
    sizeHash = myProd.GetSize(myProd.GetStyle, myProd.GetFitSelect, colorBoxSelect, myConn);
}
catch (Exception ex)
{
}

if (sizeHash.IsAvailable("small"))
{
}

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

public string[] parseMsg(string msg)
{
  int myInt = 0;
  int pos = 0;
  int count = 0;

  while (!(myInt == -1))
  {
    if (!(pos == 0))
    {
       pos += 1;
    }

    myInt = msg.IndexOf('|', pos);
    pos = myInt;
    if (!(myInt == -1))
    {
      count += 1;
    }
  }

  string[] Splitter = new string[count];
  Splitter = msg.Split(new Char[] { '|' });

  return Splitter;        
}

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

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

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