-2
\$\begingroup\$

I'm trying to reduce the complexity of this piece of code following a SonarQube error. I have several similar blocks, and even if I try to extract them into private methods, the Sonar error persists.

There is the code :

if (contratPPH.getPieceIdentite() != null && fichePersonne.getPieceIdentite() != null) {
        if (contratPPH.getPieceIdentite().getAutoriteDelivrance() != null && fichePersonne.getPieceIdentite().getAutoriteDelivrance() != null) {
            if (!(fichePersonne.getPieceIdentite().getAutoriteDelivrance().equals(contratPPH.getPieceIdentite().getAutoriteDelivrance()))) {
                contratPPH.getPieceIdentite().setAutoriteDelivrance(fichePersonne.getPieceIdentite().getAutoriteDelivrance());
            }
        } else if (fichePersonne.getPieceIdentite().getAutoriteDelivrance() != null) {
            contratPPH.getPieceIdentite().setAutoriteDelivrance(fichePersonne.getPieceIdentite().getAutoriteDelivrance());
        } else if (contratPPH.getPieceIdentite().getAutoriteDelivrance() != null) {
            pieceIdentite.setAutoriteDelivrance(contratPPH.getPieceIdentite().getAutoriteDelivrance());
                       }

    } else if (contratPPH.getPieceIdentite() == null && fichePersonne.getPieceIdentite() != null) {
        if (fichePersonne.getPieceIdentite().getAutoriteDelivrance() != null) {
            contratPPH.getPieceIdentite().setAutoriteDelivrance(fichePersonne.getPieceIdentite().getAutoriteDelivrance());
        }
    } else if (contratPPH.getPieceIdentite() != null && fichePersonne.getPieceIdentite() == null) {
        if (contratPPH.getPieceIdentite().getAutoriteDelivrance() != null) {
            pieceIdentite.setAutoriteDelivrance(contratPPH.getPieceIdentite().getAutoriteDelivrance());
        }
    }

If anyone has a lead I'm interested :)

\$\endgroup\$
6
  • 2
    \$\begingroup\$ Welcome to Code Review! To help reviewers give you better answers, we need to know what the code is intended to achieve. Please add sufficient context to your question to describe the purpose of the code. We want to know why much more than how. The more you tell us about what your code is for, the easier it will be for reviewers to help you. Also, edit the title to simply summarise the task, rather than your concerns about the code. \$\endgroup\$ Dec 19, 2023 at 10:21
  • \$\begingroup\$ @TobySpeight Your comment and (presumably) close vote don't match up. Please read our meta to understand the difference between NDOC (was UWYA) and MRC (was LCC). \$\endgroup\$
    – Peilonrayz
    Dec 19, 2023 at 10:28
  • \$\begingroup\$ @Peilonrayz, Am I only allowed to comment on the close vote? That comes with its own comment if others agree. \$\endgroup\$ Dec 19, 2023 at 16:19
  • \$\begingroup\$ @TobySpeight I don't understand what you are asking. \$\endgroup\$
    – Peilonrayz
    Dec 19, 2023 at 22:53
  • \$\begingroup\$ @Peilonrayz - and I don't understand why you think that comments need to "agree" with close votes. The question is off-topic because it's missing important definitions (e.g. contratPPH, fichePersonne); if that's fixed it still needs a decent explanation and title. \$\endgroup\$ Dec 20, 2023 at 9:32

2 Answers 2

1
\$\begingroup\$

First lets simplify the names and function calls to make reading the boolean logic of the code easier.

if (A != null && B != null) {
    if (A.fn() != null && B.fn() != null) {
        if (A.fn() != B.fn()) {
            #1
        }
    } else if (B.fn() != null) {
        #1
    } else if (A.fn() != null) {
        #2
    }

} else if (!(A != null) && B != null) {
    if (B.fn() != null) {
        #1
    }
} else if (A != null && !(B != null)) {
    if (A.fn() != null) {
        #2
    }
}

Lets express the logic as a truth table:

A != null A.fn() != null B != null B.fn() != null Choice
True True True True #1
True False True True #1
True True True False #2
False False True True #1
True True False False #2

You can probably notice the only columns in the table which change the outcome are A.fn() != null and B.fn() != null. As such we can just merge A != null and A.fn() != null together, likewise for B, and rewrite the table.

A B Choice
True True #1
False True #1
True False #2
False False N/A

Note: The table does hide the if (A != B) check in the code.

boolean A = A != null && A.fn() != null;
boolean B = B != null && B.fn() != null;

if (A && B) {
    if (A.fn() != B.fn()) {
        #1
    }
} else if (B) {
    #1
} else if (A) {
    #2
}

The first if look a little suspect to me. I'd expect #1 and #2 or an else if to add the #2. However I don't speak French so you may be updating a value. I just want to point out a possible mistake.

Next the quirk in the code makes DRYing the duplicate #1s a little more complex. We can roll the first three ifs into a turnery A && B ? A != B : B. However then the else if won't know !B. So we can go two routes:

Separate ifs, or start with an if (B).

if (A && B ? A.fn() != B.fn() : B) {
    #1
}
if (A && !B) {
    #2
}
if (B) {
    if (A ? A.fn() != B.fn() : true) { // or (!A || A.fn() != B.fn())
        #1
    }
} else if (A) {
    #2
}

Converting the latter back to Java we get:

boolean A = contratPPH.getPieceIdentite() != null && contratPPH.getPieceIdentite().getAutoriteDelivrance() != null;
boolean B = fichePersonne.getPieceIdentite() != null && fichePersonne.getPieceIdentite().getAutoriteDelivrance() != null;

if (B) {
    if (!A || !contratPPH.getPieceIdentite().getAutoriteDelivrance().equals(fichePersonne.getPieceIdentite().getAutoriteDelivrance())) {
        contratPPH.getPieceIdentite().setAutoriteDelivrance(fichePersonne.getPieceIdentite().getAutoriteDelivrance());
    }
} else if (A) {
    pieceIdentite.setAutoriteDelivrance(contratPPH.getPieceIdentite().getAutoriteDelivrance());
}

We can further simplify the code by using the null conditional operator ?. and comparing to null later.

A = contratPPH.getPieceIdentite()?.getAutoriteDelivrance();
B = fichePersonne.getPieceIdentite()?.getAutoriteDelivrance();

if (B != null) {
    if (A == null || !A.equals(B)) {
        contratPPH.getPieceIdentite().setAutoriteDelivrance(B);
    }
} else if (A != null) {
    pieceIdentite.setAutoriteDelivrance(A);
}
\$\endgroup\$
2
  • \$\begingroup\$ so A = contratPPH.getPieceIdentite() != null, B = fichePersonne.getPieceIdentite() != null, A.fn() = contratPPH.getPieceIdentite().getAutoriteDelivrance() != null and B.fn() = fichePersonne.getPieceIdentite().getAutoriteDelivrance() != null. Then A = A ? A : A.fn(); <=> Boolean A = contratPPH.getPieceIdentite() != null ? contratPPH.getPieceIdentite() != null : contratPPH.getPieceIdentite().getAutoriteDelivrance() != null. Is that right ? \$\endgroup\$
    – goujon
    Dec 20, 2023 at 9:30
  • \$\begingroup\$ @goujon I made a mistake, I have updated my answer. A != B wasn't correct and I didn't allow the syntax to support the correct meaning. I have included the revised code at the bottom. Please note A probably should be named something else, but I don't know French. \$\endgroup\$
    – Peilonrayz
    Dec 20, 2023 at 11:36
0
\$\begingroup\$

In view of the previous discussion with @Peilonrayz, I have a solution to propose. If I understood correctly we have 2 solutions :

A = A ? A : A.fn();
B = B ? B : B.fn();
if (A && B ? A != B : B) {
    #1
}
if (A && !B) {
    #2
}

or

A = A ? A : A.fn();
B = B ? B : B.fn();
if (B) {
   if (A ? A != B : true) { // or (!A || A != B)
      #1
   }
} else if (A) {
   #2
}

on choisissant la première solution, j'obtiens :

boolean A = contratPPH.getPieceIdentite() != null ? contratPPH.getPieceIdentite() != null : contratPPH.getPieceIdentite().getType() != null;
boolean B = fichePersonne.getPieceIdentite() != null ? fichePersonne.getPieceIdentite() != null : fichePersonne.getPieceIdentite().getType() != null;

    if(A && B ? A != B : B) {
        contratPPH.getPieceIdentite().setType(fichePersonne.getPieceIdentite().getType());
    }
    if(A && !B) {
        pieceIdentite.setType(contratPPH.getPieceIdentite().getType());
    }

@Peilonrayz could you confirm that I'm on the right way ?

\$\endgroup\$
4
  • 1
    \$\begingroup\$ The definition of A and B look correct. The replacement for #1 and #2 look really incorrect. I don't know how contratPPH.getPieceIdentite().setAutoriteDelivrance(fichePersonne.getPieceIdentite().getAutoriteDelivrance()); became contratPPH.getPieceIdentite().setType(fichePersonne.getPieceIdentite().getType()); as the code never used setType and getType. \$\endgroup\$
    – Peilonrayz
    Dec 20, 2023 at 11:09
  • \$\begingroup\$ Really sorry, it's the same problem for another field \$\endgroup\$
    – goujon
    Dec 20, 2023 at 11:11
  • \$\begingroup\$ On a side note answers must contain one Insightful Observation. Please can you point to one issue with my code (a what) and show an improvement (a why) to bring your answer inline with site scope. If you don't please delete your answer (as you've got a response from me anyway). I also noticed A ? A : A.fn() could just be A && A.fn() (or better yet the null conditional operator A?.fn()) so the Insightful Observation should be easy to make. \$\endgroup\$
    – Peilonrayz
    Dec 20, 2023 at 11:11
  • \$\begingroup\$ in if(A && B ? A != B : B), A != B is always false so as you said, I have to use !A.equals(B) but the reference is not known \$\endgroup\$
    – goujon
    Dec 20, 2023 at 11:13

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