Apps

Why you need to clean out dead code paths

Justin James describes how a dead feature he left in place caused a bug. He encourages other developers not to get into the bad habit of never deleting dead code.

I have heard it said that "insanity is a perfectly normal response to an insane environment." Over the course of my career, I have learned some really bad development habits as a form of self-defense against bad management habits. For example, when you combine a lack of good source control systems with project management that keeps asking you to remove and put back a piece of functionality multiple times throughout a project, you tend to develop a habit of never actually deleting code, just removing the access points in the user interface. After all, without good source control, you can't easily get it back, and with indecisive management, it is likely that you will need to get it back.

I know that I am not alone in this behavior. I can't tell you how much code I have seen where there are huge blocks that are totally unreachable, pages of commented out work, conditions set to "if true == true" so they didn't have to get rid of the work in the "else" portion, and so on. It is obvious to me that there are a lot of developers who are afraid to ever hit the delete button with a large piece of code selected.

Unfortunately, while this bad habit may have saved me a lot of time, it has some very serious side effects. Here's a list of the issues that I have come across:

  • Code changes gets ugly because you are trying to keep orphaned code in-line with the rest of the system, but there is often no real regression testing or anything else.
  • Maintaining code after a long period away from it (or by someone else) is very difficult, because no one really knows why a piece of code is there, they just know that it is there.
  • The code is no longer a faithful representation of the business logic, because it contains logic that the specifications and business logic are not aware of.
  • It presents potential security risks, as unmaintained code can sometimes be reached (especially in Web applications, where tweaking parameters may trigger the code).

I'm thinking about and addressing this subject because, for the first time in a long time, a "dead feature" that I left in place caused a bug. It was a silly thing: At one point I had both "Is Approved" and "Is Active" for an invoice system, where "Is Approved" was for a manager to sign off on an invoice, and "Is Active" to act as a soft delete, and in one particular piece of code, it was checking "Is Approved" but not "Is Active." The problem was, the "Is Approved" system had been removed entirely from the UI, but I left the code in place because "we might need it again." It would have taken me less than 15 minutes to put "Is Approved" back in, linked to the UI, and re-tested, but because I never removed it from the system, I missed that one section where it was still being checked and "Is Active" was not. The end result was that in a very rare scenario, an invoice would be floating around, totally invisible to the user, but ready to get billed to the customer. And that is exactly how we discovered the bug, when a nightly billing report showed an end customer getting billed. Luckily for us, this was a rare bug, and it only affected two invoices, which were easily rectified, and the bug was fixed straightaway.

All the same, as a developer who tries to be open minded about constant improvement, it made the point all too clearly: I am no longer in an insane environment, and I no longer need to do insane things as a coping mechanism to defend myself. We had a card in Trello to remove some other unwanted code, and I had backlogged it because "I didn't have time." After this incident, I immediately took ownership of that card and resolved it. It took me 10 minutes, and I feel a lot better knowing that this other unwanted code is gone and not going to cause me any embarrassment in the future.

J.Ja

Keep your engineering skills up to date by signing up for TechRepublic's free Software Engineer newsletter, delivered each Tuesday.

About

Justin James is the Lead Architect for Conigent.

27 comments
lelandhamilton
lelandhamilton

When I have worked on a project where there is no version control, I first try to propose and implement a project repository. Some projects have a version control process that only allows working or reviewed code. For these I go under the covers and set up a CVS or Subversion repository on my own PC. I can check in and out files at will, broken or working. I will usually put the file in before changing. If a piece of code is being reworked, I will use #if 0..#endif to get rid of obsolete code or #if 1 ???new code???#else old code #endif and test that, put it in my private repository, get rid of the conditional obsolete code, test again, and then put it in both my repository and the project repository. A repository policy should allow checking code in intermediate states (maybe as a branch) with tagging/labeling or whatever the version control systems calls it. To capture code that is being mangled, intermediate labels of ready for review, passed review, passed unit test, integration can be helpful. If multiple releases are being developed, there should be labels for each release candidate. Of course there should be labels for anything released to quality control, manufacturing or anywhere else (apply the labels, then build). One little trick: have a module in the build that is compiled in each build with a timestamp (e.g. C & C++ __DATE__ __TIME__) that can be accessed from the user interface, e.g. displayed on the splash screen and available from the help about screen, or, if there is no user display, easily accessed by a debugger. This can help resolve questions related to “Which version is it?”

