Ist es eine gute Praxis, Konstanten durch Verwendung von Gettern zu vermeiden?

25

Ist es eine gute Praxis, Konstanten, die außerhalb von Klassen verwendet werden, durch Getter zu ersetzen?

Ist es zum Beispiel besser, if User.getRole().getCode() == Role.CODE_ADMINoder if User.getRole().isCodeAdmin()?

Das würde zu dieser Klasse führen:

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
    }
}
gehe zu
quelle
15
Ich würde lieber so etwas verwenden User.HasRole(Role.Admin).
CodesInChaos
4
Überprüfen Sie das Tell Don't Ask- Prinzip.
Andy
Ich frage die Prämisse: Ist User.getRole().getCode()schon eine unangenehme Lektüre, macht der Vergleich eines Codes mit einer Rolle es noch unhandlicher.
msw

Antworten:

47

Zuallererst beachten Sie bitte, dass so etwas entity.underlyingEntity.underlyingEntity.method()nach dem Gesetz von Demeter als Code-Geruch betrachtet wird . Auf diese Weise legen Sie dem Verbraucher viele Implementierungsdetails offen. Und jeder Bedarf an Erweiterung oder Modifikation eines solchen Systems wird sehr schaden.

Daher würde ich Ihnen empfehlen, eine HasRoleoder IsAdmin-Methode für den UserKommentar von CodesInChaos zu verwenden. Auf diese Weise bleibt die Art und Weise, wie die Rollen auf dem Benutzer implementiert werden, ein Implementierungsdetail für den Verbraucher. Und es fühlt sich auch natürlicher an, den Benutzer nach seiner Rolle zu fragen, anstatt ihn nach Details seiner Rolle zu fragen und dann basierend darauf zu entscheiden.


Vermeiden Sie bitte auch die Verwendung von strings, sofern dies nicht erforderlich ist. nameist ein gutes Beispiel für eine stringVariable, da der Inhalt vorher nicht bekannt ist. Auf der anderen Seite so etwas wierole wenn Sie beispielsweise zwei unterschiedliche Werte haben, die zum Zeitpunkt der Kompilierung bekannt sind, eine starke Typisierung verwenden. Hier kommt der Aufzählungstyp ins Spiel ...

Vergleichen Sie

public bool HasRole(string role)

mit

public enum Role { Admin, User }

public bool HasRole(Role role)

Der zweite Fall gibt mir viel mehr Vorstellung davon, was ich herumreichen sollte. Es verhindert auch, dass ich fälschlicherweise einen ungültigen Code weitergebe, stringfalls ich keine Ahnung von Ihren Rollenkonstanten hatte.


Als nächstes wird entschieden, wie die Rolle aussehen soll. Sie können entweder die direkt auf dem Benutzer gespeicherte Aufzählung verwenden:

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

Wenn Sie möchten, dass sich Ihre Rolle selbst verhält, sollte sie die Details darüber, wie über ihren Typ entschieden wird, auf jeden Fall wieder verbergen:

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

Dies ist jedoch ziemlich ausführlich und die Komplexität würde mit jeder zusätzlichen Rolle zunehmen - so endet der Code normalerweise, wenn Sie versuchen, das Gesetz von Demeter vollständig einzuhalten. Sie sollten das Design basierend auf den konkreten Anforderungen des zu modellierenden Systems verbessern.

Ihrer Frage zufolge sollten Sie die erste Option mit Enum direkt aktivieren User. Wenn Sie mehr Logik für die benötigen Role, sollte die zweite Option als Ausgangspunkt in Betracht gezogen werden.

Zdeněk Jelínek
quelle
10
Ich wünschte, die Leute würden das überall so machen. Ein Getter für ein sonst privatr Attribut zu haben, nur um zu prüfen, ob es mit etwas anderem identisch ist, ist solch eine schreckliche Praxis.
Andy
2
Re "instance.property.property.method () ..." Ist das nicht das flüssige Ding ?
Peter Mortensen
2
@PeterMortensen Es ist anders als eine flüssige Benutzeroberfläche. Eine fließende Schnittstelle nach Typ Xwürde es Ihnen sicherlich ermöglichen, Zeichenfolgen von Funktionsaufrufen wie X.foo().bar().fee().... In dieser fließenden Benutzeroberfläche sind foo, bar und fee Funktionen innerhalb der Klasse, Xdie ein Objekt vom Typ zurückgeben X. Bei der in diesem Beispiel erwähnten 'instance.property.property.method ()' propertywürden sich die beiden Aufrufe tatsächlich in separaten Klassen befinden. Das Problem dabei ist, dass Sie mehrere Abstraktionsebenen durchlaufen, um Details auf niedriger Ebene zu erfassen.
Shaz
10
Gute Antwort, aber das Gesetz von Demeter ist keine Übung zum Punktezählen. instance.property.property.method()ist nicht unbedingt eine Verletzung oder sogar ein Code-Geruch; Es hängt davon ab, ob Sie mit Objekten oder Datenstrukturen arbeiten. Node.Parent.RightChild.Remove()ist wahrscheinlich keine Verletzung von LoD (obwohl es aus anderen Gründen stinkt). var role = User.Role; var roleCode = role.Code; var isAdmin = roleCode == ADMIN;ist mit ziemlicher Sicherheit ein Verstoß, obwohl ich immer "nur einen Punkt" verwendet habe.
Carl Leth
1
Ich verstehe, dass dies instance.property.property.method()eine Verletzung der LoD ist, aber das OP hat instance.method().method()was in Ordnung sein sollte. In Ihrem letzten Beispiel gibt es so viel Boilerplate-Code User, der nur als Fassade für den dient Role.
Bergi
9

Es erscheint unsinnig, eine Funktion zu haben, um zu prüfen, ob der gespeicherte Code der Admin-Code ist. Was Sie wirklich wissen möchten, ist, ob diese Person ein Administrator ist. Also , wenn Sie nicht wollen , die Konstanten belichten, dann sollten Sie auch nicht aussetzen , dass es ein Code ist, und rufen Sie die Methoden isAdmin () und isUser ().

Das heißt, "if User.getRole (). GetCode () == Role.CODE_ADMIN" ist wirklich eine Handvoll, nur um zu überprüfen, ob ein Benutzer ein Administrator ist. Wie viele Dinge muss sich ein Entwickler merken, um diese Zeile zu schreiben? Er muss sich daran erinnern, dass ein Benutzer eine Rolle, eine Rolle einen Code und die Rollenklasse Konstanten für Codes hat. Das sind viele Informationen, die sich ausschließlich auf die Implementierung beziehen.

gnasher729
quelle
3
Schlimmer noch: Ein Benutzer hat immer genau eine Rolle, weder mehr noch weniger.
Deduplizierer
5

Zusätzlich zu dem, was andere bereits gepostet haben, sollten Sie berücksichtigen, dass die direkte Verwendung der Konstante einen weiteren Nachteil hat: Wenn sich etwas ändert, wie Sie mit Benutzerrechten umgehen, alle diese Stellen geändert werden.

Und es macht es schrecklich zu verbessern. Vielleicht möchten Sie eine haben Super - User Typ haben, der natürlich auch Administratorrechte hat. Bei der Verkapselung muss im Grunde genommen ein Einzeiler hinzugefügt werden.

Es ist nicht nur kurz und sauber, sondern auch einfach zu bedienen und zu verstehen. Und - vielleicht am allermeisten - es ist schwer, sich zu irren.

Eiko
quelle
2

Ich stimme zwar weitgehend mit den Vorschlägen überein, Konstanten zu vermeiden und eine Methode zu haben isFoo() usw. , ist dies ein mögliches Gegenbeispiel.

Wenn es gibt Hunderte gibt dieser Konstanten gibt und die Aufrufe wenig genutzt werden, lohnt es sich möglicherweise nicht, Hunderte von Methoden isConstant1, isConstant2 zu schreiben. In diesem besonderen, ungewöhnlichen Fall ist die Verwendung der Konstanten sinnvoll.

Beachten Sie, dass die Verwendung von Aufzählungen oder hasRole()das Vermeiden des Schreibens von Hunderten von Methoden erforderlich ist, sodass dies die beste von allen möglichen Methoden der Welt ist.

user949300
quelle
2

Ich halte keine der von Ihnen präsentierten Optionen für grundlegend falsch.

Ich sehe, Sie haben nicht die eine Sache vorgeschlagen, die ich völlig falsch nennen würde: die Rollencodes in Funktionen außerhalb der Rollenklasse hart zu codieren. Das ist:

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

Ich würde sagen, ist definitiv falsch. Ich habe Programme gesehen, die dies tun und dann mysteriöse Fehler bekommen, weil jemand die Zeichenfolge falsch geschrieben hat. Ich erinnere mich, dass ein Programmierer einmal "Lager" geschrieben hat, als die Funktion nach "Lager" suchte.

Wenn es 100 verschiedene Rollen gäbe, würde ich sehr ungern 100 Funktionen schreiben, um nach jeder möglichen zu suchen. Sie würden sie vermutlich erstellen, indem Sie die erste Funktion schreiben und dann 99 Mal kopieren und einfügen und wie viel Sie darauf setzen möchten, dass Sie bei einer dieser 99 Kopien vergessen würden, den Test zu aktualisieren, oder um eins aussteigen, wenn Sie haben die Liste durchgearbeitet, haben Sie also jetzt

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

Persönlich würde ich auch die Ketten von Anrufen vermeiden. Ich würde lieber schreiben

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

dann

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

und an diesem Punkt, warum schreibe:

if (user.hasRole(Role.FOOBATER))

Lassen Sie dann die Benutzerklasse wissen, wie eine Rolle überprüft wird.

Jay
quelle