Copyright 2017-2018 Jason Ross, All Rights Reserved

Star InactiveStar InactiveStar InactiveStar InactiveStar Inactive
 
Bins - Commented Code Goes in The Black One With The Other Garbage!

Looking through your system’s source code, you’ll almost certainly find code that’s been commented out without being deleted. This might strike you as an odd thing to do, and it is. Why would anyone clutter up their source code in this way?

In the past, this was a common practice, maybe because people had less faith in version control systems, or they just didn’t use them. Keeping changes as commented-out code was justified, at least in some people’s minds. With modern version control and build systems this isn’t an excuse; no changes should ever get lost.

Even now maybe some developers can’t be bothered to check in every change, and would rather just comment the unwanted code with the intention that they’ll “get around to” changing it later. Others just can’t seem to let go of code that they wrote just in case they need it again – I’ve seen source code with parts that, according to the source control logs, were commented-out years ago. From that anecdotal evidence alone, if you’re commenting out code, you really don’t need it.

The problem gets worse when you realize there are managers and developers who believe that commented-out code should be left where it is rather than being deleted. This isn’t just because deleted code is changed code, and so must have unit tests created for it; I’ve also seen it argued for code that’s already covered by tests. I really have no idea what purpose that serves. Maybe it’s just that some people can’t think beyond the idea of doing the minimum necessary work to get a story done, assuming they’re working with agile. Maybe there’s just an underlying fear that something will go wrong if the code is deleted.

As well as cluttering up the source code, commented code has the added bonus that it gradually goes out of sync with the original code, and can introduce bugs if you ever do decide to use it again. For example, imagine you decide to create a class to handle locations in two dimensions. You might create two constructors – a default and a parameterized version:


    public class Location
    {
        public int XCoordinate{get; private set;}
        public int YCoordinate{get; private set;}

        public Location()
        {
            XCoordinate = 0;
            YCoordinate = 0;    
        }

        public Location(int x, int y)
        {
            XCoordinate = x;
            YCoordinate = y;
        }
    }

After a while, you decide that nobody is using the parameterized version, so you comment it out:


    public class Location
    {
        public int XCoordinate{get; private set;}
        public int YCoordinate{get; private set;}

        public Location()
        {
            XCoordinate = 0;
            YCoordinate = 0;    
        }

// Comment this out - we may need it later
//        public Location(int x, int y)
//        {
//            XCoordinate = x;
//            YCoordinate = y;
//        }
    }

Maybe you decide later that you want to handle locations in three dimensions rather than just two:


    public class Location
    {
        public int XCoordinate{get; private set;}
        public int YCoordinate{get; private set;}
        public int ZCoordinate{get; private set;}

        public Location()
        {
            XCoordinate = 0;
            YCoordinate = 0;    
            ZCoordinate = 0;    
        }

// Comment this out - we may need it later
//        public Location(int x, int y)
//        {
//            XCoordinate = x;
//            YCoordinate = y;
//        }
    }

Later you decide that maybe you should reinstate the constructor you commented out:


    public class Location
    {
        public int XCoordinate{get; private set;}
        public int YCoordinate{get; private set;}
        public int ZCoordinate{get; private set;}

        public Location()
        {
            XCoordinate = 0;
            YCoordinate = 0;    
            ZCoordinate = 0;    
        }

        public Location(int x, int y)
        {
            // Something is missing here...
            XCoordinate = x;
            YCoordinate = y;
        }
    }

Straight away there’s a problem – the constructor is out of date because you added an extra field, for the Z coordinate. You may or may not notice it, especially if you also commented out all of the tests that run against that constructor, because they’ll be out of date too.

Imagine that instead of just being a constructor, the commented-out method was missing some later additions that fixed some security vulnerability. Now instead of introducing a simple uninitialized field bug, you’ve introduced a security bug, just because you couldn’t face deleting the original code properly and then rewriting it

Next time someone tells you to leave commented-out code in your source code, remember you’re not just deleting it to make things easier for you, you’re potentially saving future developers from serious problems. In many cases, that developer in the future will be you. Ignore the critics, make sure the code is covered by tests, and get rid of the garbage.