Est-ce une bonne pratique d'éviter les constantes en utilisant des getters?

Est-ce une bonne pratique de remplacer les constantes utilisées en dehors des classes par les getters?

Par exemple, est-il préférable d'utiliser if User.getRole().getCode() == Role.CODE_ADMINou if User.getRole().isCodeAdmin()?

Cela conduirait à cette classe:

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
    }
}

Tout d'abord, veuillez noter que faire quelque chose comme entity.underlyingEntity.underlyingEntity.method()est considéré comme une odeur de code selon la loi de Demeter . De cette façon, vous exposez de nombreux détails d'implémentation au consommateur. Et chaque besoin d'extension ou de modification d'un tel système sera très douloureux.

Donc, étant donné cela, je vous recommanderais d'avoir une méthode HasRoleou IsAdminsur le Usercommentaire de CodesInChaos. De cette façon, la façon dont les rôles sont implémentés sur l'utilisateur reste le détail de l'implémentation pour le consommateur. Et il semble également plus naturel de demander à l'utilisateur quel est son rôle au lieu de lui demander des détails sur son rôle, puis de décider en fonction de cela.

Veuillez également éviter d'utiliser strings sauf si nécessaire. nameest un bon exemple de stringvariable car le contenu est inconnu au préalable. D'un autre côté, quelque chose comme rolelorsque vous avez deux valeurs distinctes bien connues au moment de la compilation, vous feriez mieux d'utiliser un typage fort. C'est là que le type d'énumération entre en jeu ...

Comparer

public bool HasRole(string role)

avec

public enum Role { Admin, User }

public bool HasRole(Role role)

Le deuxième cas me donne beaucoup plus d'idée de ce que je devrais faire circuler. Cela m'empêche également de transmettre par erreur un invalide stringau cas où je n'aurais aucune idée de vos constantes de rôle.

Vient ensuite la décision sur l'apparence du rôle. Vous pouvez soit utiliser enum directement stocké sur l'utilisateur:

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;
    }
}

D'un autre côté, si vous voulez que votre rôle ait lui-même un comportement, il devrait définitivement cacher à nouveau les détails de la façon dont son type est décidé:

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();
    }
}

Ceci est cependant assez verbeux et la complexité augmenterait avec chaque ajout de rôle - c'est généralement ainsi que le code se termine lorsque vous essayez d'adhérer pleinement à la loi de Demeter. Vous devez améliorer la conception en fonction des exigences concrètes du système modélisé.

Selon votre question, je suppose que vous feriez mieux de choisir la première option avec enum directement User. Si vous avez besoin de plus de logique sur le Role, la deuxième option doit être considérée comme un point de départ.

Il semble idiot d'avoir une fonction pour vérifier si le code qui est stocké est le code administrateur. Ce que vous voulez vraiment savoir, c'est si cette personne est un administrateur. Donc, si vous ne souhaitez pas exposer les constantes, vous ne devez pas non plus indiquer qu'il existe un code et appeler les méthodes isAdmin () et isUser ().

Cela dit, "si User.getRole (). GetCode () == Role.CODE_ADMIN" est vraiment une poignée juste pour vérifier qu'un utilisateur est un administrateur. Combien de choses un développeur doit-il se rappeler pour écrire cette ligne? Il ou elle doit se rappeler qu'un utilisateur a un rôle, un rôle a un code et la classe Role a des constantes pour les codes. C'est beaucoup d'informations qui concernent uniquement la mise en œuvre.

En plus de ce que d'autres ont déjà publié, vous devez garder à l'esprit que l'utilisation directe de la constante présente un autre inconvénient: si quelque chose change la façon dont vous gérez les droits des utilisateurs, tous ces endroits doivent également être modifiés.

Et cela rend horrible à améliorer. Peut-être que vous aimeriez avoir un type de super-utilisateur à un moment donné, qui a évidemment aussi des droits d'administrateur. Avec l'encapsulation, c'est essentiellement une doublure à ajouter.

Ce n'est pas seulement court et propre, il est également facile à utiliser et à comprendre. Et - peut-être surtout - il est difficile de se tromper.

Bien que je sois largement d'accord avec les suggestions pour éviter les constantes et avoir une méthode, isFoo()etc., un contre-exemple possible.

S'il existe des centaines de ces constantes et que les appels sont peu utilisés, cela ne vaut peut-être pas la peine d'écrire des centaines de méthodes isConstant1, isConstant2. Dans ce cas particulier et inhabituel, l'utilisation des constantes est raisonnable.

Notez que l'utilisation d'énumérations ou hasRole()évite la nécessité d'écrire des centaines de méthodes, c'est donc la meilleure du monde possible.

Je ne pense pas que l'une ou l'autre des options que vous avez présentées soit fondamentalement erronée.

Je vois que vous n'avez pas suggéré la seule chose que j'appellerais carrément fausse: coder en dur les codes de rôle dans les fonctions en dehors de la classe Role. C'est:

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

Je dirais que c'est définitivement faux. J'ai vu des programmes qui font cela et obtiennent ensuite des erreurs mystérieuses parce que quelqu'un a mal orthographié la chaîne. Je me souviens d'une fois où un programmeur a écrit "stock" alors que la fonction vérifiait "Stock".

S'il y avait 100 rôles différents, je serais très réticent à écrire 100 fonctions pour vérifier chaque possible. Vous les créeriez vraisemblablement en écrivant la première fonction, puis en les copiant et en les collant 99 fois, et combien vous voulez parier que dans l'une de ces 99 copies, vous oublieriez de mettre à jour le test ou vous en sortiriez un quand vous avez parcouru la liste, vous avez donc maintenant

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

Personnellement, j'éviterais également les chaînes d'appels. Je préfère écrire

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

puis

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

et à ce moment-là, pourquoi noter:

if (user.hasRole(Role.FOOBATER))

Faites ensuite savoir à la classe Utilisateur comment vérifier un rôle.