Bad code examples (part 1)

So, I’ve been looking through some old code today and I’ve found some pretty good examples of how to not write good code. Take a look:

   1: public class MapHelper {

   2:     public MapHelper() {

   3:     }

   4:  

   5:     public static String BooleanValue(bool val) {

   6:         if (val) return Boolean.TrueString.ToLower();

   7:         else return Boolean.FalseString.ToLower();

   8:     }

   9: }

public class MapHelper {

    public MapHelper() {

    }


    public static String BooleanValue(bool val) {

        if (val)

            return Boolean.TrueString.ToLower();

        else

            return Boolean.FalseString.ToLower();

    }

}

This code has most likely been written before .Net 2.0, back when there wasn’t any static classes. I have no idea why the empty default constructor has been added, but I’ve seen that done in a lot of C# code written by Java developers, so perhaps it’s a Java habit? I dunno. Anyways, the more correct implementation of this code would of course be to remove the empty constructor and mark the class as static. Of course the code should also be commented and there are some Globalization issues with using the "ToLower" method.

And another one:

public void String saveInvoiceToContentmanager(string originalDocument, string pdfFileName, string clientID)

{

    // create the client to connect

    Client client = new Client(clientID);


    try {

        // open a connection to the client.

        client.Open();


        // save the original document into the content repository

        string filename = Path.Combine(Path.GetTempPath(), System.Guid.NewGuid().ToString() + ".xml");

        using (StreamWriter streamWriter = new StreamWriter(new FileStream(filename, FileMode.Create))) {

            streamWriter.Write(originalDocument);

        }


        try {

            docId = Client.NewDocId();

            client.CreateContent(new string[] { pdfFileName, filename });


            try  {

                File.Delete(pdfFileName);

            }

            catch (Exception) { }

        }

        finally {

            try {

                File.Delete(filename);

            }

            catch (Exception) { }

        }

    }

    finally {

        try {

            client.Close();

        }

        catch (Exception) { }

    }

}

This one comes with a plethora of issues! There are some good patterns in here though, if you look carefully. The developer has avoided the try-catch-finally trap. What trap you say? Well, it doesn’t make sense to use all three keywords together, it should be either try-catch or try-finally. I’d make a couple of changes in this code. For starters I’d implement the IDisposable interface in the Client class (yes, in this example that would be possible) and then I’d add some more if statements before the first File.Delete call, to make sure that we’d be able to delete the file. Retry code such as this is hardly a good idea. The code doesn’t provide any hints as to way this is done, but I’m assuming that it’s a delay in letting the file-handle go from other parts of the code. And finally the casing is Java style for the method name. So my version would perhaps look a bit like this:

public void string SaveInvoiceToContentmanager(string originalDocument, string pdfFileName, string clientID) {

    // create the client to connect

    using (Client client = new Client(clientID)) {

        // open a connection to the client.

        client.Open();


        // save the original document into the content repository

        string filename = Path.Combine(Path.GetTempPath(), System.Guid.NewGuid().ToString() + ".xml");

        using (StreamWriter streamWriter = new StreamWriter(new FileStream(filename, FileMode.Create))) {

            streamWriter.Write(originalDocument);

        }


        try {

            docId = Client.NewDocId();

            client.CreateContent(new string[] { pdfFileName, filename });

        }

        finally {

            if (File.Exists(filename))

                File.Delete(filename);

        }

    }

}

Of course you could always improve this code even further by making use of a Dependency Injection container, such as Structure Map, but that’s all for another day. But, and here comes the real point of my post (!); how would one be able to trap these sorts of bugs? Yes, I call lack of code quality a bug. I’d recommend using static code analysis and CI build environment. That should catch perhaps 80% of the bugs, like the static class stuff. And then make sure to do proper code review, aka Peer Review.