CODEWITHSUNDEEP
javaperformanceif-statement
Hanamiya
Hanamiya

Reputation: 43

Java nested if else? Other approaches?

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

Upvotes: 1

Views: 179

Answers (2)

davidxxx
davidxxx

Reputation: 131326

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

Upvotes: 2

Dave
Dave

Reputation: 1874

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

Upvotes: 3

Related Questions