125

You've found some code that looks superfluous, and the compiler doesn't notice that. What do you do to be sure (or as close to sure as you can) that deleting this code won't cause regression.

Two ideas spring to mind.

  1. "Simply" use deduction based on whether or not the code looks like it should execute. However, sometimes this can be a complex, time-consuming job, slightly risky (your assessment is prone to error) for no substantial business return.

  2. Place logging in that code section and see how often it gets entered in practice. After enough executions, you should have reasonable confidence removing the code is safe.

Are there any better ideas or something like a canonical approach?

  • 55
    It may be helpful to look at version control history: When was the "dead code" created? Did it look dead when it was created? Was other code (which has since been modified or deleted) checked in which used it, at the same time as it was created? – ChrisW Mar 8 '17 at 15:59
  • 9
    The compiler can't be trusted if any form of meta-programming such as reflection is in play. A proper grep of the base directory is an essential practice. It is also common to have multiple applications sharing code so be sure to grep at the correct directory layer. – Adam Caviness Mar 8 '17 at 21:41
  • 16
    I would ask - "Why bother to delete it?". If it's used in some edge case that is called once in a blue moon, you've broken things. If it's never used and you leave it there, the damage is that the executable file is a tiny bit larger. Unless this is for some embedded device, this is a big deal? Stick a comment on it and move on to using your time productively. – mickeyf_supports_Monica Mar 8 '17 at 21:51
  • 40
    @mickeyf The damage is that someone has to maintain that code. That means a developer has to understand what it does. The developer has to modify it if things change and the code around it now has to do something different. Believe me, when you leave cruft lying around, it's a major headache for anyone trying to figure things out later. – jpmc26 Mar 9 '17 at 2:24
  • 9
    @MichaelJ. My point is that it shouldn't have been there in the first place. And if I leave it, the situation is worse: now some other developer down the line has to investigate it as well. The longer it stays, the more time gets spent understanding it. This is the cost of superfluous code. – jpmc26 Mar 10 '17 at 21:27

20 Answers 20

111

In my perfect fantasy world where I have 100% unit test coverage I would just delete it, run my unit tests, and when no test turns red, I commit it.

But unfortunately I have to wake up every morning and face the harsh reality where lots of code either has no unit tests or when they are there can not be trusted to really cover every possible edge case. So I would consider the risk/reward and come to the conclusion that it is simply not worth it:

  • Reward: Code is a bit easier to maintain in the future.
  • Risk: Break the code in some obscure edge case I wasn't thinking about, cause an incident when I least expect it, and be at fault for it because I had to satisfy my code quality OCD and make a change with no business value perceivable to any stakeholder who isn't a coder themselves.
  • 249
    Having observed the state of large-ish software projects where engineers, for ten to twelve years, had mostly followed the "not worth it" approach to removing likely superfluous code, I cannot recommend this approach. – njuffa Mar 7 '17 at 15:48
  • 125
    Also, unit tests tell you nothing about whether this code is actually used in production. The code could be perfectly tested, but still not used in production, therefore superfluous. – knub Mar 7 '17 at 16:15
  • 8
    In my experience the state of such projects is never due to a "not worth it" approach to fixing the code after the fact, it's always due to bad code being written in the first place. It is a mistake to write bad code, but it's also a mistake (a more advanced mistake) to think that cleaning code is always worth it. – Weyland Yutani Mar 7 '17 at 17:46
  • 42
    @Weyland Yutani That does not match my experience at all. Most of the likely superfluous code I have seen was sufficiently well written and perfectly reasonable given software specifications, hardware platforms, or operating system versions in existence a decade prior, needed to work around bugs or limitations in the tool chain or hardware available earlier, etc. – njuffa Mar 7 '17 at 18:15
  • 20
    Answer misses one of the most critical and important benefits of going ahead and making the change: learning a thing. Having learned it, you can add structure (comments, tests) to help maintenance devs later. Choosing not to change something because you don't know the consequences is cargo cult programming, even if that choice is not to delete something. – kojiro Mar 8 '17 at 11:15
