Is it a good practice to avoid constants by using getters?

Question

Is it a good practice to replace constants used outside of classes by getters?

As an example, is it better to use if User.getRole().getCode() == Role.CODE_ADMIN or if User.getRole().isCodeAdmin()?

That would lead to this class:

class Role {
    constant CODE_ADMIN = "admin"
    constant CODE_USER = "user"

    private code

    getRoleCode() {
       return Role.code
    }

    isCodeAdmin () {
       return Role.code == Role.CODE_ADMIN
    }

    isCodeUser () {
       return Role.code == Role.CODE_USER
    }
}

Answer 1

First off, please note that doing something like entity.underlyingEntity.underlyingEntity.method() is considered a code smell according to the Law of Demeter. This way, you're exposing a lot of implementation details to the consumer. And each need of extension or modification of such a system will hurt a lot.

So given that, I'd recommend you to have a HasRole or IsAdmin method on the User as per CodesInChaos' comment. This way, the way how the roles are implemented on the user remains implementation detail for the consumer. And also it feels more natural to ask the user what his role is instead of asking him about details of his role and then deciding based on that.

---

Please also avoid using strings unless where necessary. name is a good example of string variable because the contents are unknown beforehand. On the other hand, something like role where you have two distinct values that are well known at compilation time, you'd better use strong typing. That's where enumeration type comes into play...

Compare

public bool HasRole(string role)

with

public enum Role { Admin, User }

public bool HasRole(Role role)

The second case gives me a lot more idea of what I should be passing around. It also prevents me from erroneously passing in an invalid string in case I had no idea about your role constants.

---

Next is the decision on how will the role look. You can either use enum directly stored on the user:

public enum Role
{
    Admin,
    User
}

public class User
{
    private Role _role;

    public bool HasRole(Role role)
    {
        return _role == role;
    }

    // or
    public bool IsAdmin()
    {
        return _role == Role.Admin;
    }
}

On the other hand, if you want your role to have a behaviour itself, it should definitely again hide the details of how its type is being decided:

public enum RoleType
{
    User,
    Admin
}

public class Role
{
    private RoleType _roleType;

    public bool IsAdmin()
    {
        return _roleType == RoleType.Admin;
    }

    public bool IsUser()
    {
        return _roleType == RoleType.User;
    }

    // more role-specific logic...
}

public class User
{
    private Role _role;

    public bool IsAdmin()
    {
        return _role.IsAdmin();
    }

    public bool IsUser()
    {
        return _role.IsUser();
    }
}

This is however quite verbose and the complexity would rise with each role addition - that's usually how the code ends up when you try to fully adhere to the Law of Demeter. You should improve the design, based on the concrete requirements of the system being modeled.

According to your question, I guess you'd better go with the first option with enum directly on User. Should you need more logic on the Role, the second option should be considered as a starting point.

Answer 2

It seems daft to have a function to check whether the code that is stored is the admin code. What you really want to know is whether that person is an admin. So if you don't want to expose the constants, then you also shouldn't expose that there is a code, and call the methods isAdmin () and isUser ().

That said, "if User.getRole().getCode() == Role.CODE_ADMIN" really is a handful just for checking that a user is an admin. How many things does a developer have to remember to write that line? He or she has to remember that a user has a role, a role has a code, and the Role class has constants for codes. That's a lot of information that is purely about the implementation.

Answer 3

In addition to what others have posted already, you should keep in mind that using the constant directly has another drawback: if anything changes how you handle user rights, all those places needs to be changed, too.

And it makes it horrible to enhance. Maybe you'd like to have a super user type at one point, that obviously also has admin rights. With encapsulation, it's basically a one-liner to add.

It's not only short and clean, it's also easy to use and understand. And - maybe most of all - it's hard to get wrong.

Answer 4

While I largely agree with the suggestions to avoid constants and have a method isFoo() etc., one possible counterexample.

If there are hundreds of these constants, and the calls are little used, it might not be worth the effort to write hundreds of isConstant1, isConstant2, methods. In this particular, unusual case, using the constants is reasonable.

Note that using enums or hasRole() avoids the need to write hundreds of methods, so it is the best of all possible world's.

Answer 5

I don't think either of the options you presented is fundamentally wrong.

I see you have not suggested the one thing that I would call flatly wrong: hard-coding the role codes in functions outside the Role class. That is:

if (user.getRole().equals("Administrator")) ...

I'd say is definitely wrong. I've seen programs that do this and then get mysterious errors because someone mis-spelled the string. I recall one time that a programmer wrote "stock" when the function was checking for "Stock".

If there were 100 different roles, I'd be very reluctant to write 100 functions to check for every possible one. You would presumably create them by writing the first function and then copying and pasting it 99 times, and how much you want to bet that in one of those 99 copies you'd forget to update the test or you'd get off by one when you ran through the list, so you now have

public bool isActuary() { return code==Role.ACTUARY; }
public bool isAccountant() { return code==Role.ACTUARY; }
... etc ...

Personally, I'd also avoid the chains of calls. I'd rather write

if (user.getRole().equals(Role.FOOBATER))

then

if (user.getRole().getRoleCode()==Role.FOOBATER_CODE)

and at that point why note write:

if (user.hasRole(Role.FOOBATER))

Then let the User class know how to check a role.