False Positive S2583 Conditionally executed code should be reachable

[Fred_Robinson]

SonarQube 8.5 analyzing Java code with the SonarWay quality gate

The following code produces a false positive:

    public String findProspect() {
        String retVal = null;
        int attempts = 0;
        try {
            this.lastName = this.lastName.trim();
            final String fullOfferCode = getFullOfferCode(MediaType.DIRECT_MAIL);
            // get person from marketing database
            createPerson(fullOfferCode, MediaType.DIRECT_MAIL);
            // get campaign from marketing database
            createCampaign(fullOfferCode);
            retVal = "/term/quote.jsp?faces-redirect=true";
        } catch (final MalformedDataException ex) {
            logger.log(Level.INFO, "MalformedDataException: {0}", new Object[] {ex.getMessage()});
            attempts++;
        } catch (SystemUnavailableException | KBMSystemUnavailableException ex) {
            logger.log(Level.SEVERE, SYSTEM_UNAVAILABLE_EXCEPTION_0, new Object[] {ex.getMessage()});
            retVal = "/term/system-error.jsp?faces-redirect=true";
        } catch (final PersonNotFoundException ex) {
            logger.log(Level.INFO, ex.getMessage());
            attempts++;
        }
        // False Positive right here
        if (attempts > 0) {
            if (attempts < MAX_NUMBER_OF_ATTEMPTS) {
                final FacesMessage msg = new FacesMessage("Member Last Name or Personal Code not found. Please try again.");
                FacesContext.getCurrentInstance()
                        .addMessage("Member Last Name or Personal Code not found. Please try again.", msg);
            } else {
                retVal = "/term/access-error.jsp?faces-redirect=true";
            }
        }
        return retVal;
    }

(Yeah, it’s old crappy code that’s probably not used anymore.)

SQ does not seem to notice that attempts could be incremented in one or the other of the catch blocks. The exceptions could all be thrown by createPerson() and createCampaign().

Fred Robinson

[Margarita_Nedzelska]

Hi Fred!

Welcome to the community and thanks for your feedback.

Regarding your case, could you please clarify, which condition is reported as always evaluated to true.

attempts > 0
or
attempts < MAX_NUMBER_OF_ATTEMPTS

I tried your example, and I got that the second condition has an issue. But If you look at the code provided attentively, there is no way that the attempts variable will be greater than 1. Once you caught an exception you will never increment this local variable.

At the same moment, the first check doesn’t have any issue, because exception can be thrown and attempts variable can be greater than 0.

If you observe different behaviour on your side, please do not hesitate to come back here with more detailed explanation.

Kind regards,
Margarita

[Fred_Robinson]

Here is a screenshot of the report form SonarQube.

I read this as SonarQube saying that attempts will never be greater than zero.

I agree that the second condition will never be false, the way the method is written. It was supposed to permit up to 5 attempts to find a prospect, but setting attempts to zero every time renders this moot. attempts was originally a field of the object, but was only used in findProspect() (including setting it to zero), so SonarQube complained about that. On top of that, I can’t find any references to findProspect() in the code base (Java, JSP, JSF), leading me to think that this method is obsolete.

I’m more concerned by the SQ report.

Fred Robinson