84

There are two halves to this process. The first is confirming that the code is indeed dead. The second is comprehending the costs of being wrong and making sure they are properly mitigated.

Many answers here have great solutions to the former half. Tools like static analyzers are great for identifying dead code. grep can be your friend in some cases. One unusual step I often take is to try to identify what the code’s original purpose was. It’s a lot easier to argue “X is no longer a feature of our product, and code segment Y was designed to support feature X” than it is to say “I don’t see any purpose for code segment Y.”

The second half is a key step to breaking any gridlock over whether you should remove code. You need to understand what the implications are of getting the answer wrong. If people are going to die if you get the answer wrong, pay attention! Maybe it’s a good time to accept that code cruft develops over time and instead try not to write more cruft yourself. If people aren’t going to die, ask yourself how forgiving your users are. Can you send them a hotfix if you broke something and maintain your customer relations? Do you have a Q&A team that’s paid to find issues like this? These sorts of questions are essential for understanding how certain you must be before you hit the delete key.

In the comments, rmun pointed out an excellent phrasing of the concept of understanding the original purpose of the code before removing it. The quote is now known as Chesterton’s Fence. While it is too large to be quoted directly in a comment, I think it deserves to be properly quoted here:

In the matter of reforming things, as distinct from deforming them, there is one plain and simple principle; a principle which will probably be called a paradox. There exists in such a case a certain institution or law; let us say, for the sake of simplicity, a fence or gate erected across a road. The more modern type of reformer goes gaily up to it and says, “I don’t see the use of this; let us clear it away.” To which the more intelligent type of reformer will do well to answer: “If you don’t see the use of it, I certainly won’t let you clear it away. Go away and think. Then, when you can come back and tell me that you do see the use of it, I may allow you to destroy it.

  • 14
    Chesterton's Fence assumes discovery comes purely by thinking, and offers no recourse–a rather Ancient-Greek philosophy. The scientific method is more modern–devise a controlled experiment to determine the consequences of removing the structure. To be sure, I wouldn't nonchalantly propose this experiment be done in production. But if there is no alternative, and the cost of trial in production is very high, well, I would rather quickly leave a workplace which offered me no safe means to learn why some code exists–the personal risk at such a job is too high. – kojiro Mar 8 '17 at 12:37
  • 15
    @kojiro That is very true. I like to think that Chesterton would be okay if I, as a "modern reformer" came by and said "I don't know why this fence is here, I admit it. But I'd like to paint it red to see if that provides me any new information," Chesterton would probably be happy with it. – Cort Ammon Mar 8 '17 at 14:13
  • 41
    The VMS OS kernel mode clock decoding method SYS$ASCTIM contains a line of code that helps correct for the sidereal drift of the vernal equinox. None of DEC's unit tests exercises this code. It ran once -- correctly -- on 2000-02-28 at 23:59:59:999. It will not run again until year 2400. Please do not delete it. – A. I. Breveleri Mar 8 '17 at 21:40
  • 13
    @A.I.Breveleri I should hope that there's a comment in the code itself explaining this, and not merely here on StackExchange :D – Kyle Strand Mar 9 '17 at 23:44
  • 11
    @KyleStrand What are you talking about? This is the documentation. Can you think of a better place to put crucial information so that other people find it than StackExchange? ;-) – Cort Ammon Mar 9 '17 at 23:46
43

