10

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) { 
    }
    
  • 2
    What are "found-establish" and "needed-establish" ? – Tulains Córdova Jun 21 '17 at 18:22
  • 4
    That's supposed to be an em dash, read like "valid user found: establish their session". – BJ Myers Jun 21 '17 at 18:24
  • 2
    @A_B Which of those return values are successful logins are which are failed logins. Not all are self-evident. – Tulains Córdova Jun 21 '17 at 18:27
  • @A_B Does "establish their session" mean "session stablished" or "needs stablishing a session" ? – Tulains Córdova Jun 21 '17 at 18:33
  • @TulainsCórdova: "Establish" means as much as "create" (in this context at least) - so "establish their session" is roughly equal to "create their session" – hoffmale Jun 23 '17 at 17:48
22

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
  • you can also build a "complex" enum (enum with attributes) to embed the level of error in the Enum. However be careful because if you use somme serialization tools, that might not pass very well. – Walfrat Jun 23 '17 at 14:23
  • Throwing up exceptions in the error cases and saving the enums for success only is also an option. – T. Sar - Reinstate Monica Jun 23 '17 at 16:14
  • @T.Sar Well, as I understood they are not errors per se but denials to create a session for some reason. I will edit the answer. – Tulains Córdova Jun 23 '17 at 16:39
  • @T.Sar I changed the values to PASS, WAIVER and DENIAL to make it clear thar what I previously called ERROR is a valid status. Maybe now I should come up with a better name for Severity – Tulains Córdova Jun 23 '17 at 16:59
  • I was thinking of something else with my suggestion, but I really liked your suggestion! I'm throwing up a +1, for sure! – T. Sar - Reinstate Monica Jun 23 '17 at 17:03
15

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.

7

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.

  • That's not what is usually meant with 'magic numbers'. – D Drmmr Jun 23 '17 at 12:45
  • 2
    It will show up as magic numbers at call sites, as in if (processLogin(..) == 3) – Daenyth Jun 23 '17 at 13:18
  • @DDrmmr - This is exactly what is meant by the 'magic numbers' code smell. This function signature almost certainly implies that processLogin() contains lines like "return 8;" in its implementation, and it pretty much forces code using processLogin() to look something like this "if (resultFromProcessLogin == 7) {". – Stephen C. Steel Jun 23 '17 at 13:21
  • 3
    @Stephen The actual value of the numbers is irrelevant here. They are just IDs. The term magic numbers is usually used for values in code that have a meaning, but who's meaning is undocumented (e.g. in a variable name). Replacing the values here with named integer variables will not fix the issue. – D Drmmr Jun 24 '17 at 5:09
2

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.