Code Smells and Other Problems

August 24, 2011

one comment

Here are some Bad ThingsTM I’ve come across during the last few days and felt like sharing with you to let out some steam. These aren’t Coding Horrors per se, but perhaps there is something for all of us to learn here.

Bad Naming

I tweeted yesterday about a method called WaitForAllRequestsToExecute which doesn’t wait for all the requests to execute. This is one of the many bad things you can do to the maintenance programmer, which may lead to an axe-hunting adventure. (“Always write code as if the maintenance programmer is an axe murderer who knows where you live.”)


Just the other day I was reviewing and fixing a class responsible for asynchronous work item execution. It is really full to the brim with features – you can queue a work item for execution, you can update it, you can cancel it, you can wait for all work items to complete, you can query for all unfinished work items, and so on. Under the covers this glorified thread pool uses the .NET thread pool (the project is targeting .NET 3.5, so the TPL is not available).

The application, however, uses just two of these features. Namely, it needs to queue a bunch of work items for execution from various places, and then in some other, unrelated place, wait for all these work items to complete. Honestly, this is not something for which you need this glorified thread pool with all the unused and untested features. A simple WaitForNoneEvent would suffice (pseudo-code follows). The funny thing is that the glorified thread pool actually uses one of these under the covers.

class WaitForNoneEvent {
    ManualResetEvent e = new ManualResetEvent(true);
    int threadsInside = 0;

    public void Enter() {
        if (Interlocked.Increment(ref threadsInside) == 0)
    public void Exit() {
        if (Interlocked.Decrement(ref threadsInside) == 0)
    public void Wait() { e.WaitOne(); }

Leaving TODOs in the Code

This is a really popular practice – you write a couple of classes, maybe even a few tests, and then before going home to call it a day you put a TODO comment in some method, as a reminder that something needs to be fixed. So far so good, as long as you remember to fix it the next day!

Yesterday I came across a TODO comment in a class that has been “live” for a couple of months that said roughly the following:

//TODO: Make sure there are no other operations on this item now

The consequences of the premise not being met? Well, there is no thread-safety built-in, so if there are actually “other operations on this item”, I can’t vouch for the item’s health after they complete 🙂

Now, this is not the kind of thing you leave as a TODO comment in your code and then go away for a few months. This is a showstopper bug that needs to be fixed right away, and at the very least you need to put a big post-in in the middle of your screen before going home to remind you of this.

Testing by Production Deployment

How does the following sound as a testing plan? “We will deploy the application to production, let some users (not the important ones) play with it a little bit, and then see what bugs we find and fix them.”

The results were… interesting. First of all, shortly after the deployment, something failed and the production database was rendered unusable. But that’s fine, because we can always rename the production database to the “testing database” and see – now there’s no problem fiddling around with data in the production environment.

Next, there were so many bugs that the logs would become impossible to read with all the exceptions and error messages. But that’s also fine – you fix a bug, and then you test the fix by deploying it back to the production environment and letting users see if the problem went away.

The amazing thing here? The team has a build server, has a bunch of staging servers for these experiments, has well-written unit tests which run in the CI build and QA tests which run in the nightly build – and they have completely forsaken all these practices and went berserk with the “test in production” mantra.

I can’t fully explain why this happened yet, but one obvious thing that comes to mind is that the CI build and the nightly build have been failing for a few weeks. Whenever this happens, you lose quite a bit of the confidence you have in the tests, and might be inclined to try the “time-tested” approach described above.

I am happy to report, however, that this testing in deployment practice has ceased and we are now working on unit tests and QA tests for all the “new” code that hasn’t been tested yet.

Add comment
facebook linkedin twitter email

Leave a Reply

Your email address will not be published.

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <s> <strike> <strong>


one comment

  1. Peter MorlionAugust 24, 2011 ב 7:50 PM

    Yes! Bad naming and TODOs that have been there for ages. Even TODOs for a piece of code that actually works. Either fix it now, or leave it be, but don’t just add a TODO comment.
    I’ve had similar experiences with other stuff, which I also blogged about ( and