I also tend to grep for the function/class name in the code, which can give some additional benefits that a code analyzer might not, like if the name is mentioned in a comment or in a documentation file, or a script, for example. I run grep on the files in the source tree and store the result in a file; usually the result gives a condensed information: file name/path, line number, and the line where the name is encountered, which can give clues to where the function/class is called or mentioned without any semantic meaning (in contrast to a code analyzer), and regardless of the file extensions. Definitely not the ultimate solution but a nice addition to an analysis.

  • 11
    I am not a downvoter, but what if the code section you suspect doesn't ever get run is not a complete function or class but a section of a method/function? – Tulains Córdova Mar 7 '17 at 17:01
  • 9
    Depending on the language this may not always be reliable - some languages allow method calls to be made dynamically. For example php: $object->$variableMethod($arg1, $arg2); – Dezza Mar 7 '17 at 17:20
  • 9
    @TulainsCórdova Then this is probably not a good solution, obviously. As for every potential solution, use it if it makes sense in the given context. But maybe greping the eg. function encompassing the code section can give clues as to how the function is used and therefore its content? – piwi Mar 7 '17 at 21:44
  • 7
    "like if the name is mentioned in a comment or in a documentation file, or a script" - Or if someone is using reflection to call the method (if using a high-level/OO language). Though still not necessarily 100% accurate, as some languages support some very obtuse ways of finding and invoking methods. – aroth Mar 8 '17 at 5:16
  • This is helpful if you have access to all possible code that could use the function. However, if the code is part of a library, how are you going to know that something else you can't search through won't break? – Kevin Shea Mar 8 '17 at 14:54
30
  • Identify the code that looks dead (static analysis, etc).
  • Add a log message for every invocation of the allegedly dead code. It's easy with functions / methods; with static members like constants it's trickier. Sometimes you can just mark code as deprecated, and the runtime will log a message for you automatically. Leave the code otherwise intact.
    • Log a message when the dead module is loaded: most languages have a way to run static initialization at load time, which can be piggy-backed.
    • Make sure your log message includes a reasonable stack trace, so that you understand what called the allegedly dead code.
  • Run the altered code through all your test suites. Your test suites should also test for special times, like crossing a midnight, a turn of a quarter, a turn of a year, etc. Look at the logs, update your understanding of what's dead. Note that unit tests may specifically test the dead code, while no other unit tests and no integration tests touch it.
  • Run the altered code in production for a few weeks. Make sure every periodic process, like those once-a-month ETL cron jobs, are run during this period.
  • Look at the logs. Anything that was logged is not actually dead. The transitive closure of the call graph over the rest of the dead code is also potentially not dead, even if it was not invoked. Analyze it. Maybe some branches are safely dead (e.g. working with an API of a now-extinct service), while some are not yet. Maybe a whole module / class is only loaded for a constant defined in it, and you can easily disuse it.
  • Whatever is left is safely dead, and can be removed.
  • There is a first time for everything, so just because code didn't run before doesn't mean it will never run, particularly if there is an execution path to it. That said, deciding to remove something that can execute is a different process from determining if it could run. – user251748 Mar 7 '17 at 17:20
  • 7
    @nocomprende exactly what happens if this code is only used for an end of year function and you run your test is run Feb - Nov. You won't find the usage even though you profiled it for nearly a whole year. – Erik Mar 8 '17 at 20:42
  • 1
    @9000 I agree in theory but legacy systems are riddled with hidden dependencies that can foil this kind of testing. For your testing example to work you need to know that there is some kind of temporal coupling. If the temporal coupling is external to the code you're testing then you aren't going to see it in the code and know that temporal coupling is a factor. For example silo A's code is installed in the GAC. Years ago silo B asked silo A to add a code path for a report they need run against silo A's data. cont. – Erik Mar 8 '17 at 21:56
  • 1
    @9000 cont. Now silo B's code has a temporal coupling to a "dead" code path in silo A's code that isn't apparent by just looking at silo A's code. How would you unit test for this temporal coupling since the temporal coupling is only in silo B's code without knowing to talk to silo B? Even then time isn't a factor in your code (silo A) so what would a temporal test look like? – Erik Mar 8 '17 at 22:03
  • 2
    Anybody remember Y2k panics? Some code was right in 2000 because it was wrong for 2100! We had a potential issue that could occur once in 100 years or even once in 400 years. – Steve Barnes Mar 11 '17 at 6:43
16

