Sonarlint, complexity of the "equals() method"

Question

Sonarlint for Eclipse, accuses error:

Refactor this method to reduce its Cognitive Complexity from 64 to the 15 allowed.

Rewrite this method to reduce your cognitive complexity of 64 for 15 allowed

I mean, my method has many ifs and elses, many decision points and I need to decrease this.

In a method of with business rules this until it is "easy", I rewrite the method by separating into several other small methods and "Junitizáveis".

The problem is the method equals(), I have some objects with various fields, and the method equals() need to check the similarity of all these fields, check for null, of equals and == for primitives.

That one of mine equals() is already a method with simple context, it only does a task, is already testable by Junit, but has several decision points within.

What to do in this case? Create several methods, one to test null, one to test equals(), one to test primitives and gather all in the equals() main? Have any better approach?

NOTE: I know I can join several ifs in a single line, but this greatly disrupts the visualization by a human being and I am trying to avoid this practice.

For this class, for example, Sonarlint is accusing that it has complexity of 34:

package testes;

import java.math.BigDecimal;

public class EntidadeGrande {

    private int codUser;
    private String descUser;
    private long idUser;
    private BigDecimal valueX;
    private int scoreUser;
    private int scoreStore;
    private int cdLogo;
    private String nmMother;
    private String sgEstado;
    private String dsBandeira;
    private String codCupom;

    /* UM MONTE DE GET E SET AQUI, HASHCODE TAMBÉM */


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

        final EntidadeGrande e = (EntidadeGrande) o;
        if (getCodUser() != e.getCodUser() 
                || getIdUser() != e.getIdUser()
                || getScoreUser() != e.getScoreUser()
                || getScoreStore() != e.getScoreStore()
                || getCdLogo() != e.getCdLogo()) {
            return false;
        }

        if (getValueX() == null && e.getValueX() != null
                || getValueX() != null && e.getValueX() == null) {
            return false;
        } 

        if (!getValueX().equals(e.getValueX())) {
            return false;
        }

        if (getDescUser() != null && e.getDescUser() == null
                || getDescUser() == null && e.getDescUser() != null) {
            return false;
        }

        if (getNmMother() != null && e.getNmMother() == null
                || getNmMother() == null && e.getNmMother() != null) {
            return false;
        }

        if (getSgEstado() != null && e.getSgEstado() == null
                || getSgEstado() == null && e.getSgEstado() != null) {
            return false;
        }

        if (getDsBandeira() != null && e.getDsBandeira() == null
                || getDsBandeira() == null && e.getDsBandeira() != null) {
            return false;
        }

        if (getCodCupom() != null && e.getCodCupom() == null
                || getCodCupom() == null && e.getCodCupom() != null) {
            return false;
        }

        if (!getDescUser().equals(e.getDescUser())) {
            return false;
        }

        if (!getNmMother().equals(e.getNmMother())) {
            return false;
        }

        if (!getSgEstado().equals(e.getSgEstado())) {
            return false;
        }

        if (!getDsBandeira().equals(e.getDsBandeira())) {
            return false;
        }

        if (!getCodCupom().equals(e.getCodCupom())) {
            return false;
        }

        return true;
    }

}

Answer 1

I hate this kind of tool that puts numbers. 15 is absurdly high in some situations and little in others. To fix this would have to make the code spread (in various methods) without need, it seems to me that everything that is there is one thing. I would shut the mouth of this software. There must be a way to do this with setup and/or annotation.

The problem is not quantity of if and yes of decisions to be all, has not 34 if, but has 34 comparisons that generate booleans. If you want to reduce to just two ifit’s quiet because everyone generates the same result, to tell the truth it needs a ifs (in C# could be done with zero).

See if it disturbs the view:

public boolean equals(Object o) {
    if (o == null || o.getClass() != getClass()) return false;
    final EntidadeGrande e = (EntidadeGrande) o;
    return (getCodUser() == e.getCodUser() && 
            getIdUser() == e.getIdUser() &&
            getScoreUser() == e.getScoreUser() &&
            getScoreStore() == e.getScoreStore() &&
            getCdLogo() == e.getCdLogo()) &&
            ((getValueX() == null || e.getValueX() != null) &&
                (getValueX() != null || e.getValueX() == null)) &&
                getValueX().equals(e.getValueX()) &&
            ((getDescUser() != null || e.getDescUser() == null) &&
                (getDescUser() == null || e.getDescUser() != null)) &&
                getDescUser().equals(e.getDescUser()) &&
            ((getNmMother() != null || e.getNmMother() == null) &&
                (getNmMother() == null || e.getNmMother() != null)) &&
                getNmMother().equals(e.getNmMother()) &&
            ((getSgEstado() == null || e.getSgEstado() != null) &&
                (getSgEstado() != null || e.getSgEstado() == null)) &&
                getSgEstado().equals(e.getSgEstado()) &&
            ((getDsBandeira() == null || e.getDsBandeira() == null) &&
                (getDsBandeira() == null || e.getDsBandeira() != null)) &&
                getDsBandeira().equals(e.getDsBandeira()) &&
            ((getCodCupom() == null || e.getCodCupom() != null) &&
                (getCodCupom() != null || e.getCodCupom() == null)) &&
                getCodCupom().equals(e.getCodCupom());
}

I put in the Github for future reference.

I may have eaten ball in some, so don’t copy and paste.

If you do not want to shut up this analyzer you will have to do what it says and create each group of conditions in different methods and then create methods to paste everything together, getting more confused and less efficient. Ridiculous, but everyone does as they see fit.

If you go this way I would go through the groups you have already created, maybe regrouping a little (you had started well), check if it is null and if it is the same together, which makes more sense even if you keep everything in one method, it will probably be more efficient and more readable.

In C# I would make it much simpler than that, but I know that Java likes verbose and less robust code, I can’t tell if it could do much better than that as it does in C# with the right architecture.

That being said, it’s possible that it might actually be interesting to be in separate methods that control each of the fields individually, because they should be treated in isolation, or is all software engineering needs to be thought out to deal with it, especially if each condition can be used again in some other location, after all we must follow the DRY, that is, to make the good old abstraction, which even has to do with orientation to the object that both defend. But I don’t have enough information to know if this is the right thing to do in this case. Certainly it would be more testable, what I question if it should be the main objective of the code, but if it is to change it should be first to meet the demand of the problem, only then think about the test, and not think about the Sonarlint badly configured.

I wonder if there are more important problems in the whole concept adopted.