Solution for Magic Number issue......?

Question

Consider the following code segment...

 public static UserStatus getEnum(int code) {
    switch (code) {
        case 0:
            return PENDING;
        case 1:
            return ACTIVE;
        case 2:
            return SUSPENDED;
        case 3:
            return DELETED;
        case 4:
            return LOGIN_DISABLED;
        default:
            return null;
        }

}

Now number 3 and 4 in cases(case 3 and case 4) are detected as magic numbers by SONAR.

To avoid that issue I changed my code segment as follows...

 public static UserStatus getEnum(int code) {        
    final int Pending=0;
    final int Active=1;
    final int Suspended=2;
    final int Deleted= 3;
    final int Login_details=4;

    switch (code) {
        case Pending:
            return PENDING;
        case Active:
            return ACTIVE;
        case Suspended:
            return SUSPENDED;
        case Deleted:
            return DELETED;
        case Login_details:
            return LOGIN_DISABLED;
        default:
            return null;
    }
}

Is this a good way to solve the magic number issue in this kind of scenario ?.

Solution 1

I gather that you want to avoid using integer literals in the code. Your solution is not particularly effective because it simply moves the literals to the top of the method. It gains a little bit because it gives meaningful names to the constants, but these names are private to the method.

A better approach would be to define the numbers as fields in an interface. You can then statically import the fields and use them as symbolic names for the constants.

If the enum is declared in the same order as the constants:

enum UserStatus {PENDING, ACTIVE, SUSPENDED, DELETED, LOGIN_DISABLED}

you can do another trick:

public static UserStatus getEnum(int code) {
    UserStatus[] values = UserStatus.values();
    return (code >= 0 && code < values.length) ? values[code] : null;
}

However, this creates a linkage between the constant values and the declaration of the enum. This may be okay, depending on where the actual parameter values are generated in calls to getEnum.

Solution 2

The problem is straight-forward and obvious: when people is reading your code, it is not obvious why 1 will give PENDING. What is the meaning of 1?

You should give semantic meaning to that. Using constants is what normally should be done:

(Assume the getEnum() is part of the UserService, the code should look something like)

public interface UserService {
   public static final int USER_STATUS_VAL_PENDING = 1;
   public static final int USER_STATUS_VAL_ACTIVE = 2;

   UserStatus getUserStatus(int userStatusVal);
}

public class SimpleUserService implements UserService {
   public UserStatus getUserStatus(int userStatusVal) {
      switch userStatusVal {
      case USER_STATUS_VAL_PENDING:
         return UserStatus.PENDING;
      case USER_STATUS_VAL_ACTIVE:
         return UserStatus.ACTIVE;
      //.....
   }
}

As suggested by another answer, you may rely on the ordinal value of enum to do the int-enum mapping. However you have to be aware that, such way can lead to problem if you rearraged the values of enum, or added new values at position apart from the end. The ordinal values will be changed and you have no way to override that.

Another thing to pay attention is, there are something you are not done right in your code:

1. As the purpose is to give semantic meaning to the integer literal by making it a constant, it should be something visible to the caller, therefore local final variable is not the right way to do.
2. you should use ALL_CAP_UNDERSCORE_DELIMITED for constants name