In addition to the existing answers mentioned you could also iteratively remove the code over several versions.

In the initial version you could do a deprecation warning with the code block still functioning. In the version after that you could remove the code block but leave an error message letting users the feature is deprecated and no longer available. In final version you would remove the code block and any messages.

This may help identify unforeseen functionality with no warning to end users. In the best case scenario the code really does nothing and all that happens is that the unneeded code is kept over several versions before being removed.

  • 7
    +1. I've seen so many bugs and customer-facing issues that could have been avoided if the developers (or their management) had been willing to break a project into two or three safe phases instead of trying to throw everything in at once to meet an arbitrary deadline before moving onto the next project. – ruakh Mar 8 '17 at 18:15
  • This answers the question asked in the title, but if you ask me, the question has the wrong title. The title asks how to delete the code, but the body is about how to identify whether the code is safe to delete in the first place. You definitely answer the former, but I'm not sure that's what the OP was really asking. – underscore_d Mar 12 '17 at 13:57
7

A lot of people are suggesting that the "safe" thing to do is to leave the code in if you can't prove it's unused.

But code is not an asset, it's a liability.

Unless you can explain why it's important and point to its tests, the "safer" alternative might very well be to delete it.

If you're still not sure, then at least make sure to add tests to exercise the zombie code.

  • Step 1: Delete all code. Step 2: put back what is needed. Step 3: repeat. – user251748 Mar 9 '17 at 14:26
  • 1
    @Adrian Can I draw your attention to Knight Capital? en.wikipedia.org/wiki/… - just in case you want to bolster your point with a reference to it. They basically imploded because some old code came back to life and kindly lost almost $500m for them. The ensuing inquiry focused on their release procedures, but the core problem was "dead functionality wasn't deleted". – SusanW Mar 13 '17 at 13:02
6

You can use feature toggles to change the execution path of your software to completely ignore the code in question.

This way you can safely deploy your change with the code not in use, and the toggle off. If you notice some major faults related to the code turn the toggle back on and investigate the possible path to it.

This approach should give you confidence if you see no problems over prolonged period of time as well as the ability to turn it back on live w/o a deployment. However an even better way would be to also apply extra logging and test coverage around the area in question which will provide more evidence whether it is used or not.

Read more about toggles here: https://martinfowler.com/articles/feature-toggles.html

6

Static analysis of course... and the nice thing is, you don't need any new tools; your compiler has all the static analysis tools you need.

Just change the name of the method (e.g. change DoSomething to DoSomethingX) and then run a build.

If your build succeeds, the method, obviously, isn't being used by anything. Safe to delete.

If your build fails, isolate the line of code that calls it and check to see what if statements surround the call to determine how to trigger it. If you can't find any possible data use case that triggers it, you can safely delete it.

If you are really worried about deleting it, consider keeping the code but marking it with a ObsoleteAttribute (or the equivalent in your language). Release it like that for one version, then remove the code after no problems have been found in the wild.

  • 8
    This is ok advice for programming languages that don't have full dynamic/late binding. However, for those that have, it's trickier. And, of course - code might be using the method even if it's never used in production. – eis Mar 8 '17 at 7:30
  • The question presupposes compile-time symbol resolution (So you've found some code that looks superfluous. The compiler doesn't notice that.). And even late bound function or objects are typically defined in two places, e.g. an interface or map file, so the build would still fail. – John Wu Mar 8 '17 at 9:48
  • 1
    Hmm... I'm intrigued, I don't think the latter you say is true. It's not for vanilla javascript (not precompiled), ruby, python or smalltalk for example, right? If you refer to stuff that doesn't exist, those error only at runtime. – eis Mar 8 '17 at 10:31
  • Maybe we disagree on what is mean by "binding" which to me indicates the resolution of a logical representation to a physical representation, e.g. a symbol to an address. Javascript functions are stored as tag/value pairs within the containing object, so any notion of binding seems non sequitur. – John Wu Mar 8 '17 at 20:46
  • 1
    Yes, we really disagree. The term has different meaning on languages that don't have full late binding (like I explained in the first comment). When you bind methods at runtime - in languages that can add and remove methods runtime, on the fly - you are using late binding in the way I understand it to be done. This is true in ruby/python/smalltalk-style languages, and there you don't have separate map files. Since especially ruby and python are very widely used, I disagree about your notion what it would "typically" mean. – eis Mar 8 '17 at 21:44
