Code coverage highlights unused methods - what should I do?

Question

I have been tasked to increase code coverage of an existing Java project.

I noticed that the code coverage tool (EclEmma) has highlighted some methods that are never called from anywhere.

My initial reaction is not to write unit tests for these methods, but to highlight them to my line manager/team and ask why these functions are there to begin with.

What would the best approach be? Write unit tests for them, or question why they're there?

Answer 1

1. Delete.
2. Commit.
3. Forget.

Rationale:

1. Dead code is dead. By its very description it has no purpose. It may have had a purpose at one point, but that is gone, and so the code should be gone.
2. Version control ensures that in the (in my experience) rare unique event that someone comes around later looking for that code it can be retrieved.
3. As a side effect, you instantly improve code coverage without doing anything (unless the dead code is tested, which is rarely the case).

---

Caveat from comments: The original answer assumed that you had thoroughly verified that the code is, beyond doubt, dead. IDEs are fallible, and there are several ways in which code which looks dead might in fact be called. Bring in an expert unless you're absolutely sure.

Answer 2

All other answers are based on the assumption that the methods in question are really unused. However, the question didn't specify whether this project is self-contained or a library of some sort.

If the project in question is a library, the seemingly unused methods may be used outside of the project and removing them would break those other projects. If the library itself is sold to customers or made available publicly, it may be even impossible to track down the usage of these methods.

In this case, there are four possibilities:

• If the methods are private or package-private, they can be safely removed.
• If the methods are public, their presence may be justified even without actual usage, for feature completeness. They should be tested though.
• If the methods are public and unneeded, removing them will be a breaking change and if the library follows semantic versioning, this is only allowed in a new major version.
• Alternatively, public methods can also be deprecated and removed later. This gives some time for API consumers to transition over from the deprecated functions before they get removed in the next major version.

Answer 3

First check that your code coverage tool is correct.

I've had situations where they haven't picked up on methods being called via references to the interface, or if the class is loaded dynamically somewhere.

Answer 4

As Java is statically compiled, it should be pretty safe to remove the methods. Removing dead code is always good. There is some probability that there is some crazy reflection system which runs them in runtime, so check first with other developers, but otherwise remove them.

Answer 5

What would the best approach be? Write unit tests for them, or question why they're there?

Deleting code is a good thing.

When you can't delete the code, you can certainly mark it as @Deprecated, documenting which major release you are targeting to remove the method. Then you can delete it "later". In the mean time, it will be clear that no new code should be added that depends upon it.

I would not recommend investing in deprecated methods - so no new unit tests just to hit coverage targets.

The difference between the two is primarily whether or not the methods are part of the published interface. Arbitrarily deleting parts of the published interface can come as an unpleasant surprise to consumers who were depending on the interface.

I can't speak to EclEmma, but from my experiences one of the things that you need to be careful of is reflection. If, for instance, you use text configuration files to choose which classes/methods to access, the used/unused distinction may not be obvious (I've been burned by that a coupled times).

If your project is a leaf in the dependency graph, then the case for deprecation is weakened. If your project is a library, then the case for deprecation is stronger.

If your company uses a mono-repo, then delete is lower risk than in the multi-repo case.

As noted by l0b0, if the methods are already available in source control, recovering them after deletion is a straight forward exercise. If you were really worried about needing to do that, give some thought to how to organize your commits so that you can recover the deleted changes if you need them.

If the uncertainty is high enough, you could consider commenting out the code, rather than deleting it. It's extra work in the happy path (where the deleted code is never restored), but it does make it easier to restore. My guess is that you should prefer a straight delete until you have been burned by that a couple of times, which will give you some insights on how to evaluate "uncertainty" in this context.

question why they're there?

Time invested in capturing the lore is not necessarily wasted. I've been known to perform a remove in two steps -- first, by adding and committing a comment explaining what we've learned about the code, and then later deleting the code (and the comment).

You could also use something analogous to architectural decision records as a way of capturing the lore with the source code.

Answer 6

A code coverage tool is not all-knowing, all-seeing. Just because your tool claims that the method is not called, that doesn't mean it isn't called. There is reflection, and depending on the language there may be other ways to call the method. In C or C++ there may be macros that construct function or method names, and the tool might not see the call. So the first step would be: Do a textual search for the method name, and for related names. Ask experienced colleagues. You may find it is actually used.

If you are not sure, put an assert() at the start of each "unused" method. Maybe it gets called. Or a logging statement.

Maybe the code is actually valuable. It may be new code that a colleague has been working on for two weeks, and that he or she was going to turn on tomorrow. It's not called today because the call to it is going to be added tomorrow.

Maybe the code is actually valuable part 2: The code may be performing some very expensive runtime tests that would be able to find things going wrong. The code is only turned on if things actually go wrong. You may be deleting a valuable debugging tool.

Interestingly, the worst possible advice "Delete. Commit. Forget." is the highest rated. (Code reviews? You don't do code reviews? What on earth are you doing programming if you don't do code reviews? )

Answer 7

Depending on the environment the software runs in, you could log if the method is ever called. If it's not called within a suitable period of time, then the method can be safely removed.

This is a more cautious approach than just deleting the method, and may be useful if you are running in a highly fault-sensitive environment.

We log to a dedicated #unreachable-code slack channel with a unique identifier for each candidate for removal, and it's proved to be pretty effective.