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 ?.

What do you mean that 3 and 4 are "detected as magic numbers"? What's the "magic number issue"? — Ted Hopp, Mar 21 '13 at 04:24
I assume you're talking about `SONAR` detecting them as `Magic Numbers`? — Rahul, Mar 21 '13 at 04:24
yes your're right: check this other question for reference: http://stackoverflow.com/questions/47882/what-is-a-magic-number-and-why-is-it-bad — Stefano Crespi, Mar 21 '13 at 04:24
Better option would be to have a class, with `static` values, so that frequently used `Magic` numbers can be moved there, do that they can be used anywhere in the project. — Rahul, Mar 21 '13 at 04:26
This is regarding the SONAR. SONAR detect those as Magic Numbers — Ruchira Gayan Ranaweera, Mar 21 '13 at 04:27

score 7 · Answer 1 · answered Mar 21 '13 at 04:27

7

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.

answered Mar 21 '13 at 04:27

Ted Hopp

232,168
48
399
521

1

Interface constants is anti-pattern in Java. – jFrenetic Mar 21 '13 at 04:47
@jFrenetic - Yeah, these days it's considered better style to use a class to declare symbolic constants. Either way would be an improvement over OP's current code. – Ted Hopp Mar 21 '13 at 04:57
I think it is not about putting the constant in "class" or "interface". The anti-pattern is : creating an interface just for putting constants. Constants should goes to the class/interface/type that is suitable to contain it. If none then use a final un-instantiable class instead, to avoid giving incorrect semantic meaning to the enclosing type – Adrian Shum Mar 21 '13 at 06:28
@AdrianShum - The real anti-pattern is bringing the constants into a class's namespace by implementing an interface. For that reason, "class "vs. "interface" is an important distinction. (You extend, rather than implement, classes.) Using `import static` and declaring the constants in a (non-instantiable) class instead of an interface takes it out of the realm of an anti-pattern. (See, for instance, the Wikipedia article [_Constant interface_](http://en.wikipedia.org/wiki/Constant_interface).) – Ted Hopp Mar 21 '13 at 07:03
@TedHopp I think the real anti-pattern as described in your quoted page is "use of an interface **solely** to define constants, and having classes implement that interface in order to achieve convenient syntactic access to those constants". Class vs Interface is not really the main point. If constants are closely related to corresponding behaviors, it is in fact a good choice to put those behavior and constants in same interface. (note the points in your quoted page, about cohesion). Anyway, a bit OT for this discussion :P – Adrian Shum Mar 25 '13 at 01:48
@AdrianShum - The second sentence of the Wikipedia page offers the reason why this is an anti-pattern: _"However, since constants are very often merely an implementation detail, and the interfaces implemented by a class are part of its exported API, this practice amounts to putting implementations details into the API, which was considered inappropriate..."_ The issue is not using an interface to define constants, it's having a class _implement_ the interface that is the anti-pattern. That's why I suggested statically importing the constants from the interface. – Ted Hopp Dec 22 '13 at 16:36
1

@jFrenetic - The anti-pattern is **implementing** a constants interface, not declaring one and statically importing the constant names. – Ted Hopp Dec 22 '13 at 16:38

Adrian Shum · Answer 2 · 2013-03-25T01:44:12.243

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:

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.
you should use ALL_CAP_UNDERSCORE_DELIMITED for constants name

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

2 Answers2

Linked