4
  • I would start using a code analyzer to check whether this is dead code
  • I would start checking my tests and trying to reach the code. If I am not able to reach the code within a test case it might be dead code
  • I would remove the code and throw an exception instead. For one release the exception is activated, and in the second release the exception can be removed. To be safe, place an emergency flag (in Java, a system property) to be able to activate the original code, when a customer notices the exception. So the exception can be disabled and the original code can be activated in a production environment.
  • 6
    -1. It is absolutely not recommendable to make these changes in productive code, safety flag or not. Only when one is sure the code in question isn't used should it be removed in productive code. That kind of messing-around is exactly why there should always be different productive and testing environments. – Johannes Hahn Mar 7 '17 at 16:18
  • 3
    This will work in fantasy when test coverage is nearly 90%, but what if not and you have a project with 100.000 lines of code. Thats why i described to try testing the code and if no test is able to reach the code... it might be dead. – Markus Lausberg Mar 8 '17 at 6:40
  • IMHO If you are unsure enough to hold yourself from removing the code entirely - you should not be throwing exceptions in production - logging suggested by the OP is a much better alternative, achives the same, and you can get diagnostics instead of relying on customers complaining – Sebi Mar 8 '17 at 19:15
  • @JohannesHahn I think you mean, "production code," which is code that runs in a production environment. – jpmc26 Mar 9 '17 at 2:18
  • @jpmc26 Yes, of course. – Johannes Hahn Mar 9 '17 at 8:18
3

Removing unreachable code

In a principled statically typed language, you should always know whether the code is actually reachable or not: remove it, compile, if there is no error it was not reachable.

Unfortunately, not all languages are statically typed, and not all statically typed languages are principled. Things that could go wrong include (1) reflection and (2) unprincipled overloading.

If you use a dynamic language, or a language with sufficiently powerful reflection that the piece of code under scrutiny could potentially be accessed at run-time via reflection, then you cannot rely on the compiler. Such languages include Python, Ruby or Java.

If you use a language with unprincipled overloading, then merely removing an overload could simply switch the overload resolution to another overload silently. Some such languages allow you to program a compile-time warning/error associated with the usage of the code, otherwise you cannot rely on the compiler. Such languages include Java (use @Deprecated) or C++ (use [[deprecated]] or = delete).

So, unless you are very lucky to work with strict languages (Rust comes to mind), you may really be shooting yourself in the foot by trusting the compiler. And unfortunately test suites are generally incomplete so of not much more help either.

Cue the next section...


Removing potentially unused code

More likely, the code is actually referenced, however you suspect that in practice the branches of code that reference it are never taken.

In this case, no matter the language, the code is demonstrably reachable, and only run-time instrumentation can be used.

In the past, I've successfully used a 3-phases approach to removing such code:

  1. On each branch suspected NOT to be taken, log a warning.
  2. After one cycle, throw an exception/return an error upon entering the specific piece of code.
  3. After another cycle, delete the code.

What's a cycle? It's the cycle of usage of the code. For example, for a financial application I would expect a short monthly cycle (with salaries being paid at the end of the month) and a long yearly cycle. In this case, you have to wait at least a year to verify that no warning is ever emitted for the end-of-year inventory could use code paths that are otherwise never used.

Hopefully, most applications have shorter cycles.

