Reworking a function returning an integer code that represents many different statuses

Question

I've inherited some awful code that I've included a short sample of below.

• Is there a name for this particular anti-pattern?

• What are some recommendations for refactoring this?

  // 0=Need to log in / present username and password
  // 2=Already logged in
  // 3=Inactive User found
  // 4=Valid User found-establish their session
  // 5=Valid User found with password change needed-establish their session
  // 6=Invalid User based on app login
  // 7=Invalid User based on network login
  // 8=User is from an non-approved remote address
  // 9=User account is locked
  // 10=Next failed login, the user account will be locked
  
  public int processLogin(HttpServletRequest request, HttpServletResponse response, 
                          int pwChangeDays, ServletContext ServContext) { 
  }
  

Answer 1

The code is bad not only because the magic numbers, but because it coalesces several meanings in the return code, hiding inside of its meaning an error, a warning, a permission to create a session or a combination of the three, which makes it a bad input for decision making.

I would suggest the following refactoring: returning an enum with the possible results (as suggested in other answers), but adding to the enum an attribute indicating whether it is a denial, a waiver (I'll let you pass this last time) or if it is OK (PASS):

public LoginResult processLogin(HttpServletRequest request, HttpServletResponse response, 
                            int pwChangeDays, ServletContext ServContext) { 
    }

==> LoginResult.java <==

public enum LoginResult {
    NOT_LOGGED_IN(Severity.DENIAL),
    ALREADY_LOGGED_IN(Severity.PASS),
    INACTIVE_USER(Severity.DENIAL),
    VALID_USER(Severity.PASS),
    NEEDS_PASSWORD_CHANGE(Severity.WAIVER),
    INVALID_APP_USER(Severity.DENIAL),
    INVALID_NETWORK_USER(Severity.DENIAL),
    NON_APPROVED_ADDRESS(Severity.DENIAL),
    ACCOUNT_LOCKED(Severity.DENIAL),
    ACCOUNT_WILL_BE_LOCKED(Severity.WAIVER);

    private Severity severity;

    private LoginResult(Severity severity) {
        this.severity = severity;
    }

    public Severity getSeverity() {
        return this.severity;
    }
}

==> Severity.java <==

public enum Severity {
    PASS,
    WAIVER,
    DENIAL;
}

==> Test.java <==

public class Test {

    public static void main(String[] args) {
        for (LoginResult r: LoginResult.values()){
            System.out.println(r + " " +r.getSeverity());           
        }
    }
}

Output for Test.java showing the severity for each LoginResult:

NOT_LOGGED_IN : DENIAL
ALREADY_LOGGED_IN : PASS
INACTIVE_USER : DENIAL
VALID_USER : PASS
NEEDS_PASSWORD_CHANGE : WAIVER
INVALID_APP_USER : DENIAL
INVALID_NETWORK_USER : DENIAL
NON_APPROVED_ADDRESS : DENIAL
ACCOUNT_LOCKED : DENIAL
ACCOUNT_WILL_BE_LOCKED : WAIVER

Based on both the enum value and its severity, you can decide whether creation of session proceeds or not.

EDIT:

As a response to @T.Sar's comment, I changed the severity's possible values to PASS,WAIVER and DENIAL instead of (OK,WARNING and ERROR). That way it is clear that a DENIAL (previously ERROR) is not an error per se and shouldn't necessarily translate into throwing an exception. The caller examines the object and decides whether or not to throw an exception, but DENIAL is a valid result status resulting from calling processLogin(...).

• PASS: go ahead, create a session if one doesn't already exist
• WAIVER: go ahead this time, but next time user you may not be allowed to pass
• DENIAL: sorry, user cannot pass, don't create a session

Answer 2

This is an example of Primitive Obsession - using primitive types for "simple" tasks that eventually become not so simple.

This may have started out as code that returned a bool to indicate success or failure, then turned into an int when there was a third state, and eventually became a whole list of nigh-undocumented error conditions.

The typical refactoring for this problem is to create a new class/struct/enum/object/whatever that can better represent the value in question. In this case, you might consider switching to an enum that contains the result conditions, or even a class that could contain a bool for success or failure, an error message, additional information, etc.

For more refactoring patterns that might be useful, have a look at Industrial Logic's Smells to Refactorings Cheatsheet.

Answer 3

I'd call that a case of "magic numbers" - numbers that are special and have no obvious meaning by themselves.

The refactoring I would apply here is to restructure the return type to an enum, since it encapsulates the domain concern in a type. Dealing with the compile errors falling out from that should be possible piecemeal, since java enums can be ordered and numbered. Even if not, it shouldn't be hard to deal with them directly instead of falling back to ints.

Answer 4

This is a particularly unpleasant bit of code. The antipattern is known as "magic return codes". You can find a discussion here.

Many of the return values indicate error states. There's a valid debate on whether to use error handling for flow control, but in your case, I think there are 3 cases: success (code 4), success but need to change password (code 5), and "not allowed". So, if you don't care about using exceptions for flow control, you could use exceptions to indicate those states.

Another approach would be to refactor the design so you return a "user" object, with a "profile" and "session" attribute for a successful login, a "must_change_password" attribute if necessary, and a bunch of attributes to indicate why the log-in failed if that was the flow.