Java nested if else? Other approaches?

Question

Is there any other approach to handle such nested if else statements? I've read everywhere that too many if else statements are bad. Is this the correct way to handle such cases or is there need to refactor?

User user = loginHistoryService.findByUsername(loginRequest.getUsername());
    boolean isPasswordValid = bcryptPasswordEncoder.matches(loginRequest.getPassword(), user.getPassword());
    if (user.isDeleted()) {
        throw new AccountDeletedAuthException("Account is Deleted");
    } else if (user.isLocked()) {
        if (DateUtil
                .addHours(user.getLockedTime(), Integer.parseInt(
                        messageSource.getMessage(Constants.UNLOCK_ACCOUNT_AFTER_XX_HOURS, null, "24", null)))
                .compareTo(DateUtil.getCurrentDateTime()) < 0 && isPasswordValid) {
            logger.info("|*|*| Unlocking account. Account Lock Timer Over.. |*|*|*|");
            loginHistoryService.lockUserAccount(user.getUserId(), false);
        } else {
            throw new LockedException("Account is Locked");
        }
    } else if (user.getUserStatusId() == UserStatus.INACTIVE.getStatusCode()) {
        throw new InactiveAccountAuthException("Account is Inactive");
    } else if (user.getUserStatusId() == UserStatus.PENDING_VERIFICATION.getStatusCode()) {
        throw new DisabledException("".trim() + user.getUserId());
    } else if (!isPasswordValid) {

        List<Integer> loginAttemptStatuses = loginHistoryService.getLastThreeLoginAttempts(user.getUserId());
        loginHistoryService.createLoginHistoryEntry(user.getUserId(), LoginStatus.FAILURE.getStatusCode());
        int consecutiveFailedAttempts = 0;
        for (int tempIndex = 0; tempIndex < loginAttemptStatuses.size(); tempIndex++) {
            if (loginAttemptStatuses.get(tempIndex).intValue() == LoginStatus.FAILURE.getStatusCode()) {
                consecutiveFailedAttempts++;
            } else {
                break;
            }
        }
        if (consecutiveFailedAttempts == 3) {
            loginHistoryService.lockUserAccount(user.getUserId(), true);
        }
        throw new InvalidPasswordAuthException("".trim() + consecutiveFailedAttempts);
    }

if nothing follows after this, you could substitute if (a) { b } else { c } with if (a) { b; return } c — Coderino Javarino
– Coderino Javarino, Commented Jul 15, 2016 at 11:40
IMHO you could just optimize your code a bit for readability but as the others stated, all those conditions make sense and you have to check for them — Supahupe
– Supahupe, Commented Jul 15, 2016 at 11:45
In fact, you don't need most of the elses. You seem to throw Exceptions in many paths. So if they apply, the code after will not be executed. BTW: In case of successful unlocking of account, the rest will not be checked. — Fildor
– Fildor, Commented Jul 15, 2016 at 11:47
I looked again and if I see correctly, in case of successful unlock, you do check password vailidity. And that's the only path without exception being thrown. So my suggestion would be: get rid of else. — Fildor
– Fildor, Commented Jul 15, 2016 at 11:56

Dave · Accepted Answer · 2016-07-15 12:18:46Z

3

Sometimes, when dealing with business logic, you are going to have structures like this. You can clean it up a little by making a switch statement, but it's not going to help much in this case.

Instead of trying to resolve this, I would definitely try to refactor this huge chunk to not jump between so many layers of abstraction.

Take as an example this block here:

