Unit testing equals and hashcode - a complexity story

Question

I'm having a moral dilemma. I have some value objects in my application, which are immutable and extremely simple. I've generated the equals and hashcode with an IDE (intellij in my case) but doing that, made the code coverage drop, plus the reports now indicate that those value objects are very complex (using the cyclomatic complexity metric) when in fact they're dead simple.

As an example, the following equals is in a value object that has 3 immutable attributes. The Code complexity is 14 (javaNCSS) and it has 26 execution branches (Cobertura). I should add too, that I fail the build if any method has a complexity greater than 10.

@Override
public boolean equals(Object o) {
    if (this == o) {
        return true;
    }
    if (o == null || getClass() != o.getClass()) {
        return false;
    }

    TranscriptTaskDetails that = (TranscriptTaskDetails) o;

    if (inputFile != null ? !inputFile.equals(that.inputFile) : that.inputFile != null) {
        return false;
    }
    if (language != that.language) {
        return false;
    }
    if (outputFile != null ? !outputFile.equals(that.outputFile) : that.outputFile != null) {
        return false;
    }

    return true;
}

I'm wondering what other devs use to circumvent this, as I pay quite a lot of attention to the complexity reports, as in my experience a high complexity metric relates to more bugs, so this auto-generated equals and hashcodes are polluting the reports.

I'm thinking of using EqualsBuilder and HashcodeBuilder from apache commons-lang to circumvent this, but I'm not 100% happy :S.

Edit

I should have added that part of the code I'm writing for this project is a library that will be used by other business units... And will be maintained by a different team too :S.

score 7 · Accepted Answer · answered Apr 23 '11 at 13:40

7

I'm thinking of using EqualsBuilder and HashcodeBuilder from apache commons-lang to circumvent this, but I'm not 100% happy :S.

Why not use these?

Using them reduces the size and complexity of your own methods and it's much easier to verify visually that equals and hashCode are implemented consistently.

It's of course also a good idea and pretty easy to test that the equals/hashCode contract is satisfied, so by all means, write tests.

answered Apr 23 '11 at 13:40

Don Roby

40,677
6
91
113

I think this is the best answer, as the other discouraged to check the tests. In the end, I've found that some of the equals/hashcodes were added just for testing purposes (it is bad to add production code for testing purposes). And in the cases that we actually needed a equals/hashcode, we found that we would *always* (aka until this changes) want to compare all the fields of the value object for equality. – Augusto Apr 26 '11 at 13:38
It is in no way a bad thing to have code in your projects that exists for the sole purpose of testing. Testability and test coverage are core properties of good code. And removing the test code after testing would mean that you haven't actually tested the code you're delivering. – yeoman May 28 '16 at 05:33

Stephen C · Answer 2 · 2011-04-23T13:56:04.553

I think that your real problem is placing too much faith in artificial measure such as code coverage cyclomatic complexity.

The plain facts are that:

the generated code is correct ... modulo that you have chosen the correct model of equality, and
the generated code is not complicated ... or at least, no more complicated than it needs to be.

Learn to trust your own judgment, and stop relying on tools to make your design decisions for you.

And the corollary of what I just said is that if you think you can (and should) simplify the generated code while still ensuring that it is correct for your current and projected use cases, go ahead and simplify it.

By the way, a generated equals / hashcode pair are probably more likely to be more correct than a hand written one ... written by an "average bear" programmer. For instance, note the careful way that the generated code handles null fields and the comparison of the types. A lot of developers wouldn't get those right.

score 3 · Answer 3 · edited May 23 '17 at 12:09

I think your goals of low complexity and high coverage are admirable. I've found that the Google Guava library is quite useful in this kind of problem (and also great for other problems.) You could write the following:

@Override
public boolean equals(Object o) {
    if (this == o) {
        return true;
    }
    if (o == null || getClass() != o.getClass()) {
        return false;
    }

    TranscriptTaskDetails that = (TranscriptTaskDetails) o;
    return Objects.equal(inputFile, that.inputFile) && Objects.equal(language, that.language) && Objects.equal(outputFile, that.outputFile);

}

