Sollten Sie in solchen Fällen immer das Nötigste an Daten an eine Funktion übergeben?

81

Angenommen, ich habe eine Funktion IsAdmin, die überprüft, ob ein Benutzer ein Administrator ist. Nehmen wir auch an, dass die Administratorüberprüfung durchgeführt wird, indem Benutzer-ID, Name und Kennwort mit einer Regel abgeglichen werden (nicht wichtig).

In meinem Kopf gibt es dann zwei mögliche Funktionssignaturen dafür:

public bool IsAdmin(User user);
public bool IsAdmin(int id, string name, string password);

Am häufigsten greife ich zur zweiten Art der Unterschrift und denke, dass:

  • Die Funktionssignatur gibt dem Leser viel mehr Informationen
  • Die in der Funktion enthaltene Logik muss nichts über die UserKlasse wissen
  • Es führt normalerweise zu etwas weniger Code in der Funktion

Allerdings stelle ich diesen Ansatz manchmal in Frage und stelle auch fest, dass er irgendwann unhandlich wird. Wenn zum Beispiel eine Funktion zwischen zehn verschiedenen Objektfeldern in einen resultierenden Bool abbilden würde, würde ich natürlich das gesamte Objekt einschicken. Aber abgesehen von einem so krassen Beispiel sehe ich keinen Grund, das eigentliche Objekt weiterzugeben.

Ich würde mich über Argumente für beide Stile sowie über allgemeine Beobachtungen freuen, die Sie anbieten könnten.

Ich programmiere sowohl in objektorientierten als auch in funktionalen Stilen, daher sollte die Frage in Bezug auf alle Redewendungen gesehen werden.

Anders Arpi
quelle
40
Off-Topic: Warum hängt der Status "Ist Admin" eines Benutzers aus Neugier von seinem Passwort ab?
Stakx
43
@ syb0rg Falsch. In C # ist das die Namenskonvention .
Magnattic
68
@ syb0rg Richtig, er hat die Sprache nicht angegeben, daher finde ich es ziemlich seltsam, ihm mitzuteilen, dass er gegen die Konvention einer bestimmten Sprache verstößt.
Magnattic
31
@ syb0rg Die allgemeine Aussage, dass "Funktionsnamen nicht mit einem Großbuchstaben beginnen sollten", ist falsch, weil es Sprachen gibt, in denen sie sollten. Wie auch immer, ich wollte dich nicht beleidigen, ich habe nur versucht, das aufzuklären. Dies wird zu der Frage selbst ziemlich unangebracht. Ich sehe keine Notwendigkeit, aber wenn Sie fortfahren möchten, können Sie die Debatte in den Chat verschieben .
Magnattic
6
Generell ist es in der Regel eine schlechte Sache , die eigene Sicherheit auf den Prüfstand zu stellen . Die meisten Sprachen und Frameworks verfügen über ein Sicherheits- / Berechtigungssystem, und es gibt gute Gründe, sie zu verwenden, auch wenn Sie etwas Einfacheres wünschen.
James Snell

Antworten:

123

Ich persönlich bevorzuge die erste Methode von nur IsAdmin(User user)

Es ist viel einfacher zu verwenden, und wenn sich Ihre Kriterien für IsAdmin zu einem späteren Zeitpunkt ändern (möglicherweise basierend auf Rollen oder isActive), müssen Sie Ihre Methodensignatur nicht überall neu schreiben.

Es ist wahrscheinlich auch sicherer, da Sie nicht bekannt geben, welche Eigenschaften bestimmen, ob ein Benutzer ein Administrator ist oder nicht, oder die Kennworteigenschaft überall weitergeben. Und syrion macht einen guten Punkt, dass was passiert, wenn Ihr idnicht mit dem name/ übereinstimmt password?

Die Länge des Codes in einer Funktion sollte eigentlich keine Rolle spielen, wenn die Methode ihre Aufgabe erfüllt, und ich hätte lieber einen kürzeren und einfacheren Anwendungscode als einen Hilfsmethodencode.

