Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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
    }
}
share|improve this question
15  
I'd rather use something like User.HasRole(Role.Admin). – CodesInChaos Jan 22 at 10:07
1  
4  
Check the Tell don't Ask principle. – David Packer Jan 22 at 11:08
    
I question the premise: User.getRole().getCode() is already an unpleasant read, comparing a Code to a Role makes it yet more ungainly. – msw Jan 22 at 20:53
up vote 48 down vote accepted

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.

share|improve this answer
10  
I wish this was the way people would do it everywhere. Having a getter on an otherwise privatr attribute just for the sake of checking whether it is equal to something else is such an awful practice. – David Packer Jan 22 at 11:13
2  
Re "instance.property.property.method()..." Isn't that the fluid thingy? – Peter Mortensen Jan 22 at 18:06
2  
@PeterMortensen It's different than a fluent interface. A fluent interface on type X would certainly allow you to make strings of function calls like X.foo().bar().fee().... In this fluent interface, foo, bar, and fee would all be functions inside class X returning an object of type X. But for the 'instance.property.property.method() mentioned in this example, the two property calls would actually be in separate classes. The problem there is that you're passing through several layers of abstraction in order to get hold of low level details. – Ryan Jan 22 at 18:59
10  
Good answer, but the Law of Demeter is not a dot-counting exercise. instance.property.property.method() is not necessarily a violation or even a code smell; it depends on whether you're working with objects or data structures. Node.Parent.RightChild.Remove() is probably not a violation of LoD (although smelly for other reasons). var role = User.Role; var roleCode = role.Code; var isAdmin = roleCode == ADMIN; is almost certainly a violation, despite the fact that I always "used only one dot". – Carl Leth Jan 23 at 3:23
1  
I understand that instance.property.property.method() is a violation of the LoD, but the OP has instance.method().method() which should be fine. In your last example, there is so much boilerplate code is User that only serves as a facade for the Role. – Bergi Jan 23 at 12:02

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.

share|improve this answer
3  
It's worse: That a user always has exactly one role, neither more nor less. – Deduplicator Jan 22 at 13:52

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.

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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