SonarLint, complexity of the "equals ()" method

5

SonarLint for Eclipse, accuses the error:

  

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

     

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

That is, my method has many if s and else s, many decision points and I need to decrease it.

In a business rules method, until it is "easy", I rewrite the method by separating into several other small methods and "JUnitizables".

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

This my equals() is already a method with simple context, it only does one task, it is already testable by JUnit, but it has several decision points inside.

What to do in this case? Create multiple methods, one to test null , one to test equals() , one to test primitives, and join all in equals() master? Do you have a better approach?

NOTE: I know that I can join several% s of% s on a single line, but this greatly disrupts the visualization by a human being and I am trying to avoid this practice.

Example, for this class SonarLint is accusing that it has complexity of if :

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;
    }

}
    
asked by anonymous 13.08.2018 / 15:13

1 answer

4

I hate this type of tool that puts numbers. 15 is absurdly loud in some situations and little in others. To fix this would have to make the code spread (in various methods) needlessly, it seems to me that everything that is there is one thing. I'd shut the fuck up on this software. Must have a way to do this with configuration and / or annotation.

The problem is not a quantity of if but of decisions to be all, it does not have 34 if , but it has 34 comparisons that generate Booleans. If you want to reduce to only two if s é tranquilo porque todos geram o mesmo resultado, pra falar a verdade precisa de um if's (in C # it could be done with zero).

See if you can not see 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 placed GitHub for future reference .

I may have eaten some ball, so do not copy and paste.

If you do not want to shut down this parser you will have to do what it says and create each condition group in different methods and then create methods to paste it together, making it more confusing and less efficient. Ridiculous, but each one does what he thinks best.

If you go down this path you would go through the groups you have already created, perhaps regrouping a bit (you have started well), make sure it is null and if it is equal together, which makes more sense even in the case to keep everything in one method, will probably be more efficient and more readable.

In C # I would do it in a much simpler way than this, but I know Java likes verbose code and less robust, I do not know if I could do much better than this in C # with the right architecture. > Having said all this, it is possible that it really might be interesting to be in separate methods that control each of the fields individually, because they alone should be treated that way, or all software engineering needs to be thought to deal with it, mainly if each condition can be used again somewhere else, after all we must follow the DRY , that is, to make good and old abstraction, which even has to do with orientation to the object that they defend so much. But I do not have enough information to know if this is the most appropriate in this case. Surely it would be more testable, which I question if it should be the main purpose of the code, but if it is to change it must be first to meet the demand of the problem, only then think of the tste, and not think about the misconfigured SonarLint. / p>

I wonder if you have any major issues with the whole concept.

    
13.08.2018 / 19:16