Rachel
quelle
3
Ich denke, das Refactoring-Problem ist ein sehr guter Punkt - danke.
Anders Arpi
1
Ich würde das Argument der Methodensignatur machen, wenn Sie es nicht taten. Dies ist besonders hilfreich, wenn viele Abhängigkeiten von Ihrer Methode bestehen, die von anderen Programmierern verwaltet werden. Dies hilft, die Menschen voneinander fernzuhalten. +1
jmort253
1
Ich stimme mit dieser Antwort, aber lassen Sie mich Flagge zwei Situationen , in denen der andere Ansatz ( „Mindestdaten“) könnte vorzuziehen sein: (1) Sie wollen Methode Ergebnisse zwischenzuspeichern. Es ist besser, nicht ein ganzes Objekt als Cache-Schlüssel zu verwenden oder die Eigenschaften eines Objekts bei jedem Aufruf zu überprüfen. (2) Sie möchten, dass die Methode / Funktion asynchron ausgeführt wird, wobei die Argumente möglicherweise serialisiert und beispielsweise in einer Tabelle gespeichert werden. Beim Verwalten von ausstehenden Jobs ist es einfacher und einfacher, eine Ganzzahl nachzuschlagen als ein Objekt-Blob.
GladstoneKeep
Das primitive Obsession-Anti-Pattern könnte für Unit-Tests schrecklich sein.
Samuel
82

Die erste Signatur ist überlegen, da sie die Kapselung benutzerbezogener Logik im UserObjekt selbst ermöglicht. Es ist nicht vorteilhaft, eine Logik für die Erstellung des Tupels "id, name, password" zu haben, das über Ihre Codebasis verstreut ist. Darüber hinaus erschwert es die isAdminFunktion: Was passiert, wenn jemand zum Beispiel ein idnicht passendes Passwort eingibt name? Was ist, wenn Sie überprüfen möchten, ob ein bestimmter Benutzer ein Administrator aus einem Kontext ist, in dem Sie dessen Kennwort nicht kennen sollten?

Außerdem als Anmerkung zu Ihrem dritten Punkt zugunsten des Stils mit zusätzlichen Argumenten; es kann zu "weniger Code in der Funktion" führen, aber wohin geht diese Logik? Es kann nicht einfach verschwinden! Stattdessen ist es auf jeden einzelnen Ort verteilt, an dem die Funktion aufgerufen wird. Um fünf Codezeilen an einem Ort zu speichern, haben Sie fünf Zeilen pro Verwendung der Funktion bezahlt!

asthasr
quelle
45
Wäre es Ihrer Logik folgend nicht user.IsAdmin()viel besser?
Anders Arpi
13
Ja absolut.
Asthasr
6
@ AndersHolmström Es kommt darauf an, ob Sie testen, ob der Benutzer im Allgemeinen ein Administrator ist, oder nur ein Administrator für die aktuelle Seite / das aktuelle Formular, auf dem Sie sich befinden. Wenn im Allgemeinen, dann ja, das wäre besser, aber wenn Sie nur IsAdminfür eine bestimmte Form oder Funktion testen , sollte dies für den Benutzer nicht relevant sein
Rachel
40
@Anders: nein sollte es nicht. Ein Benutzer sollte sich nicht entscheiden, ob es sich um einen Administrator handelt oder nicht. Das Privilegien- oder Sicherheitssystem sollte. Etwas, das eng mit einer Klasse verbunden ist, bedeutet nicht, dass es eine gute Idee ist, sie zu einem Teil dieser Klasse zu machen
Marjan Venema,
11
@MarjanVenema - Die Idee, dass die Benutzerklasse nicht "entscheiden" darf, ob eine Instanz ein Administrator ist, scheint mir etwas frachtkultig zu sein. Diese Verallgemeinerung kann man wirklich nicht so stark machen. Wenn Ihre Anforderungen komplex sind, ist es vielleicht vorteilhaft, sie in einem separaten "System" zu kapseln, aber unter Berufung auf KISS + YAGNI würde ich diese Entscheidung gerne angesichts dieser Komplexität treffen, und nicht generell :-). Und beachten Sie, dass auch wenn die Logik nicht „Teil“ des Benutzers ist es immer noch nützlich sein kann , als eine Frage der Praktikabilität einen Pass - Through auf Benutzer zu haben , dass nur die Delegierten zu einem komplexeren System.
Eamon Nerbonne
65

Die erste, aber nicht (nur) aus den Gründen, die andere angegeben haben.

public bool IsAdmin(User user);

Dies ist typsicher. Insbesondere da User ein Typ ist, den Sie selbst definiert haben, gibt es kaum oder keine Möglichkeit, Argumente zu transponieren oder zu vertauschen. Es kann eindeutig auf Benutzer angewendet werden und ist keine generische Funktion, die int und zwei Zeichenfolgen akzeptiert. Dies ist ein wichtiger Punkt bei der Verwendung einer typsicheren Sprache wie Java oder C #, wie Ihr Code aussieht. Wenn Sie nach einer bestimmten Sprache fragen, möchten Sie Ihrer Frage möglicherweise ein Tag für diese Sprache hinzufügen.

public bool IsAdmin(int id, string name, string password);

Hier genügen int und zwei Zeichenketten. Was hindert Sie daran, den Namen und das Passwort zu übertragen? Die zweite Methode ist aufwändiger und bietet mehr Möglichkeiten für Fehler.

Vermutlich erfordern beide Funktionen, dass user.getId (), user.getName () und user.getPassword () entweder innerhalb der Funktion (im ersten Fall) oder vor dem Aufrufen der Funktion (im zweiten Fall) aufgerufen werden Das Ausmaß der Kopplung ist in beiden Fällen gleich. Da diese Funktion nur für Benutzer gültig ist, würde ich in Java alle Argumente entfernen und diese Methode zu einer Instanz für Benutzerobjekte machen:

user.isAdmin();

Da diese Funktion bereits eng mit den Benutzern verknüpft ist, ist es sinnvoll, sie zu einem Teil dessen zu machen, was ein Benutzer ist.

PS Ich bin sicher, dass dies nur ein Beispiel ist, aber es sieht so aus, als würden Sie ein Passwort speichern. Sie sollten stattdessen nur einen kryptografisch sicheren Passwort-Hash speichern. Während der Anmeldung sollte das angegebene Passwort auf die gleiche Weise gehasht und mit dem gespeicherten Hash verglichen werden. Das Versenden von Passwörtern im Klartext sollte vermieden werden. Wenn Sie gegen diese Regel verstoßen und isAdmin (int Str Str) den Benutzernamen protokollieren soll, wird möglicherweise stattdessen das Kennwort protokolliert, wodurch ein Sicherheitsproblem entsteht (schreiben Sie keine Kennwörter in die Protokolle) ) und ist ein weiteres Argument für die Übergabe eines Objekts anstelle seiner Bestandteile (oder die Verwendung einer Klassenmethode).

GlenPeterson
quelle
11
Ein Benutzer sollte sich nicht entscheiden, ob es sich um einen Administrator handelt oder nicht. Das Privilegien- oder Sicherheitssystem sollte. Etwas, das eng mit einer Klasse verbunden ist, bedeutet nicht, dass es eine gute Idee ist, sie zu einem Teil dieser Klasse zu machen.
Marjan Venema
6
@MarjanVenema Der Benutzer entscheidet nichts. Das Benutzerobjekt / die Tabelle speichert Daten über jeden Benutzer. Tatsächliche Benutzer können nicht alles an sich selbst ändern. Selbst wenn der Benutzer Änderungen vornimmt, die ihm gestattet sind, kann dies zu Sicherheitswarnungen wie einer E-Mail führen, wenn er seine E-Mail-Adresse, seine Benutzer-ID oder sein Kennwort ändert. Verwenden Sie ein System, in dem Benutzer auf magische Weise alles an bestimmten Objekttypen ändern dürfen? Ich kenne ein solches System nicht.
GlenPeterson
2
@ GlenPeterson: Verstehst du mich absichtlich falsch? Als ich Benutzer sagte, meinte ich natürlich die Benutzerklasse.
Marjan Venema
2
@GlenPeterson Völlig ohne Thema, aber mehrere Runden von Hashing- und kryptografischen Funktionen werden die Daten schwächen.
Gusdor
3
@ MarjanVenema: Ich bin nicht absichtlich schwierig. Ich denke, wir haben nur sehr unterschiedliche Perspektiven und ich möchte Ihre besser verstehen. Ich habe eine neue Frage geöffnet, bei der dies besser erörtert werden kann: programmers.stackexchange.com/questions/216443/…
GlenPeterson,
6

