59

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?

118
  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.

  • 118
    Generally I would agree with this approach, but if you are new to a project and/or a junior developer, better ask your mentor/superior before deleting. Depending on the project some methods might not look used but be called/used by outside code not in the project code you have access to. It might be autogenerated code so your removal will achieve nothing but log history noise. Your IDE may not be able to find reflection based access from within the project. Etc. pp. If you are not sure about such corner-cases at least ask once. – Frank Hopkins May 28 '18 at 12:09
  • 11
    Though I agree with the core of this answer, the literal question was "should I question why they are there?". This answer might give the impression the OP should not ask their team before deleting. – Doc Brown May 28 '18 at 12:30
  • 58
    I'd add a step first though - check the git blame log for the unused code lines. If you can find the person who wrote them you can ask what they're for. If the lines are new then there's a good chance someone is planning to use them soon (in which case they should probably be unit tested now). – bdsl May 28 '18 at 13:15
  • 10
    There are so many things wrong with this answer, as expressed in comments or other answers, that I can't believe this is the top voted and accepted answer. – user949300 May 28 '18 at 17:15
  • 11
    @Emory The best way to start in a new team is to break the build by carelessly deleting things because you thought nobody needed them. Guarantees that you're popular from day 1. Sure it might not be needed (because every large, older application always has 100% code coverage cough), but that's a very bad ROI. – Voo May 29 '18 at 18:58
55

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.
  • 9
    Plus if it is a library the functions are there for completeness sake – PlasmaHH May 28 '18 at 16:13
  • Can you elaborate on how they should be tested? If you don't know why the method is there, you probably don't know what it's supposed to do. – ReinstateMonicaSackTheStaff May 30 '18 at 15:34
  • Method names like filter_name_exists, ReloadSettings or addToSchema (randomly picked from 3 arbitrary open source projects) should provide a few hints to what the method is supposed it to do. A javadoc comment may be even more useful. Not a proper specification, I know, but could be enough to create a few tests that may prevent regressions at least. – Zoltan May 30 '18 at 18:51
  • 1
    public methods on a private class or interface should not be considered public for this purpose. similarly if a public class is nested inside a private class, it is not really public. – emory May 31 '18 at 18:27
30

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.

  • Thanks, will do. On Eclipse, I do a search on code-base for function, and nothing comes up. If you have any other suggestions of how to do a more comprehensive search, I'd be most grateful. – Lucas T May 28 '18 at 10:18
  • 3
    yes, you should check other projects which might import the class as a dll – Ewan May 28 '18 at 10:21
  • 7
    This answer feels incomplete. "First check that your code coverage tool is correct. If it is, then.... [insert rest of answer]". Specifically, the OP wants to know what to do if the code coverage tool is correct. – Jon Bentley May 28 '18 at 23:07
  • 1
    @jonbentley The OP is asking for the best approach to the tools report. "Check it manually" because its pretty obvious from the context that its incorrect – Ewan May 29 '18 at 3:10
15

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.

  • 9
    +1 for checking with people, it's kind of following the least surprise principle. You don't want someone to spend too much time looking for the method the just left there a few commits earlier. Also, in some edge cases dead code may be the new stuff that is already checked in but not wired anywhere yet (though in this case it should be well-documented with comments, and tested). – Frax May 28 '18 at 11:00
  • 4
    Well unless the code uses reflection to call the "unused" methods, but in that case you have far bigger problems, and we look forward to see the code at thefailywft.com (Do a quick test to se if any code does a ClassName.class). – MTilsted May 28 '18 at 11:51
  • 2
    It's statically compiled, but there's also the invokedynamic instruction, so, ya know... – corsiKa May 28 '18 at 21:05
  • @MTilsted: There are many frameworks that will call code by using strings - I can think of at least Spring and Hibernate off the top of my head. – jhominal May 29 '18 at 8:57
9

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.

9

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? )

  • I respectfully disagree about "DELETE. COMMIT. FORGET" being the worst advice. (I think it is the best.). Your approach is also OK. I think the worst possible advice would be to write unit tests that exercise the "dead code" but make no assertions. The code coverage tool will be fooled into thinking they are used. – emory May 29 '18 at 0:11
  • 3
    Nothing in the "Delete. Commit. Forget" answer says not to do code review. It's perfectly possible (and advisable, IMHO) to review code after it's been committed (but before deployment :-) ). – sleske May 29 '18 at 7:54
  • @sleske How do you review code that isn't there anymore? Nor does that answer mention "review". – user949300 May 29 '18 at 14:03
  • @user949300: Whether or not a code change needs review or not is a separate question, and independent of whether code is added, changed or deleted (adding code can be even more disastrous than deleting it, see e.g. the Heartbleed vulnerability). Plus, the answer (now) says to "bring in an expert", which sounds pretty close to a code review. – sleske May 30 '18 at 7:27
  • 1
    @user949300: As to "How do you review code that isn't there anymore" - I'd hope that is obvious: By looking at the change in your version control tool of choice. How else would you review changes (any changes)? – sleske May 30 '18 at 7:29
6

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.

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