60

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?

Peter Mortensen
  • 1,050
  • 2
  • 12
  • 14
Lucas T
  • 753
  • 1
  • 5
  • 9
  • 1
    Comments are not for extended discussion; this conversation has been [moved to chat](https://chat.stackexchange.com/rooms/78203/discussion-on-question-by-lucas-t-code-coverage-highlights-unused-methods-what). – maple_shaft May 30 '18 at 15:41

7 Answers7

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.

l0b0
  • 11,014
  • 2
  • 43
  • 47
  • 119
    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
  • 1
    @DocBrown, yes. I agree with you, and with OP. I will ask before deleting unused functionality, as a precautionary measure. – Lucas T May 28 '18 at 12:34
  • 5
    Is reflection not a caveat here? Maybe not applicable to OP's application, but I'd be apprehensive of deleting a (supposedly unused) method if I know my application uses reflection at some point. – Flater May 28 '18 at 13:06
  • 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
  • @bdsl right. Though then again, if someone just _plans_ to use these methods but doesn't yet actually use then, shouldn't they keep them in a separate branch outside master for the time being? – leftaroundabout May 28 '18 at 13:22
  • 3
    @leftaroundabout typically they should, but mistakes/suboptimal things happen. Sometimes they are systemically needed (e.g. if they are exported and another project uses this one as a dependency). If you don't know the project well, it's possibly more efficient and socially beneficial to first ask. – Frank Hopkins May 28 '18 at 13:31
  • @Darkwing if you're writing a library and exporting the methods, they aren't unused (regardless of whether some third party actually does use them) and would not be highlighted as unused by the code coverage tool. – leftaroundabout May 28 '18 at 13:34
  • @leftaroundabout true, the example was bad, it would require quite a hack that shouldn't be there in the first place. Still, my main point that suboptimal things happen stands. Especially when you are new/unexperienced, I wouldn't start enforcing the "proper way" by simply removing code without asking/notification. – Frank Hopkins May 28 '18 at 13:57
  • 3
    Deleting code based on a code coverage tool is reckless at best. Absent necessary caveats this answer is simply bad advice. – Jack Aidley May 28 '18 at 14:49
  • @leftaroundabout No, if you accept the argument that Trunk Based Development is generally the best workflow then all work in progress should be on the one branch for the entire team to see. Opens it up to discussions about what the functions are for and whether they are well written, which are better to have now than next week when stuff has been built on top of them. These functions may include calls to other functions. If someone decides to rename one of those other functions stuff would break if it was on separate branches. – bdsl May 28 '18 at 15:59
  • @bdsl that seems a very strange argument. If the new functions are developed on a separate branch, it is much easier to keep track of what was renamed when, and match changes, without noise from unrelated commits in between. (You can even re-write the history to avoid the initial names popping up in the first place if the team decides on better ones, but that's controversial practice.) And, code review is orthogonal to branching – you can just as well push other branches in addition to master, without needing to merge anything yet. – leftaroundabout May 28 '18 at 16:07
  • 1
    It's not an original argument. You can longer versions of it at http://www.davefarley.net/?p=247 https://twitter.com/jezhumble/status/982988915740168192 https://www.infoq.com/news/2018/04/trunk-based-development and https://trunkbaseddevelopment.com – bdsl May 28 '18 at 16:10
  • 3
    "Dead code is dead". The point is : you don't know if the code is dead or if it just smells funny. – Eric Duminil May 28 '18 at 16:18
  • I wasn't talking about a rename of one of the new functions, I was talking about a rename of an older function which a new one depends on. – bdsl May 28 '18 at 16:21
  • 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
  • 2
    This answer is basically correct. The only thing I would nitpick is there is no good reason to verify that the code is in fact dead. **JUST DELETE IT** If the code is in fact needed, then whomever needs it should have written an appropriate unit test. Let them clean it up. – emory May 29 '18 at 00:06
  • 3
    @emory By definition, since the new developer is being asked to improve code-coverage, this is not a test-driven design and writing tests is not an expected part of general development. Which is its own problem, sure, but it’s still *true*, and a new developer isn’t doing him-or-herself any favors by summarily deleting something and then turning around and saying “well that’s because your design/process is bad” if someone complains. – KRyan May 29 '18 at 02:45
  • 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
  • @Voo of course the application has less than 100% code coverage. if it had 100% coverage, (1) the OP could call the task complete and move on to something else; and (2) there would be no "dead code" identified. Breaking the build is a great thing. If there is a build to break, I would recommend that OP incrementally delete code until there is no "dead code" left that can be deleted without breaking the build. Then the OP should write tests for the remaining "dead code." This will bring the coverage up to 100%. – emory May 29 '18 at 19:31
  • 1
    Maybe add step 0: add copious logging in these dead method and see if anything pops up in production for "some" time. Or, harshed, just raise an exception in them so that you should be able to very rapidly see who uses them (removing them has the same effect, but with logging or internal exception raising you will have on your side details on who used them). – Patrick Mevzek May 29 '18 at 21:51
  • 3
    @emory "Breaking the build is a great thing". And what about the dozens of people who might want to also get some work done and get stopped by a non-working build? Particularly cross-component check-ins that ruin other teams builds will be noticed and not in any positive way. [There's a reason many, many teams have policies to discourage such cavalier behavior with shared resources](https://blogs.msdn.microsoft.com/oldnewthing/20051207-17/?p=33063). – Voo May 30 '18 at 07:09
  • 2
    Build breaking is what branches are for. Do what you think is needed and run it through the products acceptance test. If the product doesn't have an acceptance test, you have found amore urgent thing to do. – Thorbjørn Ravn Andersen May 30 '18 at 17:07
  • @Voo your link is from 2005. Read this essay from 2013 https://dzone.com/articles/breaking-build-not-crime "I just want to commit/push my new stuff and let CI server perform full testing. I hope everything flies but if not, I don't want to feel guilty. I don't want to stay late or apologize my team-mates." If your team is still using best practices from 2005 and disregarding innovations championed in 2013 (it is 2018 now) then you have more urgent things to do. – emory May 31 '18 at 16:11
  • @Voo RE shame: Bruce Morgan ~ "This bit of negative reinforcement had some interesting side effects. Some people actually felt so shamed, they went to extreme productivity killing efforts". Jon ~ "I actually relish getting these items so I can put them in my drawer and stop that retarded practice" Les C. ~ "if someone in a company tried to publicly embarrass me like this for a genuine mistake, I would (a) complain to senior management and/or (b) resign." Norman Diamond ~ "But please, could someone explain why breaking the build is such a big deal?" – emory May 31 '18 at 16:21
  • @emory You either only work on toy programs, very young programs that are command line only or you will *never* have 100% code coverage. And even if you had 100% code coverage you still couldn't test interactions with external interfaces. Yes we do use CI, our code despite being almost two decades old has 80% code coverage and nothing gets checked into the main branch that hasn't run the complete build, including tests and static checks. And you know what? If I wanted to I could still check in code that would cause troubles at runtime, because that's how complex systems work. – Voo May 31 '18 at 16:22
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.
Zoltan
  • 631
  • 4
  • 5
  • 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. – StackOverthrow 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.

Ewan
  • 70,664
  • 5
  • 76
  • 161
  • 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 03: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.

max630
  • 2,543
  • 1
  • 11
  • 15
  • 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 08: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.

VoiceOfUnreason
  • 32,131
  • 2
  • 42
  • 79
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? )

Lucas T
  • 753
  • 1
  • 5
  • 9
gnasher729
  • 42,090
  • 4
  • 59
  • 119
  • 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 00: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 07: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 07: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 07:29
  • @sleske In my (limited) experience in code reviews, we have never looked at deleted code. Is your experience different? – user949300 May 30 '18 at 16:05
  • @user949300: Well, I can only speak for myself, but I do look at deleted code (in Git it's part of the diff). I can't think of a reason not to look at it. BTW, that might make a good new question (hint :-)). – sleske May 30 '18 at 21:23
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.

Jamie Bull
  • 464
  • 3
  • 11