Wenn Ihre gewählte Sprache Strukturen nicht nach Wert (User user)übergibt, scheint die Variante besser zu sein. Was ist, wenn Sie sich eines Tages dazu entschließen, den Benutzernamen zu entfernen und nur die ID / das Passwort zur Identifizierung des Benutzers zu verwenden?

Außerdem führt dieser Ansatz zwangsläufig zu langen und überlasteten Methodenaufrufen oder Inkonsistenzen. Sind 3 Argumente in Ordnung? Wie wäre es mit 5? Oder 6? Warum darüber nachdenken, wenn das Übergeben eines Objekts in Bezug auf Ressourcen billig ist (wahrscheinlich sogar billiger als das Übergeben von drei Argumenten)?

Und ich bin (irgendwie) anderer Meinung, dass der zweite Ansatz dem Leser mehr Informationen liefert - intuitiv fragt der Methodenaufruf "ob der Benutzer Administratorrechte hat", nicht "ob die angegebene Kombination aus ID, Name und Passwort Administratorrechte hat".

Wenn das Übergeben des UserObjekts nicht zum Kopieren einer großen Struktur führen würde, ist der erste Ansatz für mich klarer und logischer.

Maciej Stachowski
quelle
3

Ich hätte wahrscheinlich beides. Die erste als externe API mit Hoffnung auf Stabilität in der Zeit. Die zweite als private Implementierung, die von der externen API aufgerufen wird.

Wenn ich zu einem späteren Zeitpunkt die Prüfregel ändern muss, würde ich einfach eine neue private Funktion mit einer neuen Signatur schreiben, die von der externen aufgerufen wird.

Dies hat den Vorteil, dass die innere Implementierung leicht geändert werden kann. Es ist auch durchaus üblich, dass Sie manchmal beide Funktionen gleichzeitig benötigen und abhängig von einer Änderung des externen Kontexts die eine oder andere aufrufen müssen (z. B. können alte Benutzer und neue Benutzer mit unterschiedlichen Feldern gleichzeitig vorhanden sein).

In Bezug auf den Titel der Frage geben Sie in beiden Fällen die minimal erforderlichen Informationen an. Ein Benutzer scheint die kleinste anwendungsbezogene Datenstruktur zu sein, die die Informationen enthält. Die drei Felder id, password und name scheinen die kleinsten Implementierungsdaten zu sein, die tatsächlich benötigt werden, es handelt sich jedoch nicht wirklich um Objekte auf Anwendungsebene.

Mit anderen Worten, wenn Sie sich mit einer Datenbank befassen, wäre ein Benutzer ein Datensatz, während ID, Kennwort und Anmeldung Felder in diesem Datensatz wären.

kriss
quelle
Ich sehe keinen Grund für die private Methode. Warum beides pflegen? Wenn die private Methode andere Argumente benötigt, müssen Sie noch eines der öffentlichen ändern. Und Sie werden es durch öffentliche testen. Und nein, es ist nicht üblich, dass Sie beide zu einem späteren Zeitpunkt benötigen. Sie raten hier - YAGNI.
Piotr Perak
Sie gehen beispielsweise davon aus, dass die Benutzerdatenstruktur von Anfang an kein anderes Feld enthält. Es ist normalerweise nicht wahr. Die zweite Funktion macht deutlich, welcher innere Teil des Benutzers daraufhin überprüft wird, ob es sich um einen Administrator handelt. Typische Anwendungsfälle können von der Art sein: Wenn der Zeitpunkt der Benutzererstellung vor einem bestimmten Datum liegt, rufen Sie die alte Regel auf, wenn keine neue Regel aufgerufen wird (die von anderen Teilen des Benutzers abhängen kann oder von derselben, aber mit einer anderen Regel). Wenn Sie den Code auf diese Weise aufteilen, erhalten Sie zwei minimale Funktionen: eine minimale konzeptionelle API und eine minimale Implementierungsfunktion.
kriss
Mit anderen Worten, auf diese Weise ist aus der internen Funktionssignatur sofort ersichtlich, welche Teile des Benutzers verwendet werden oder nicht. Im Seriencode ist das nicht immer offensichtlich. Ich arbeite derzeit an einer großen Codebasis mit einer mehrjährigen Wartungshistorie und diese Art der Aufteilung ist sehr nützlich für die langfristige Wartung. Wenn Sie nicht beabsichtigen, den Code zu pflegen, ist er in der Tat nutzlos (aber auch das Bereitstellen einer stabilen konzeptionellen API ist wahrscheinlich nutzlos).
kriss
Ein anderes Wort: Ich werde die interne Methode mit Sicherheit nicht durch eine externe testen. Das ist eine schlechte Testpraxis.
kriss
1
Dies ist eine gute Praxis, und es gibt einen Namen dafür. Es heißt das "Prinzip der einmaligen Verantwortung". Es kann sowohl auf Klassen als auch auf Methoden angewendet werden. Methoden mit einer einzigen Verantwortung sind eine gute Möglichkeit, modulareren und wartbareren Code zu erstellen. In diesem Fall wählt eine Aufgabe einen Basisdatensatz aus dem Benutzerobjekt aus und eine andere Aufgabe prüft diesen Basisdatensatz, um festzustellen, ob der Benutzer ein Administrator ist. Durch Befolgen dieses Prinzips erhalten Sie den Vorteil, dass Sie Methoden erstellen und Codeduplizierungen vermeiden können. Dies bedeutet auch, wie von @kriss angegeben, dass Sie bessere Unit-Tests erhalten.
diegoreymendez
3

