I see the following concerns here:
- Check for banned IP. Checks whether user's IP is in a banned IP range. You will execute this no matter what.
- Check if trials exceeded. Checks whether user have exceeded their login attempts. You will execute this no matter what.
- Authenticate user. Attempt to authenticate user.
Pseudo code:
CheckBannedIP(login.IP);
CheckLoginTrial(login);
Authenticate(login.Username, login.Password);
CheckBannedIP(string ip)
{
// If banned then re-direct, else do nothing.
}
CheckLoginTrial(LoginAttempt login)
{
// If exceeded trials, then inform user, else do nothing
}
Currently your example has too many responsibilities. All I did, was encapsulate those responsibilities within methods. Code looks cleaner and you don't have condition statements all over the place.
Factory encapsulates construction of objects. You don't need to encapsulate construction of anything in your example, all you need is to separate your concerns.