ConfusingConditionCheck problem with sequential IF #153

romani opened this Issue Oct 13, 2013 · 4 comments


None yet

3 participants

romani commented Oct 13, 2013

I have IgnoreSequentialIf = true, look at code below.
I know that we previously decided to check latest IF in sequence, but after testing of more wide range of sources, we found it is inconvenient.
Please update Checks to not raise error on that case.

        if (isUserExists(email)) {
            model.addAttribute("error", "User with given email already exists");
        } else if (!agreementAccepted) {
            model.addAttribute("error", "You have to accept user agreement in order to register");
        } else if (!nullSafeEquals(password, rpassword)) {  ///WARNING IS HERE !!!!
            model.addAttribute("error", "Password and repeated password do not match");
        } else {
            // very long body of processing

another example:

        if (themesUser == null || !AuthInfo.hasRoles(themesUser, ROLE_USER_MUST_CHANGE_PASSWORD)) {
  "User does not have grants to access this page");
            redirectUrl = "redirect:/login";
        } else if (StringHelper.isNullOrEmpty(password.trim())) {
            redirectUrl = "redirect:/change-password?error=passIsEmpty";
        } else if (!nullSafeEquals(password, rpassword)) { // WARNING HERE
            redirectUrl = "redirect:/change-password?error=passNotMatched";
        } else {
            ///// long code
        return redirectUrl;

Let me explain the problem.
See example 1. It can be fixed by:

  1. creating an additional 'notNullSafeEquals' method;
  2. by reordering the code blocks.
    Both these variants are not applicable, as:
  • for case 1 new method will still have a bad name and the new method still will contain negation, but in it`s name isted of '!' sign
  • for case 2, if we will do reordering of code blocks, code (sure) will still work as exctected, but.. After such refactoring code becomes less readable as most people searches for main logic (and writes the main logic) inside the latest 'else' statement.

Let me explain further if you still have any questions

romani commented Oct 29, 2013

build tested - works fine.
I am ok with merge. Any other Admin is fine with mere ?

romani commented Nov 6, 2013

merged #143.

@romani romani closed this Nov 6, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment