Annoying Coding Practices

I am right now in the middle of transferring a piece of functionality from one project (ASP.net 1.1 web application) to a second project (Windows Forms desktop application). Although it is a significant feature, most of the work should be straight copy-paste. It is a large chunk of code written in C#, referencing a business object library shared between the two projects, a bunch of external dlls, etc.

Really, all that I should have to do is to customize the function for my projects codebase, migrate some of the ASP.net aspect to a WinForms environment, and hook it up to my UI. Yet, as I am going through this code, trying to make sense of what is going on, I find myself having to delete/modify/rewrite much more of the code than I should be doing. Although some of this is to be expected, much of it has to do with poor programming practices that if maintained lead to future difficulties with maintenance, poor readability, logic errors and bugs and are detrimental to debugging and testing. True, I have not found so many examples that are worthy of public exhibition, but I have received an education in how poor coding practices can effect a section of code long after the original programmer has completed his (or her) work.

A sampling of some of the things that are currently causing me grief:

Not enclosing the body of an if statement (or other control-body) in brackets

DoWork();
foreach (string s in List<string>)
   if (s == "a")
      DoA();
   else if (s == "b")
      DoB();
DoWork();

True, this code compiles and works. And thanks to Visual Studio’s automatic indentation system, you can sort of tell what items belong in what structure by looking at the indentation. But it is confusing. And if someone adds one more line of code somewhere in there without thinking about it, you are going to have big problems on your hands. (What should be done: use open- and close-brackets very liberally)

Leaving superfluous and redundant code in production

I found two different functions, each with hundreds of lines of code, each intended to perform the same function. One of them is used, one of them is not. Yet, neither of them is commented out, and there is not indication given that one of them is not used and is not necessary. This doesn’t break anything, but is a headache for those who have to deal with the code. (What should be done: if there is a whole section of code that is redundant, either comment it out or delete it)

Mixing different return types in the same functions

private object GetText(string s) {
   if (s == "a") {
      string t = a.ToLower()s
      return t;
   } else {
      StringCollection x = new StringCollection();
      x.Add(s);
      return x;
   }
}

private void Process() {
   // Do Work
   if (a) {
      txtName.Text = (string)GetText(x);
   } else {
      txtName.Text = ((StringCollection)GetText(x))[0];
   } 
}

The GetText function above can return either a string or a StringCollection, depending on the value of the parameter. Although this works if the programmer knows exactly what is going on and makes no mistakes, it is very error-prone and is a nightmare for maintenance. (What should be done: find some way to use one return type in a function. If this is not possible, create two separate functions. If this is not possible, use reflection to test the value of a returned object before using it in the guise of its assumed type)

Using strings instead of enums

private void DoStuff(string size) {
   switch (size) {
      case "small":
        // work
        break;
      case "large":
        // work
        break;
   }
}

Like the previous examples, this code compiles and works. One would presumably either call this function with DoStuff(“small”) or DoStuff(“large”). But what if you use a different parameter value. DoStuff(“Small”) compiles but fails at runtime, as does DoStuff(“medium”). However, if instead of using a string parameter, you create an enum called Sizes with values “small” and “large”, you guarantee that only valid values can be passed to the function.

Misspelling the product name in the assembly

This doesn’t really affect anything. It is just sloppy. If your program is called Writing Workshop, please do not create a namespace in your code called “ritingworksop”. It is just annoying and reflects badly on your understanding of the application that you are creating.

Tagged , . Bookmark the permalink.

Leave a Reply

Your email address will not be published. Required fields are marked *