mattohare
mattohare

It's a bear to to read when a full class or module is comments. In the decades I've been working, the only time I've seen code uncommented, or put back in to use successfully, is when it's created and reused in the same part of the same development cycle. Otherwise, it just takes up space. In the event someone decides to leave code in without comments, and use some conditional to keep it invisible, it better stil have complete unit testing on it to ensure that it doesn't break while it's in retirement. Still, if its functionality becomes useful in the future, it's best to develop it from scratch to the new needs and circumstances. If code is retired by commenting or conditionals, it's not likely to ever be useful in its current form anyway.

HGunter
HGunter

An approach to removing undead code is to hive it off into a subroutine, add a comment where it used to be which points to the "subroutine", and preferably comment out the code itself. Another is to move it to a code file which contains only undead code (a code morgue), again as a subroutine/function, and put in a comment which points to that - better if there's lots of it. Both approaches retain the code and a pointer to it, and remove clutter from where it no longer belongs. A final thought is that one can delete it and add a comment which indicates briefly what was deleted and in which source version it last existed, which makes it easily findable should it need resurrection. The only approach I haven't actually used is the morgue code file.

cfg83
cfg83

Hello - IMO cleaning out dead code is good as long as the "dead" version of a method is recorded in the source control database. One strategy to avoid accidental invocation of dead code is to change it's name FROM : theMethodName() TO : HIDE_theMethodName() You could pick any name : OFF_theMethodName() OBSOLETE_theMethodName() DEADCODE_theMethodName() VERSION123_theMethodName() The idea is that you make it impossible to use the method without informing the developer that something is amiss. This is easy in Java because you only have to change the name in one place. In other languages like C++ you also have to change it in the include file where the class is declared. Temporarily renaming methods+compiling is also a good trick to see who depends on them. This is especially true if you are compiling against binaries that are using your code, aka you can't search the library because it's not a source file. ...cfg...

Charles Bundy
Charles Bundy

when I was a GTA. Person approaches for some help saying their assignment complied fine but it isn't producing any output. I go and take a look. Lo and Behold every single line of code is in a comment block. With an incredulous tone I ask, "Why are all the lines commented out?" The reply "That's the only way I could get it to compile w/o errors."

codepoke
codepoke

I've been known to leave code around, too. I clean it up eventually, but just often enough I'm very glad it was still sitting there. :-)

RMSx32767
RMSx32767

A program that called a subroutine which did nothing except return. Perhaps in the mists of time past the sub did in fact serve a purpose but there was no code there and no comments. Still, I am more in favor of simply rendering unused code inert via "if 0=1 then" and leave a comment regarding why it is no longer being used. I do this because I've seen too many cases of code yanked that had to then be replaced. Worse (?) is code that is commented out by commenting out each line rather that testing a "never will be true" condition. Of course, this all is assuming disk space is not a problem and that noone will ever care you are testing a condition which will never be true.

Favicon
Favicon

Old comments are as bad as old code for hanging around; no one wants to be the guy who presses delete, just in case it turns out to be helpful 6 months from now. I work on several projects now that are jammed full of "JL 2012-02-26 Not needed anymore since ..." type comments, and they do nothing but get in the way (especially the ones referring to code that has since been removed/uncommented/etc, as the messages are then just damn confusing). These types of comments are exactly what version control exists for; why insert dated comments all over your code when version control dates the code changes for you?

MadestroITSolutions
MadestroITSolutions

I think there are times when commented code can be useful, although with source control there is really no need unless you want to preserve an idea in the spot for later consideration. The issue is that you need to have some good regression testing in place so that situations like the author's don't happen (yeah, right, life is perfect, right?). That being said, removing code is ALWAYS a pain. Let's be honest, checking for references for every little piece you remove is not realistic. the compiler might catch most of the issues but the bottom line is there is always a risk anytime you introduce changes.

