Removing dead code

Ambulance_Stretcher What does your code terrain look like?  Are there bodies of dead logic lying here and there?  Maybe they helped briefly while you worked towards a better solution or perhaps they just fell victim to changing business rules.

At a recent Code Camp, there was a question about code generation and I answered that we (as developers) are required to love every line of code (it drew a laugh from many).  But love isn’t cheap … it takes time, care and hard work.  Therefore we should only love and care for the lines of code that we absolutely need.  Anything not being used should be removed.

 

 

 

 

 

What excuses are there to keep unused code?

  • We may need it in the future
    • The classic argument – the typical agile response being YAGNI
    • Keeping it in the code-base takes maintenance in the form of understanding it, updating associated tests and working around it to add other functionality.
    • It will always remain in source control should we ever need it again
  • If it ain’t broke, don’t fix it.
    • Bob Martin sums this up well when he defines what “broke” means:  Code should 1) work 2) be maintainable and 3) communicate its intent.  Clearly dead code violates 2 and 3.
    • This idea also defeats the goal of loving every line of code – leaving something alone for no good reason is neglect.

I just spent a good half hour cleaning up some code and mostly removing unused code.  It was annoying since it wasn’t even being used but at least I am happier with the code now.  A good tool when investigating code you suspect may be dead is ALT-F7 using Jetbrains Resharper – you can easily find all usages of a method or class even in large code-bases.  A cheap alternative is to use “lean on the compiler” (as discussed by Michael Feathers) – this is simply changing a class name or method name by one character and try to compile (the compiler will then let you know what is depending on that code).

Jonathan Cogley is the CEO and founder of Thycotic Software, a .NET consulting company and ISV in Washington DC.  Our product, Secret Server is a enterprise password manager system for teams to secure their passwords.  Is your team still storing passwords in Excel?

  1. mark
    December 5, 2007 at 9:27 am

    When using C# it’s a better idea to use

    [System.Obsolete(“use class Namespace.Security.Password instead”)]
    public class Password
    {
    }

    Then the compiler would state obsolete functions are being used, this might work better when cleaning up and not being sure if it’s still used. (Also it’s faster when cleaning up more than one function:))

  2. Jonathan Cogley
    December 5, 2007 at 12:43 pm

    Hmmm, good idea – thanks for the post.

    Our experience with Obsolete has been poor – usually they get thrown in and then stay in the code for weeks to months. I guess that is just down to discipline though.

    It makes sense to use it as a tool while cleaning up but try not to commit the Obsoletes.

    Disclaimer to readers: I am not saying to never commit Obsolete, just not if it is only being used as a clean up tool.

  3. http://
    December 6, 2007 at 1:27 pm

    You can also use FXCop to report out unused code and variables.

  4. Patrick Smacchia
    December 6, 2007 at 2:06 pm

    To find potential dead code you can also use the following Code Query Language queries with the tool NDepend (http://www.NDepend.com)

    // Potentially unused methods
    WARN IF Count > 0 IN SELECT TOP 10 METHODS WHERE
    MethodCa == 0 AND // Ca=0 -> No Afferent Coupling -> The method is not used in the context of this application.
    !IsPublic AND // Public methods might be used by client applications of your assemblies.
    !IsEntryPoint AND // Main() method is not used by-design.
    !IsExplicitInterfaceImpl AND // The IL code never explicitely calls explicit interface methods implementation.
    !IsClassConstructor AND // The IL code never explicitely calls class constructors.
    !IsFinalizer // The IL code never explicitely calls finalizers.

    // Potentially unused fields
    WARN IF Count > 0 IN SELECT TOP 10 FIELDS WHERE
    FieldCa == 0 AND // Ca=0 -> No Afferent Coupling -> The field is not used in the context of this application.
    !IsPublic AND // Although not recommended, public fields might be used by client applications of your assemblies.
    !IsLiteral AND // The IL code never explicitely uses literal fields.
    !IsEnumValue AND // The IL code never explicitely uses enumeration value.
    !NameIs “value__” // Field named ‘value__’ are relative to enumerations and the IL code never explicitely uses them.

    // Potentially unused types
    WARN IF Count > 0 IN SELECT TOP 10 TYPES WHERE
    TypeCa == 0 AND // Ca=0 -> No Afferent Coupling -> The type is not used in the context of this application.
    !IsPublic AND // Public types might be used by client applications of your assemblies.
    !NameIs “Program” // Generally, types named Program contain a Main() entry-point method and this condition avoid to consider such type as unused code.

  5. Ryan Olshan
    December 9, 2007 at 5:26 am

    I guess the use of ObsoleteAttribute depends on rather it’s private code or a public API. In the case of a public API, ObsoleteAttribute is a necessity to provide backwards compatibility.

  6. Patrick Farrell
    December 12, 2007 at 9:50 pm

    > this is simply changing a class name or
    > method name by one character and try to
    > compile (the compiler will then let you know
    > what is depending on that code).

    Good article, but that last part is BAD BAD ADVICE.

    By your methods, one may be removing functionality that will not cause a compilation error, but is necessary to proper execution.

    Your method only accounts for code called within the loaded solution/project (depending on compile method). If you have a multi solution project, you may only be removing methods/classes that are needed for compilation of other libraries.

    PLUS, you better hope your codebase is not using reflection to load assemblies, or your technique just screwed them up.

    There is such a thing as runtime code resolution.

    Your last piece of advice is bad and anyone finding this should be aware that they may be introducing errors into a codebase if they follow these instructions.

  1. No trackbacks yet.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: