complexity reduction (refacto) [closed]

Question

Closed. This question is off-topic. It is not currently accepting answers.

Missing Review Context: Code Review requires concrete code from a project, with enough code and / or context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site.

Closed 1 year ago.

Improve this question

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 :)

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. — Toby Speight
– Toby Speight, Commented Dec 19, 2023 at 10:21
@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). — Peilonrayz
– Peilonrayz ♦, Commented Dec 19, 2023 at 10:28
@Peilonrayz, Am I only allowed to comment on the close vote? That comes with its own comment if others agree. — Toby Speight
– Toby Speight, Commented Dec 19, 2023 at 16:19
@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. — Toby Speight
– Toby Speight, Commented Dec 20, 2023 at 9:32

Peilonrayz · Accepted Answer · 2023-12-20 11:34:57Z

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

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 ? — goujon
– goujon, Commented Dec 20, 2023 at 9:30
@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. — Peilonrayz
– Peilonrayz ♦, Commented Dec 20, 2023 at 11:36

goujon · Accepted Answer · 2023-12-20 11:00:57Z

0

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 ?

answered Dec 20, 2023 at 11:00

goujon

11

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
– Peilonrayz ♦

2023-12-20 11:09:11 +00:00
Commented Dec 20, 2023 at 11:09
\$\begingroup\$ Really sorry, it's the same problem for another field \$\endgroup\$

goujon
– goujon

2023-12-20 11:11:07 +00:00
Commented 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
– Peilonrayz ♦

2023-12-20 11:11:39 +00:00
Commented 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
– goujon

2023-12-20 11:13:24 +00:00
Commented Dec 20, 2023 at 11:13

Add a comment |

Stack Exchange Network

complexity reduction (refacto) [closed]

2 Answers 2

Hot Network Questions

complexity reduction (refacto) [closed]

2 Answers 2

Related

Hot Network Questions