else if (user.isLocked()) {
    if (DateUtil
            .addHours(user.getLockedTime(), Integer.parseInt(
                    messageSource.getMessage(Constants.UNLOCK_ACCOUNT_AFTER_XX_HOURS, null, "24", null)))
            .compareTo(DateUtil.getCurrentDateTime()) < 0 && isPasswordValid) {
        logger.info("|*|*| Unlocking account. Account Lock Timer Over.. |*|*|*|");
        loginHistoryService.lockUserAccount(user.getUserId(), false);
}

In the beginning of the block, we are working on the "user level abstraction layer", asking if the user is locked. In the end, we are also in the same abstraction level, unlocking the user. But in between, we go down to parse some ints, perform some date arithmetic and do other ungodly stuff like pulling data out of a messagesource.

Consider pulling these things out. Compare the upper block of code to how this reads:

else if(user.isLocked() && canUnlockUser(user)) {
    logger.info("|*|*| Unlocking account. Account Lock Timer Over.. |*|*|*|");
    loginHistoryService.lockUserAccount(user.getUserId(), false);
}

or to remain equivalent to your code (thanks to tobias_k for pointing it out)

else if(user.isLocked()) {
    if(canUnlockUser(user)){
        logger.info("|*|*| Unlocking account. Account Lock Timer Over.. |*|*|*|");
        loginHistoryService.lockUserAccount(user.getUserId(), false);
    } else {
        throw new LockedException("Account is Locked");
    }       
}

Nice! I can actually tell what is going on from the first look. You can apply this to the whole mess.

Another thing that would increase the readability of the block I mentioned: Consider making both a loginHistoryService.lockUserAccount() and a loginHistoryService.unlockUserAccount() method. Passing boolean flags like this is unreadable, ugly and prone to introducing bugs. So, in the end, the block would read

else if(user.isLocked() && canUnlockUser(user)) {
    logger.info("|*|*| Unlocking account. Account Lock Timer Over.. |*|*|*|");
    loginHistoryService.unlockUserAccount(user.getUserId());
}

or

else if(user.isLocked()) {
    if(canUnlockUser(user)){
        logger.info("|*|*| Unlocking account. Account Lock Timer Over.. |*|*|*|");
        loginHistoryService.lockUserAccount(user.getUserId(), false);
    } else {
        throw new LockedException("Account is Locked");
    }       
}

edited Jul 15, 2016 at 12:18

answered Jul 15, 2016 at 12:02

Dave

1,8743 gold badges25 silver badges36 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

tobias_k Over a year ago

Note that your if condition is not entirely equivalent. In OP's code, if user.isLocked() all the other conditions will not be evaluated. Also, where would you raise the "Locked"-Exception?

Dave Over a year ago

Thanks, @tobias_k, that's right. While I wasn't specifically aiming to provide a semantically identical version, I will update the post to reflect that.

Fildor Over a year ago

@tobias_k "In OP's code, if user.isLocked() all the other conditions will not be evaluated." - I don't know but that seems to be a bug to me. And as I already pointed out: With all the exceptions being thrown, he could as well get rid of the else's and just queue the ifs. Would change that specific semantic of skipping checks on unlock - but as I said: I consider this a bug.

Dave Over a year ago

@Hanamiya nothing, per se. It's just a bit overkill for what it is used here, in my opinion.

davidxxx · Accepted Answer · 2016-07-15 12:42:40Z

You code is not complicated but it is a bit complicated to read. At your place, I would work on my java code formatting a little bit. Then, I would move each large sub-conditional block code in a private method. Many embedded "if" may become hard to read and to maintain. It may be convenient to split them in private methods with a relevant name.

In your case, I find that it is. It could look like that :

User user = loginHistoryService.findByUsername(loginRequest.getUsername()); 
boolean isPasswordValid = bcryptPasswordEncoder.matches(loginRequest.getPassword(), user.getPassword());

if (user.isDeleted()) {
    throw new AccountDeletedAuthException("Account is Deleted");
}
else if (user.isLocked()) {
    handleWhenUserLocked(user);
}
else if (user.getUserStatusId() == UserStatus.INACTIVE.getStatusCode()) {
    throw new InactiveAccountAuthException("Account is Inactive");
}
else if (user.getUserStatusId() == UserStatus.PENDING_VERIFICATION.getStatusCode()) {
    throw new DisabledException("".trim() + user.getUserId());
}
else if (!isPasswordValid) {
    handleWhenPasswordNotValid(user);
}

Collectives™ on Stack Overflow

Java nested if else? Other approaches?

2 Answers 2

4 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

4 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related