Es gibt einen negativen Punkt beim Senden von Klassen (Referenztypen) an Methoden, nämlich die Sicherheitsanfälligkeit bezüglich Massenzuweisung . Stellen Sie sich vor, dass IsAdmin(User user)ich anstelle einer Methode eine UpdateUser(User user)Methode habe.

Wenn Userclass eine boolesche Eigenschaft namens IsAdminhat und ich sie während der Ausführung meiner Methode nicht überprüfe, bin ich anfällig für Massenzuweisungen. GitHub wurde 2012 von genau dieser Signatur angegriffen.

Saeed Neamati
quelle
2

Ich würde mit diesem Ansatz gehen:

public bool IsAdmin(this IUser user) { /* ... */ }

public class User: IUser { /* ... */ }

public interface IUser
{
    string Username {get;}
    string Password {get;}
    IID Id {get;}
}
  • Die Verwendung des Schnittstellentyps als Parameter für diese Funktion ermöglicht die Übergabe von Benutzerobjekten, die Definition von nachgebildeten Benutzerobjekten zum Testen und bietet Ihnen mehr Flexibilität bei der Verwendung und Wartung dieser Funktion.

  • Ich habe auch das Schlüsselwort hinzugefügt this, um die Funktion zu einer Erweiterungsmethode aller IUser-Klassen zu machen. Auf diese Weise können Sie OO-freundlichen Code schreiben und diese Funktion in Linq-Abfragen verwenden. Noch einfacher wäre es, diese Funktion in IUser und User zu definieren, aber ich nehme an, dass es einen Grund gibt, warum Sie sich entschieden haben, diese Methode außerhalb dieser Klasse zu platzieren?

  • IIDStattdessen verwende ich die benutzerdefinierte Benutzeroberfläche, damit intSie die Eigenschaften Ihrer ID neu definieren können. zB sollten Sie jemals von ändern müssen intzu long, Guidoder etwas anderes. Das ist wahrscheinlich übertrieben, aber es ist immer gut zu versuchen, die Dinge so flexibel zu gestalten, dass Sie nicht an frühzeitige Entscheidungen gebunden sind.

  • Hinweis: Ihr IUser-Objekt kann sich in einem anderen Namespace und / oder einer anderen Assembly als die User-Klasse befinden. Sie haben daher die Möglichkeit, Ihren Benutzercode vollständig von Ihrem IsAdmin-Code zu trennen, sodass nur eine referenzierte Bibliothek gemeinsam genutzt wird. Genau das, was Sie dort tun, ist eine Aufforderung, wie Sie vermuten, dass diese Gegenstände verwendet werden.