I advise putting a TODO comment, with a date, advising on when to move on to the next step. And a reminder in your calendar.

  • 1
    Actually, in C++ you can mark your suspect overload as [[deprecated]] to identify sites that call that version; then you can inspect them to see if their behaviour will change. Alternatively, define your overload as = delete - in this case, you will need to explicitly cast arguments to use one of the other versions. – Toby Speight Mar 8 '17 at 11:15
  • @TobySpeight: That's true, and a good alternative too. Let me edit that in. – Matthieu M. Mar 8 '17 at 11:34
  • "Doctor, it hurts when I do this." "Well don't do that!" Don't use approaches which make the code unmanageable. – user251748 Mar 9 '17 at 14:23
2

Removing code from production is like cleaning house. The moment you throw away an object from the attic, your wife will kill you the next day for throwing away her great grand mother's third nephew's homecoming gift from their neighbor who died in 1923.

Seriously, after cursory analysis using various tools that everyone has already mentioned, and after using the stepped deprecation approach already mentioned, keep in mind that you may be gone from the company when the actual removal takes place. Letting the code remain and have its execution logged and ensured that any alerts of its execution are definitely communicated to you (or your successors and assigns) is essential.

If you don't follow this approach, you're likely to be killed by your wife. If you keep the code (like every keepsake) then you can rest your conscience in the fact that Moore's law comes to your rescue and the cost of junk disk space used by the code that feels like a "rock in my shoe", reduces every year and you don't risk becoming the center of multiple water cooler gossips and weird looks in the hallways.

PS: To clarify in response to a comment.. The original question is "How do you safely delete.." of course, Version Control is assumed in my answer. Just like digging in the trash to find the valuable is assumed. No body with any sense throws away code and version control should be part of every developer's rigor.

The problem is about code that may be superfluous. We can never know a piece of code is superfluous unless we can guarantee 100% of the execution paths do not reach it. And it's assumed that this is a large enough piece of software that this guarantee is next to impossible. And unless I read the question wrong, the only time this whole thread of conversation is relevant is for cases during production where the removed code might get called and hence we have a runtime/production issue. Version control does not save anyone's behind due to production failures, so the comment about "Version Control" is not relevant to the question or my original point, i.e. properly deprecate over an extremely long time-frame if you really have to, otherwise don't worry because superfluous code adds relatively a very small cost in disk space bloat.

IMHO, the comment is superfluous and is a candidate for removal.

  • 12
    If I had the equivalent of version control, my wife's objection will be easily reversed. So too with deleting code: it's not a killing offense. The repository is never thrown away. – JDługosz Mar 8 '17 at 8:04
  • I thought that Object Oriented programming was supposed to narrow the scope in which a method could be called. So it should lead to code that is more easily understood and managed, right? Otherwise, why are we doing it that way? – user251748 Mar 9 '17 at 14:25
  • @nocomprende Nowhere in this thread is it indicated that the discussion is limited to OOP design/languages only. I'm not sure why you think that's relevant. – underscore_d Mar 12 '17 at 14:07
1

Maybe the compiler does notice that, it just doesn't make a fuss.

Run a build with full compiler optimizations, geared towards size. Then delete the suspect code, and run the build again.

Compare the binaries. If they're identical, then the compiler has noticed and silently deleted the code. You may delete it from source safely.

If the binaries are different... then it's inconclusive. It could be some other change. Some compilers even include date and time of compilation in the binary (and maybe it can be configured away!)

  • 1
    Not all languages are compiled, not all compilers have optimisation and not all optimisations are equal, many do not remove un-invoked code just in case that code is there for a reason. If the code lives in a shared library/DLL it will not be deleted. – Steve Barnes Mar 11 '17 at 6:50
  • 1
    @SteveBarnes Well yeah, if you're not doing LTO and static-linking everything, you're in for some pain. That's a given. – Navin Mar 13 '17 at 11:21
1

I've actually recently come across this exact situation, with a method called "deleteFoo". Nowhere in the entire codebase was that string found other than the method declaration, but I wisely wrote a log line at the top of the method.

PHP:

public function deleteFoo()
{
    error_log("In deleteFoo()", 3, "/path/to/log");
}

