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.

First post, new blog; Summer is here

So the heading doesn’t make sense, but this is my first post on my new blog; ICodeNazi. Please note the capitalization, it’s intended of course. BUt what’s with the name of the blog? You’ve probably seen Seinfeld and perhaps the infamous “Soup Nazi” episode? My blog is inspired by the soup nazi, only I won’t be serving soup. I will be talking about code quality, code refactoring and code layout. Links to interesting stuff will still be served every now and then on my old blog, but this blog will be just my content and only on code. I’m no expert, so this is also an attempt to learn something, but I am passionate about code quality. I love taking ugly code and running code analysis on it; removing anti-patterns and refactoring it to be easier to read and more maintainable. Let me tell you abit more about the history of this blog;

I’ve always been interested in computers and programming, for as long as I can remember I’ve loved taking things apart and at some point I also started putting them back together again. I’ve been a developer in my heart since 1994 and a .net coder since 2002. I used to love building machines, for friends and family, setting up networks and throing LAN parties… I’ve created a few websites, ugly, but working. I’ve done heaps of things and I enmjoy doing it all. However for the longest time I struggled with finding something that I really, really loved doing. Something that I could perhaps be very good at, not just knowledgeable at. You see I’ve always said that I know a little about most things and little about anything. I’m a “jack of all trades”  I guess. But a spring morning in April it just dawned on me; I love refactoring, I love writing unit tests for legacy code, I love removing old and dead code and I love to watch code go from ugly and unstructured to structured and beautiful code. I really love it! So I found my passion! Go me!

As I’ve already said; I’m no expert. I’m just passionate about this and so I will try to blog my experiences with tools (PEX, Resharper etc.), my project experiences and I will tell you my opinion in matters related to code quality.