You could then use the EqualsTester to quickly get coverage of this method. You get simplicity and testing coverage. Thanks to this SO question for helping me find EqualsTester.

Another option that I've recently started using is Lombok. Lombok is a great library for writing immutable value objects. Here is a sample lombok class:

@Data
public class Value {
    private final String field1;
    private final Foo field2;
}

The @Data annotation causes lombok to generate a bunch of code for you. You get getters (and for non-final fields) setters. You get equals, hashCode, and toString. You get an all args constructor. And all those methods are automatically updated when you change the field list. I've trimmed hundreds of lines from my current project with Lombok and my value objects are now always completely implemented.

score 3 · Answer 4 · answered Apr 23 '11 at 13:21

3

There's a contradiction here. You don't have any reason at all to worry about the complexity of auto-generated code. It's the hand coding you should be worrying about.

answered Apr 23 '11 at 13:21

user207421

305,947
44
307
483

Don is right. I wouldn't really mind if the whole class is auto-generated, but since a part of it is auto-generated and "on-demand" rather than automatically, it becomes more of a concern. – Augusto Apr 23 '11 at 13:46
@Don true but so as to increase the complexity? – user207421 Apr 26 '11 at 08:32

score 3 · Answer 5 · answered Apr 23 '11 at 13:24

3

How high is your code coverage? Some people argue that shooting for 100% is a sign of anal retentive tendencies. If you're in the 70-80% range, and you know that what you haven't tested isn't a problem, then why worry about it?

On the other hand, these tests aren't that difficult to write. Why not write them, be done with it, and sleep at night ? You would have finished the test in the time it took to type your moral dilemma here and waiting for answers.

answered Apr 23 '11 at 13:24

duffymo

305,152
44
369
561

1

The line coverage stands at 82% at the moment, and I prefer it to be above 80%. But the branch is at 65% and it's lowered mostly by these methods. The problem is not only the coverage, but the "complexity" they add to the class, which is shown in the reports. I know that I'm being a bit anal about this, but I'm trying to build something that will last rather than thrown away in a couple of years. – Augusto Apr 23 '11 at 13:48

score 1 · Answer 6 · answered May 13 '13 at 22:10

1

Simple POJOs - immutable with or without builders, or mutable, are at best automatically generated from a simple DSL description in the first place, until Java gets direct language support for this simple, yet important feature.

answered May 13 '13 at 22:10

yeoman

1,671
12
15

score 0 · Answer 7 · answered Jul 08 '18 at 17:52

You can reduce complexity as well by making sure certain fields cannot be null because it makes no sense. You can enforce this at construction time and have their values or the complete object be immutable. This saves a lot of null checks in the equals code.

At the same time, your class becomes easier to reason about, and any dependent classes don't have to deal with some fields being null, in turn reducing their complexity.

score 0 · Answer 8 · answered Apr 23 '11 at 13:49

In my opinion, seeing code coverage drop is an indicator that you should have a look at the code and find out by yourself if the drop in coverage is justified. Code metrics on things like generated equals or hashcode are not really important, they are so simple that they just work. Same for simple getters and setters, who really cares if some of them are not covered? (That may be a symptom of some other thing not being tested though, but that is beside the point).

Tools are here to help us write good code and applications... we should not be slaves to them.

Anand Varkey Philips · Answer 9 · 2020-06-08T08:44:27.923

If you are looking for some tool to make up coverage and ensure equals and hashcode is implemented properly, You can try https://jqno.nl/equalsverifier/

I see that you are having immutable fields only and this should be easier.

Additional things you can try to Automatically detect and generate missing assertions for Junit test cases (also known as test amplification): https://github.com/STAMP-project/dspot
PS: I have just started exploring the projects.

Unit testing equals and hashcode - a complexity story

Edit

9 Answers9