Java verschachtelt, wenn sonst? Andere Ansätze?

Question

Gibt es einen anderen Ansatz, um solche verschachtelten if else-Anweisungen zu behandeln? Ich habe überall gelesen, dass zu viele, wenn noch andere Aussagen schlecht sind. Ist dies der richtige Weg, um solche Fälle zu behandeln, oder muss es umgestaltet werden?

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

Answer 1

Manchmal, wenn es um Geschäftslogik geht, werden Sie Strukturen wie diese haben. Sie können es etwas aufräumen, indem Sie eine switch-Anweisung machen, aber es wird in diesem Fall nicht viel helfen.

Anstatt zu versuchen, dies zu lösen, würde ich definitiv versuchen, diesen riesigen Brocken neu zu gestalten, um nicht zwischen so vielen Schichten der Abstraktion zu springen.

Nehmen wir als Beispiel dieser Block hier:

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

Am Anfang des Blocks, wir arbeiten an der "user level abstraction layer", zu fragen, ob der Benutzer gesperrt ist. Am Ende sind wir auch auf der gleichen Abstraktionsebene und entsperren den Benutzer. Aber dazwischen gehen wir hin, um ein paar Ints zu parsen, einige Datumsarithmetik durchzuführen und andere ungöttliche Sachen zu machen, wie das Ziehen von Daten aus einem messagesource.

Ziehen Sie diese Dinge heraus. Vergleichen Sie den oberen Code-Block, wie diese lautet:

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

oder bleiben gleich Ihren Code (dank tobias_k es für den Hinweis auf)

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! Ich kann wirklich sagen, was von Anfang an passiert. Sie können dies auf das ganze Chaos anwenden.

Eine andere Sache, die die Lesbarkeit des Blocks erhöhen würde, erwähnte ich: Ziehen Sie in Betracht, sowohl eine loginHistoryService.lockUserAccount() und eine loginHistoryService.unlockUserAccount() Methode. Das Übergeben von booleschen Flags ist unlesbar, hässlich und anfällig für Bugs. Also, am Ende, würde der Block

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

oder

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

Answer 2

Sie Code zu lesen ist nicht kompliziert, aber es ist ein wenig kompliziert zu lesen. Bei Ihnen würde ich an meinem Java-Code-Formatierung ein wenig arbeiten. Dann würde ich jede große sub-bedingte Block-Code in einer privaten Methode verschieben. Viele eingebettete "if" können schwer zu lesen und zu warten sein. Es kann sinnvoll sein, sie in private Methoden mit einem relevanten Namen aufzuteilen.

In Ihrem Fall finde ich, dass es ist. Es könnte so aussehen:

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