JohnLBevan
quelle
3
IUser ist hier nutzlos und erhöht nur die Komplexität. Ich verstehe nicht, warum Sie in Tests keinen echten Benutzer verwenden konnten.
Piotr Perak
1
Es erhöht die Flexibilität der Zukunft mit sehr geringem Aufwand, so dass es nicht schadet, obwohl es derzeit keinen Mehrwert bietet. Persönlich mache ich das am liebsten immer, da ich nicht über zukünftige Möglichkeiten für alle Szenarien nachdenken muss (was angesichts der unvorhersehbaren Natur der Anforderungen so gut wie unmöglich ist und viel mehr Zeit in Anspruch nehmen wird, um eine teilweise gute Antwort zu erhalten, als dies der Fall wäre) in eine Schnittstelle stecken).
JohnLBevan
@Peri eine echte Benutzerklasse zu erstellen kann komplex sein. Wenn Sie eine Schnittstelle verwenden, können Sie sie ausspotten, um Ihre Tests einfach zu halten
jk.
@ JohnLBevan: YAGNI !!!
Piotr Perak
@jk .: Wenn es schwierig ist, User zu bauen, dann stimmt etwas nicht
Piotr Perak
1

Haftungsausschluss:

In meiner Antwort werde ich mich auf das Stilproblem konzentrieren und vergessen, ob eine ID, ein Login und ein einfaches Passwort aus Sicherheitsgründen eine gute Auswahl an Parametern sind. Sie sollten in der Lage sein, meine Antwort auf alle grundlegenden Daten zu extrapolieren, die Sie verwenden, um die Berechtigungen des Benutzers herauszufinden ...

Ich halte user.isAdmin()das auch für äquivalent zu IsAdmin(User user). Das ist eine Entscheidung, die Sie treffen müssen.

Antworten:

Meine Empfehlung gilt nur für beide Benutzer:

public bool IsAdmin(User user);

Oder verwenden Sie beide nach folgenden Grundsätzen:

public bool IsAdmin(User user); // calls the private method below
private bool IsAdmin(int id, string name, string password);

Gründe für die öffentliche Methode:

Wenn Sie öffentliche Methoden schreiben, ist es normalerweise eine gute Idee, darüber nachzudenken, wie sie verwendet werden.

In diesem speziellen Fall wird diese Methode normalerweise aufgerufen, um herauszufinden, ob ein bestimmter Benutzer ein Administrator ist. Das ist eine Methode, die Sie brauchen. Sie möchten wirklich nicht, dass jeder Anrufer die richtigen Parameter zum Senden auswählen muss (und riskieren Fehler aufgrund von Code-Duplikationen).

Gründe für die private Methode:

Die private Methode, die nur den Mindestsatz der erforderlichen Parameter empfängt, kann manchmal eine gute Sache sein. Manchmal nicht so sehr.

Das Aufteilen dieser Methoden hat den Vorteil, dass Sie die private Version von mehreren öffentlichen Methoden in derselben Klasse aufrufen können. Zum Beispiel, wenn Sie (aus welchem ​​Grund auch immer) eine etwas andere Logik für Localuserund RemoteUser(vielleicht gibt es eine Einstellung zum Ein- / Ausschalten von Remote-Administratoren?) Haben wollten :

public bool IsAdmin(Localuser user); // Just call private method
public bool IsAdmin(RemoteUser user); // Check if setting is on, then call private method
private bool IsAdmin(int id, string name, string password);

Wenn Sie aus irgendeinem Grund die private Methode öffentlich machen müssen, können Sie dies auch. Es ist so einfach wie nur die Änderung privatean public. Es mag für diesen Fall nicht so großartig erscheinen, aber manchmal ist es wirklich so.

Außerdem können Sie beim Testen von Einheiten wesentlich mehr Atomtests durchführen. Es ist normalerweise sehr schön, kein vollständiges Benutzerobjekt erstellen zu müssen, um Tests mit dieser Methode auszuführen. Wenn Sie eine vollständige Abdeckung wünschen, können Sie beide Anrufe testen.

diegoreymendez
quelle
0
public bool IsAdmin(int id, string name, string password);

ist aus den genannten Gründen schlecht. Das heißt nicht

public bool IsAdmin(User user);

ist automatisch gut. Insbesondere dann , wenn Benutzer eine Objektmethode, die wie: getOrders(), getFriends(), getAdresses(), ...

Sie können die Domäne mit einem UserCredentials-Typ umgestalten, der nur die ID, den Namen und das Kennwort enthält, und diesen anstelle aller nicht benötigten Benutzerdaten an IsAdmin übergeben.

tkruse
quelle