andrew232006
andrew232006

I always comment it out as soon I realize it isn't being used. I go back and delete the commented code later when I'm satisfied I won't be using it.

Organic53
Organic53

it is called regression testing. Any developer who removes code without it being tested should be terminated on the spot.

Mike Page
Mike Page

On rare occasions I find it useful to leave a small paragraph of code disabled. It gets preceded by a paragraph of comments explaining why it is there along with my name and the date. It is used for historical reference. This is something version control cannot help with, because the reason for the removal is not documented, and the code is hidden in the version control system without much hope of every stumbling across it again. Let me be clear that this is not an argument against Justin's point. There are simply exceptions to his common sense advice.

mikewor
mikewor

I was taught many years ago that ideas are valuable, but the code you wrote probably stank to high heaven. So when you delete code, don't just hit the delete key. Document what was there so that if the requirement comes around again, the next guy knows where it goes. Also cross reference to the original requirements documentation - hopefully a copy will still be in the archive.

Deadly Ernest
Deadly Ernest

When I have to change any of my code I keep a copy of it on file, but create the new code with only what it needs to get the job done. That's how I was first taught all those many moons ago, and it was taught that way because we did NOT have the high powered computers we have now and we HAD to keep the code to the minimum to make sure it ran faster. Part of the training was to write a small project, get the code working, then amend it by adding things, and then amend it by deleting things, and check it all still worked as intended for the final version. I'm surprised and shocked this is NOT a standard process throughout the industry now.

Tony Hopkinson
Tony Hopkinson

On the left hand side for my ide, dead code was gone as soon as I spot it, and I look for it as well.

jkameleon
jkameleon

You usually spend about 10% of your time writing code, and 90% reading it. It therefore makes sense to optimize it for reading. Every additional line of code means additional time and effort necessary for scrolling and finding stuff. And, time is money. So, you shouldn't be sentimental about the code you've wrote. It's a program, not poetry. Delete it mercilessly, and let version control handle the remains. That approach is supported rather nicely by NetBeans 7.2 . It has built in local history, and a huge button on top of the text that takes you directly into the diff screen.

atoms
atoms

That was part that a couple commenters didn't seem to read - that the projects Justin was discussing did not use version control. Which is entirely insane. The immediate solution that leapt to mind was exactly as you described: if the project does not use version control at least set up local repository for your own work.

Slayer_
Slayer_

We will have global methods like screen open, screen close, data save, etc. Many are empty, or used to have something and are empty now because it was no longer needed. An example would be our XML blob fields, originally every screen that used them had to load and save them, when this was changed, instead of editing 1000's of instances of this methods use, the method was just emptied. A year later, a new reason to have code at that point came up, so we just reused the method, now it has a name totally unrelated to its purpose. The people that read our code are probably going to want to kill us.

SafeInAFlash
SafeInAFlash

If it is just dead code why not skip it as fast as you can.

bboyd
bboyd

Please don't make me suffer like that. I get code from older systems I need to completely re-write the code comments as I figure out what is happening. Comments that should have been in the whole time. Marking things as dead code should be handled with some kind of notation so you can removed it without deleting comments when scrubbing the code automatically.

Charles Bundy
Charles Bundy

when deleting comments. Or the source code equivalent of comments, which is Justin's point.

Justin James
Justin James

... but for things like database schemas, *it isn't that simple*. You don't just yank columns out of a DB. J.Ja

Tony Hopkinson
Tony Hopkinson

Not used / not reachable, therefore isn't run, there can have no impact on on execution, therefore no impact on test. It's called knowing what you are talking about...

MadestroITSolutions
MadestroITSolutions

That's what source control is for. You can always look at past revisions.

Slayer_
Slayer_

In other cases, using compiler directives can help.

andrew232006
andrew232006

I meant I remove commented out code that no longer serves any purpose, usually while I'm reviewing and fixing up my documentation. I don't wash away all internal documentation from my code.