In letzter Zeit erhielt ich einige negative Rückmeldungen über das Design einer Klasse, die ich ursprünglich für innovativ gehalten hatte. Nach dem Nachdenken stimme ich jedoch zu, dass es deutlich fehlerhafter als innovativ ist.
In diesem Beitrag wurde die Community eingeladen, an der Überprüfung teilzunehmen und ihr durchdachtes Feedback zu den Designfehlern zu geben und eine Lösung zu finden.
Hintergrund und Details:
Die betreffende Klasse ist ein Domänenobjekt, das in der Business-Schicht und in der Benutzeroberfläche doppelt verwendet wird. (Manchmal ist es notwendig.)
Die Klasse enthält eine Display () - Eigenschaft, die eine benutzerfreundliche Zeichenfolge zurückgibt. Die Begründung für die Aufnahme dieser Eigenschaft ist eine Bequemlichkeit für die Verbraucher der Klasse. In meiner spezifischen Verwendung war es einfach, eine Sammlung dieses Objekts an ein UI-Grid-Steuerelement in der Display () -Eigenschaft zu binden.
Die Anzeige wird über einen Delegaten implementiert, sodass die Verbraucher einen anderen Delegaten austauschen können.
Implementierung :
%Vor%...
%Vor%Die folgenden Antworten beziehen sich auf die Frage, was mit dem Design nicht stimmt und was es beheben könnte.
Angesichts der Informationen, die Sie vorgestellt haben, würde ich wahrscheinlich keinen Delegierten auf diese Weise verwenden.
Es gibt wahrscheinlich eine standardmäßige oder einfache Implementierung der Displaymethode, die es Personen ermöglicht, die Ihre Klasse verwenden möchten, ohne sich in die Interna davon einarbeiten zu müssen. Wenn sie später feststellen, dass die Anzeigemethode nicht geeignet ist, könnten sie die Anzeigemethode mit der Vererbung überschreiben.
Wenn es eine interne Klasse wäre, die nicht von anderen Leuten benutzt werden sollte, wäre es nicht so schlimm, aber wenn Sie erwarten, dass andere Leute es benutzen, ist es immer schön, am einfachsten und am häufigsten zu bleiben Art Dinge zu tun.
Wenn Sie in einem Team sind, das es wert ist, sollten Sie ihm die Frage stellen, nicht wir.
Warum hast du Kritik bekommen? Welche Regeln oder Konventionen hast du gebrochen? Was war dein negatives Feedback?
Sie sollten sie fragen, nicht wir.
Wenn Sie diese Rezension ehrlich gesagt brutal behandelt haben, dann wurde die Rezension schlecht verwaltet . Es ist durchaus möglich, sensibel Feedback zu geben, das nicht dazu führt, dass sich ein Mitarbeiter verprügelt fühlt .
Ja, es gibt Probleme mit Ihrem Design, aber sie sind bezeichnend für einen scharfen, aber jungen Ingenieur, der versucht, andere mit ihrem Verständnis einer Programmiersprache zu beeindrucken. Hast du irgendeine Anleitung bekommen, was mit deinem Design nicht stimmt? Hast du einen Mentor zugewiesen?
Fühlen Sie sich nicht so schlecht, Sie haben einen Fehler gemacht, aber lange nachdem Sie gelernt haben, Software pragmatisch zu entwerfen und zu implementieren, werden die Leute, die die "Überprüfung" durchgeführt haben, immer noch Idioten sein.
Lerne auch, dass du dich auf das Positive konzentrierst - du hast gelernt, dass dein Design in einigen Bereichen fehlgeschlagen ist. Wie viele von uns gesagt haben, nur weil eine Technik mit der Sprache oder dem Framework zur Hand ist, bedeutet das nicht, dass Sie sie benutzen sollten. Überarbeiten Sie keine Lösungen. Einfach und schnell ist normalerweise am besten.
Die Anzeige wird über einen Delegaten implementiert, so dass die Verbraucher einen anderen Delegierten austauschen können, weil ich nicht viel über die zukünftigen UI-Anzeigeanforderungen wissen kann.
Nun ... wenn ich Ihren Code überprüft hätte, hätte ich Sie für diese Aussage allein missbraucht. Du erhöhst die Komplexität von dem, was du geschrieben hast, enorm und musst mit heute arbeiten, wenn es nicht möglich ist, dass es dir später irgendwie hilft. Und das auf eine Art und Weise, die eine extrem eingeschränkte Anpassung erlaubt, wodurch die Wahrscheinlichkeit, dass es jemals wirklich nützlich sein wird, noch weiter reduziert wird!
Schreiben Sie Code, der tut, was er soll. Wenn sich die Anforderungen ändern, dann wird die allgemeine Logik in eine Bibliothek, eine Basisklasse usw. ausgeklammert. Andernfalls fügen Sie der Komplexität nur Komplexität hinzu.
Nun, für einen persönlich denke ich, dass es nie einen triftigen Grund gibt, eine schreibgeschützte Eigenschaft zu haben. Solche Eigenschaften sollten in eine Methode umgewandelt werden.
Zweitens sieht dieser Code überhaupt nicht sehr klar aus. In Windows Forms und WPF gibt es viel bessere Möglichkeiten zum Bereitstellen anpassbarer Formatierung eines Objekts oder Werts, bei denen keine Delegierten festgelegt werden. Alles, was mit UI in Verbindung steht, sollte nichts erfordern, was nicht deklarativ ausgedrückt werden kann.
Einige Vorschläge: TypeConverter , IFormattable , MVVM
Jedenfalls versuche ich nicht, ein Kritiker zu sein. Ich habe über die Jahre hinweg viel Code geschrieben, auf den ich nicht stolz bin. Aber du hast nach konstruktiver Kritik gefragt, also dachte ich, ich würde meine 2 Cent anbieten.
Wenn Sie während der Überprüfung keine Antwort auf Ihre Frage erhalten haben, ist entweder der Überprüfungsprozess unterbrochen oder Sie funktionieren nicht gut und haben nicht zugehört.
Einer oder beide müssen repariert werden.
Nur die Rezensenten könnten diese Frage beantworten - ich schlage vor, dass Sie mit ihnen sprechen. Wenn Sie nicht riskieren, immer wieder "brutalisiert" zu werden.
Wenn Sie nicht damit umgehen können, dass Ihnen gesagt wird, dass Ihr Code nicht gut ist, können Sie nicht besser werden. Willst du besser werden? Oder willst du nur, dass jeder herumläuft und dir erzählt, dass du der größte Programmierer seit Alan Turing bist?
Dein Ego würde dich nicht einmal dazu bringen, dem Code Review zuzuhören. Traurig wirklich.
Sie haben es so gelassen, dass die Leute im Stack-Überlauf raten müssen, was sie über Ihren Code gesagt haben.
Nimm zuerst dein Ego in den Griff, dann kannst du vielleicht bescheiden genug sein, um besser zu werden.
Meine Annahme wäre, was Sie in der ersten Zeile zugelassen haben - Domain-Objekte sollten nur die Daten speichern (oder auch Operationen mit dem ActiveRecord-Muster durchführen).
Die Übergabe eines Delegaten zum Formatieren der Daten in ein Domänenobjekt könnte deshalb der Grund gewesen sein.
Anders sieht es aus, wenn Sie das Domain-Objekt in ein Control überführen und herausfinden, was mit den Daten zu tun ist, anstatt mit der anderen Runde. Ich werde nicht ein ganzes Rasterbeispiel einfügen, sondern ein einfaches Label:
%Vor%Auch eine Schreibeigenschaft hat wahrscheinlich nicht geholfen.
Für mich sieht es so aus, als würdest du die Schichten zu stark koppeln. Measure consumers könnten das DisplayDelegate direkt aufrufen, es besteht keine unmittelbare Notwendigkeit, es in der Domänenklasse Measure zu verbergen, wenn es nur in der UI-Ebene verwendet wird. Und wenn Sie die Verbraucher nicht mit der Verwaltung von zwei separaten Objekten belasten möchten, fügen Sie eine MeasureView hinzu, die Measure mit DisplayDelegate transformiert.
Ich glaube, ich verstehe es nicht ... Wenn Ihr Konsument einen Verweis auf das Objekt hat, möchten Sie, dass er eine Eigenschaft setzt, die auf eine Methode verweist, die (wenn sie aufgerufen wird) eine Referenz erhält zu messen, dass sie dann mit der Ausgabe herumwühlen können, die sie wollen? Ich würde nur die Delegierten-Sachen überspringen und meine Ausgabe von dieser ursprünglichen Referenz erhalten.
Tags und Links oop code-review