It turns out that the code was used! Some AJAX method calls "method:delete,elem=Foo" which is then concatenated and called with call_user_func_array().

When in doubt, log! If you've gone sufficient time without seeing the log filled, then consider removing the code. But even if you remove the code, leave the log in place and a comment with the date to make finding the commit in Git easier for the guy who has to maintain it!

0

Use grep or whatever tools you have available first, you might find references to the code. (Not originally my instruction/advice.)

Then decide if you want to risk removing the code, or if my suggestion could come to use:

You could modify the function/block of code to write that it has been used to a log file. If no such message is "ever" recorded in the log file, then you can probably remove the logging code.

Two problems:

  1. Getting the log from other users. Perhaps you can set a global flag that will crash the program on exit and kindly ask the user to send you the error log.

  2. Tracing the caller. In some high level languages (eg. Python) you can easily get a traceback. But in compiled languages you'll have to save the return address to the log and force a core dump on exit (point 1).
    In C, this should be quite simple: use a conditional block (eg. #ifdef ... #endif) to only use this when you know it'll work (and have tested that it does), and simply read the return address from the stack (may or may not require inline assembly language).

Saving the arguments in the log file may or may not be useful.

0

If you know what the code surrounding it does, test it to see if it breaks. This is the simplest and most cost-effective way to refactor a piece of code you're working on, and should be done anyway as a matter of developing any change to the code you're working on in the first place.

If, on the other hand, you're refactoring a piece of code where you do not know what it does, do not remove it. Understand it first, then refactor it if you determine it to be safe (and only if you can determine that the refactoring would be a proper use of your time).

Now, there might be some circumstances (I know I've run into it myself) where the code that is 'dead' is actually not connected to anything. Such as libraries of code developed by a contractor that never actually got implemented anywhere because they became obsolete immediately after the app was delivered (why yes, I do have a specific example in mind, how did you know?).

I personally did wind up removing all of this code, but it is a huge risk that I do not recommend taking on lightly - depending on how much code this change could potentially impact (because there's always the potential that you've overlooked something) you should be extraordinarily careful and run aggressive unit tests to determine if removing this ancient legacy code will break your application.

Now all of that being said, it probably isn't worth your time or sanity to try and remove code like that. Not unless it's becoming a serious issue with your application (for example, not being able to complete security scans because of application bloat...why yes I DO still have an example in mind), so I wouldn't recommend it unless you reach such a situation where the code has truly started to rot in an impactful way.

So in short - if you know what the code does, test it first. If you don't, you can probably leave it alone. If you know you can't leave it alone, dedicate your time to test the change aggressively before implementing the change.

0

My approach, which I always assumed to be industry standard in this case, weirdly hasn't been mentioned so far:

Get a second pair of eyes.

There are different kinds of "unused code" and in some cases deleting is appropriate, in other cases you should refactor it, and in yet other cases you run away as far as you can and never look back. You need to figure out which one it is, and to do so you best pair up with a second - experienced! - developer, one who is very familiar with the code base. This significantly reduces the risk for an unnecessary mistake, and allows you to make the necessary improvements to keep the code base in a maintainable state. And if you don't do this, at some point you run out of developers who are very familiar with the code base.

I've seen the logging approach too - to be more exact, I've seen the logging code: even more dead code which was left in there for 10 years. The logging approach is quite decent if you're talking about removing entire features, but not if you're just removing a small piece of dead code.

0

The simple answer to your question is you can't safely delete code you don't understand, which includes not understanding how it gets called. There will be some risk.

You will have to judge how best to mitigate that unavoidable risk. Others have mentioned logging. Logging can of course prove that it is used, but can't prove that it isn't. Since the major problem with dead code is that it gets maintained, you might be able to get away with adding a comment saying that it is suspected dead code, and not to do any maintenace on it.

If the code is under source control, you can always find out when it was added and determine how it was called then.

Ultimately, you either understand it, leave it alone, or take a chance.

0

In case the developer of the code in question is still available, review the code removal with him. Also, read comments in the code.

You will get the proper explanation, why this code has been added and which function does it serve for. It can easily be support for the specific use case, or you even may not have enough expertise to judge if it is really not required. In extreme case, one may remove all free statements from a C program and fail to notice anything wrong (it may take few minutes to run out of memory).

It is really a bad culture when the author of the code sits next to you, yet you see no reason to talk because you are sure he just writes bad code, and yours is all so perfect. And, of course, he just writes random words in the comments, no need to waste time reading.

One of the most important reasons to write comment in the code is to explain WHY it has been implemented this way. You may disagree with the solution, but you need to take the problem into consideration.

Discussion with the author may also reveal that the code is the historical remnant of something removed or never finished, so is safe to delete.

-1

I strongly agree with Markus Lausberg's first point: The code should definitely be analyzed with the help of tools. Ape brains are not to be trusted with this kind of task!!

Most standard programming languages can be used in IDEs and every IDE that's worth being called that has a "find for all usages"-feature which is exactly the right tool for this kind of situation. (Ctrl+Shift+G in Eclipse for example)

Of course: Static analyzer tools are not perfect. One must be aware of more complicated situations where the decision that the method in question is being called happens only at runtime and no sooner (calls via Reflection, calls to "eval"-functions in scripting languages etc).

  • 5
    Maybe ape brains should not be writing code? "Untouched by human hands" used to be a marketing phrase. I always imagined gorilla hands doing the work. – user251748 Mar 7 '17 at 16:44
  • 3
    (Ape brains wrote the tools too) (Shhh! You'll ruin the story!) – user251748 Mar 7 '17 at 16:46
  • 1
    I think you're both missing my point. I clearly said that there is no guarantee that static code analysis will find every invocation of a method or class. It is, as it says on the tin, a static analysis. It cannot and should be not be expected to find dynamic invocations via reflection, eval() etc. My point is that static code analysis should be considered a necessary prerequisite for deleting a piece of code. And the bigger the project is the less likely it becomes that a human brain is even capable of doing that analysis at all let alone doing it correctly... – Johannes Hahn Mar 9 '17 at 14:23
  • 1
    ... static code analysis is tedious, repetitive work which is best done by a computer. Exactly the kind of task that can be reliably executed by computers but is notoriously unreliable when executed by an ape brain. The latter should only be used for things a computer cannot do like finding those tricky reflections and eval-calls. – Johannes Hahn Mar 9 '17 at 14:26
  • 1
    Again, missing the point. Even if the tools aren't perfect because they were written by ape brains (and I'm not even sure if I should grant that! Computing static call hierarchies isn't that difficult), they are still far superior to manual task execution. This isn't an all-or-nothing-kind of situation. A 99% reliable tool is still preferable to a brain that maybe starts with 99% percent but quickly falls to much lower rates after the first thousand lines of code. – Johannes Hahn Mar 9 '17 at 14:34
-1

Two possibilities:

  1. You have 99%+ test coverage: Delete the code and be done with it.
  2. You don't have trustworthy test coverage: Then the answer is to add a deprecation warning. I.e., you add some code to that place that does something sane (log a warning to some easily visible place that does not conflict with day-to-day-usage of your application, for example). Then let it simmer for a few months/years. If you never ever found any occurence of it, replace the deprecation by something that throws an exception. Let that stand for a while. Finally, remove everything completely.
  • 2
    this doesn't seem to offer anything substantial over points made and explained in prior 17 answers. Both coverage and deprecation warning were discussed over there already – gnat Mar 9 '17 at 20:53
  • @gnat, you are right. When I first scanned the answers, for some reason I thought I'd seen several ones that at the top that did not shortly and succinctly get to these two point. In the middle of the answers there are some like that. – AnoE Mar 10 '17 at 7:49

protected by gnat Mar 8 '17 at 14:57

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.