È buona norma evitare le costanti usando i getter?

Innanzitutto, tieni presente che fare qualcosa come entity.underlyingEntity.underlyingEntity.method() è considerato un odore di codice in base alla Legge di Demetra . In questo modo, stai esponendo molti dettagli di implementazione al consumatore. E ogni esigenza di estensione o modifica di un tale sistema farà molto male.

Pertanto, ti consigliamo di utilizzare un metodo HasRole o IsAdmin sul User come nel commento di CodesInChaos. In questo modo, il modo in cui i ruoli vengono implementati sull'utente rimane il dettaglio dell'implementazione per il consumatore. Ed è anche più naturale chiedere all'utente quale sia il suo ruolo, invece di chiedergli dettagli sul suo ruolo e poi decidere in base a questo.

Evita anche di usare string s a meno che non sia necessario. name è un buon esempio di string variabile perché i contenuti sono sconosciuti in anticipo. D'altra parte, qualcosa come role in cui si hanno due valori distinti che sono ben noti al momento della compilazione, è meglio usare una digitazione strong. È qui che entra in gioco il tipo di enumerazione ...

Confronto

public bool HasRole(string role)

con

public enum Role { Admin, User }

public bool HasRole(Role role)

Il secondo caso mi dà un'idea molto più ampia di quello che dovrei passare. Inoltre, mi impedisce di passare erroneamente un string non valido nel caso in cui non avessi idea delle costanti di ruolo.

La prossima è la decisione su come apparirà il ruolo. Puoi utilizzare enum direttamente memorizzato sull'utente:

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'altra parte, se vuoi che il tuo ruolo abbia un comportamento in sé, dovrebbe sicuramente nascondere di nuovo i dettagli di come viene deciso il suo tipo:

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

Questo è comunque piuttosto prolisso e la complessità aumenterebbe con ogni aggiunta di ruolo - di solito è come finisce il codice quando si tenta di aderire completamente alla Legge di Demetra. Dovresti migliorare la progettazione, in base ai requisiti concreti del sistema da modellare.

Secondo la tua domanda, suppongo che faresti meglio ad andare con la prima opzione con enum direttamente su User . Se hai bisogno di più logica su Role , la seconda opzione dovrebbe essere considerata come un punto di partenza.

Sembra pazzo avere una funzione per verificare se il codice che è memorizzato è il codice amministratore. Quello che vuoi veramente sapere è se quella persona è un amministratore. Quindi se non vuoi esporre le costanti, non devi nemmeno esporre che esiste un codice e chiamare i metodi isAdmin () e isUser ().

Detto questo, "se User.getRole (). getCode () == Role.CODE_ADMIN" è davvero una manciata solo per verificare che un utente sia un amministratore. Quante cose deve ricordare uno sviluppatore per scrivere quella riga? Lui o lei deve ricordare che un utente ha un ruolo, un ruolo ha un codice e la classe Role ha costanti per i codici. Si tratta di molte informazioni che riguardano esclusivamente l'implementazione.

Oltre a ciò che altri hanno già postato, tieni presente che l'uso della costante ha un altro svantaggio: se qualcosa cambia come gestisci i diritti degli utenti, tutti quei luoghi devono essere cambiati, anche.

E rende orribile migliorare. Forse ti piacerebbe avere un tipo super utente a un certo punto, che ovviamente ha anche diritti di amministratore. Con l'incapsulamento, è fondamentalmente un one-liner da aggiungere.

Non è solo breve e pulito, è anche facile da usare e da capire. E - forse più di tutti - è difficile sbagliare.

Pur condividendo ampiamente i suggerimenti per evitare le costanti e avere un metodo isFoo() ecc., un possibile controesempio.

Se ci sono centinaia di queste costanti e le chiamate sono poco utilizzate, potrebbe non valere la pena di scrivere centinaia di metodi isConstant1, isConstant2. In questo particolare, caso insolito, l'uso delle costanti è ragionevole.

Tieni presente che l'utilizzo di enumerazioni o% dihasRole() evita la necessità di scrivere centinaia di metodi, quindi è il migliore di tutti i mondi possibili.

Non penso che nessuna delle opzioni che hai presentato sia fondamentalmente sbagliata.

Vedo che non hai suggerito l'unica cosa che chiamerei chiaramente errata: codificare i codici ruolo in funzioni esterne alla classe Role. Cioè:

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

Direi che è decisamente sbagliato. Ho visto programmi che fanno questo e poi ottengono errori misteriosi perché qualcuno ha sbagliato a scrivere la stringa. Ricordo una volta che un programmatore ha scritto "stock" quando la funzione stava verificando "Stock".

Se esistessero 100 ruoli diversi, sarei molto riluttante a scrivere 100 funzioni per verificarne la presenza. Probabilmente li creerai scrivendo la prima funzione e poi copiandoli e incollandoli 99 volte, e quanto vuoi scommettere che in una di quelle 99 copie ti dimenticherai di aggiornare il test o ne verrai fuori uno quando hai eseguito l'elenco, quindi ora hai

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

Personalmente, eviterei anche le catene di chiamate. Preferirei scrivere

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

poi

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

ea quel punto perché scrivi nota:

if (user.hasRole(Role.FOOBATER))

Quindi fai sapere alla classe User come controllare un ruolo.