Ab wann ist Kürze keine Tugend mehr?

Nach einer kürzlichen Fehlerbehebung musste ich den von anderen Teammitgliedern geschriebenen Code durchgehen, wobei ich Folgendes fand (es ist C #):

return (decimal)CostIn > 0 && CostOut > 0 ? (((decimal)CostOut - (decimal)CostIn) / (decimal)CostOut) * 100 : 0;

Wenn man bedenkt, dass es für all diese Darsteller einen guten Grund gibt, scheint es immer noch sehr schwierig zu sein, dem zu folgen. Es gab einen kleinen Fehler in der Berechnung und ich musste ihn entwirren, um das Problem zu beheben.

Ich kenne den Codierungsstil dieser Person aus der Codeüberprüfung, und sein Ansatz ist, dass kürzer fast immer besser ist. Und da gibt es natürlich einen Wert: Wir haben alle unnötig komplexe Ketten bedingter Logik gesehen, die mit ein paar gut platzierten Operatoren aufgeräumt werden könnten. Aber er ist eindeutig geschickter als ich, wenn es darum geht, Bedienerketten zu einer einzigen Aussage zusammenzufassen.

Dies ist natürlich letztendlich eine Frage des Stils. Wurde jedoch irgendetwas geschrieben oder erforscht, um den Punkt zu erkennen, an dem das Streben nach Kürze des Codes nicht mehr nützlich ist und zum Hindernis für das Verständnis wird?

_{Der Grund für die Besetzungen ist Entity Framework. Die Datenbank muss diese als nullfähige Typen speichern. Dezimal? ist nicht gleichbedeutend mit Decimal in C # und muss umgewandelt werden.}

Zur Beantwortung Ihrer Frage zur vorhandenen Forschung

Wurde jedoch irgendetwas geschrieben oder erforscht, um den Punkt zu erkennen, an dem das Streben nach Kürze des Codes nicht mehr nützlich ist und zum Hindernis für das Verständnis wird?

Ja, in diesem Bereich wurde gearbeitet.

Um ein Verständnis für diese Dinge zu erlangen, müssen Sie einen Weg finden, eine Metrik zu berechnen, damit Vergleiche auf quantitativer Basis durchgeführt werden können (anstatt den Vergleich nur auf der Grundlage von Witz und Intuition durchzuführen, wie es die anderen Antworten tun). Eine mögliche Metrik, die untersucht wurde, ist

Zyklomatische Komplexität ÷ Quellcodezeilen ( SLOC )

In Ihrem Codebeispiel ist dieses Verhältnis sehr hoch, da alles auf eine Zeile komprimiert wurde.

Die SATC hat festgestellt, dass die effektivste Bewertung eine Kombination aus Größe und [zyklomatischer] Komplexität ist. Die Module mit sowohl einer hohen Komplexität als auch einer großen Größe neigen dazu, die geringste Zuverlässigkeit zu haben. Module mit geringer Größe und hoher Komplexität stellen auch ein Zuverlässigkeitsrisiko dar, da es sich bei ihnen in der Regel um sehr knappen Code handelt, der schwer zu ändern oder zu modifizieren ist.

Verknüpfung

Hier sind einige Referenzen, wenn Sie interessiert sind:

McCabe, T. und A. Watson (1994), Software-Komplexität (CrossTalk: The Journal of Defense Software Engineering).

Watson, AH & McCabe, TJ (1996). Strukturiertes Testen: Eine Testmethode unter Verwendung der Cyclomatic Complexity Metric (NIST Special Publication 500-235). Abgerufen am 14. Mai 2011 von der McCabe Software-Website: http://www.mccabe.com/pdf/mccabe-nist235r.pdf

Rosenberg, L., Hammer, T., Shaw, J. (1998). Software-Metriken und Zuverlässigkeit (Fortführung des IEEE International Symposium on Software Reliability Engineering). Abgerufen am 14. Mai 2011 von der Website der Penn State University: http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.104.4041&rep=rep1&type=pdf

Meine Meinung und Lösung

Persönlich habe ich nie Kürze, nur Lesbarkeit geschätzt. Manchmal hilft Kürze die Lesbarkeit, manchmal nicht. Wichtiger ist, dass Sie Really Obvious Code (ROC) anstelle von Write-Only Code (WOC) schreiben.

Nur zum Spaß, so würde ich es schreiben und Mitglieder meines Teams bitten, es zu schreiben:

if ((costIn <= 0) || (costOut <= 0)) return 0;
decimal changeAmount = costOut - costIn;
decimal changePercent = changeAmount / costOut * 100;
return changePercent;

Beachten Sie auch, dass die Einführung der Arbeitsvariablen den glücklichen Nebeneffekt hat, Festkomma-Arithmetik anstelle von Ganzzahl-Arithmetik auszulösen, so dass die Notwendigkeit für alle diese Casts decimalbeseitigt ist.

Die Kürze ist gut, wenn das Durcheinander der wichtigen Dinge verringert wird. Wenn es jedoch knapp wird und zu viele relevante Daten zu dicht gepackt werden , um sie ohne Weiteres zu verfolgen, werden die relevanten Daten selbst zu einem Durcheinander, und Sie haben ein Problem.

In diesem speziellen Fall werden die Abdrücke decimalwiederholt. es wäre wahrscheinlich insgesamt besser, es so umzuschreiben:

var decIn = (decimal)CostIn;
var decOut = (decimal)CostOut;
return decIn > 0 && CostOut > 0 ? (decOut - decIn ) / decOut * 100 : 0;
//                  ^ as in the question

Plötzlich ist die Zeile mit der Logik viel kürzer und passt auf eine horizontale Zeile, sodass Sie alles sehen können, ohne scrollen zu müssen, und die Bedeutung ist viel deutlicher.

Ich kann zwar keine bestimmten Untersuchungen zu diesem Thema anführen, aber ich würde vorschlagen, dass all diese Darstellungen in Ihrem Code gegen das Prinzip "Don't Repeat Yourself" verstoßen. Was ist Ihr Code erscheint das zu tun , um zu versuchen , ist zu konvertieren costInund costOutzu geben Decimal, und dann einige Integritätsprüfungen auf den Ergebnissen solcher Umwandlungen durchführen, und die Durchführung von zusätzlichen Operationen auf diesen gewandelten Werten , wenn die Kontrollen passieren. Tatsächlich führt Ihr Code eine der Sicherheitsüberprüfungen für einen nicht konvertierten Wert durch, wodurch sich die Möglichkeit ergibt, dass costOut einen Wert enthält, der größer als Null ist, jedoch weniger als halb so groß wie der kleinste Wert ungleich Null, der dargestellt werden Decimalkann. Der Code wäre viel klarer, wenn er Variablen vom Typ definieren würde Decimal, die die konvertierten Werte enthalten, und dann auf diese reagieren würde.

Es scheint merkwürdig, dass Sie mehr am Verhältnis der DecimalDarstellungen von costInund costOutals am Verhältnis der tatsächlichen Werte von costInund interessiert sind costOut, es sei denn, der Code wird die Dezimaldarstellungen auch für einen anderen Zweck verwenden. Wenn der Code diese Repräsentationen weiter nutzt, wäre dies ein weiteres Argument für die Erstellung von Variablen, die diese Repräsentationen enthalten, anstatt eine fortlaufende Folge von Casts im gesamten Code zu haben.

Ich schaue auf diesen Code und frage: "Wie können Kosten 0 (oder weniger) sein?". Welchen Sonderfall deutet das an? Code sollte sein

bool BothCostsAreValidProducts = (CostIn > 0) && (CostOut > 0);
if (!BothCostsAreValidProducts)
  return NO_PROFIT;
else {
   // that calculation here...
}

Ich vermute, was die Namen hier angeht: ändern BothCostsAreValidProductsund NO_PROFITentsprechend.

Kürze hört auf, eine Tugend zu sein, wenn wir vergessen, dass sie eher Mittel zum Zweck als eine Tugend an sich ist. Wir mögen Kürze, weil sie mit Einfachheit korreliert, und wir mögen Einfachheit, weil einfacher Code einfacher zu verstehen und zu ändern ist und weniger Fehler enthält. Am Ende möchten wir, dass der Code dieses Ziel erreicht:

1. Erfüllen Sie die Geschäftsanforderungen mit dem geringsten Arbeitsaufwand

2. Vermeiden Sie Fehler

3. Lassen Sie uns in Zukunft Änderungen vornehmen, die weiterhin 1 und 2 erfüllen

Das sind die Ziele. Jedes Konstruktionsprinzip oder jede Konstruktionsmethode (KISS, YAGNI, TDD, SOLID, Proofs, Typensysteme, dynamische Metaprogrammierung usw.) ist nur insoweit tugendhaft, als sie uns dabei helfen, diese Ziele zu erreichen.

Die fragliche Linie scheint das Endziel aus den Augen verloren zu haben. Es ist zwar kurz, aber nicht einfach. Es enthält tatsächlich unnötige Redundanz, indem der gleiche Casting-Vorgang mehrmals wiederholt wird. Das Wiederholen von Code erhöht die Komplexität und die Wahrscheinlichkeit von Fehlern. Das Vermischen des Castings mit der tatsächlichen Berechnung macht es auch schwierig, dem Code zu folgen.

Die Linie hat drei Anliegen: Schutzeinrichtungen (Sondergehäuse 0), Typenguss und Berechnung. Jedes Anliegen ist ziemlich einfach, wenn man es isoliert betrachtet, aber da es alle in den gleichen Ausdruck gemischt hat, wird es schwierig, ihm zu folgen.

Es ist nicht klar, warum CostOutnicht das erste Mal gegossen wird, wenn es verwendet CostInwird. Es mag einen guten Grund geben, aber die Absicht ist nicht klar (zumindest nicht ohne Kontext), was bedeutet, dass ein Betreuer vorsichtig wäre, diesen Code zu ändern, weil es möglicherweise einige versteckte Annahmen gibt. Und das ist ein Gräuel für die Wartbarkeit.

Da CostInvor dem Vergleich mit 0 gegossen wird, gehe ich davon aus, dass es sich um einen Gleitkommawert handelt. (Wenn es ein Int wäre, gäbe es keinen Grund zum Besetzen). Wenn CostOutes sich jedoch um einen Gleitkomma handelt, kann der Code einen obskuren Fehler bei der Division durch Null verbergen, da ein Gleitkommawert zwar klein, aber ungleich Null ist, aber in eine Dezimalzahl umgewandelt null (zumindest glaube ich, dass dies möglich ist).

Das Problem ist also nicht die Kürze oder das Fehlen von Kürze, sondern die wiederholte Logik und das Zusammentreffen von Bedenken, die zu schwer zu wartendem Code führen.

Das Einführen von Variablen zur Speicherung der umgesetzten Werte würde wahrscheinlich die Größe des Codes erhöhen, der in der Anzahl der Token gezählt wird, aber die Komplexität verringern, die Bedenken trennen und die Übersichtlichkeit verbessern, was uns näher an das Ziel des Codes bringt, der einfacher zu verstehen und zu verwalten ist.

Kürze ist überhaupt keine Tugend. Lesbarkeit ist DIE Tugend.

Kürze kann ein Werkzeug sein, um die Tugend zu erreichen, oder, wie in Ihrem Beispiel, ein Werkzeug, um genau das Gegenteil zu erreichen. Auf die eine oder andere Weise hat es fast keinen eigenen Wert. Die Regel, dass Code "so kurz wie möglich" sein sollte, kann ebenso gut durch "so obszön wie möglich" ersetzt werden - sie sind alle gleich bedeutungslos und können potenziell schädlich sein, wenn sie keinem größeren Zweck dienen.

Außerdem folgt der von Ihnen veröffentlichte Code nicht einmal der Kürze. Wären die Konstanten mit M Suffix deklariert werden, die meisten der schrecklichen (decimal)Abgüsse werden könnten vermieden werden, da der Compiler fördern würde noch intzu decimal. Ich glaube, dass die Person, die Sie beschreiben, nur die Kürze als Ausrede verwendet. Höchstwahrscheinlich nicht absichtlich, aber dennoch.

In meiner jahrelangen Erfahrung bin ich zu der Überzeugung gelangt , dass die ultimative Kürze die Zeit ist - die Zeit dominiert alles andere. Dies umfasst sowohl die Zeit für die Leistung - wie lange ein Programm für die Ausführung eines Auftrags benötigt - als auch die Zeit für die Wartung - wie lange es dauert, Features hinzuzufügen oder Fehler zu beheben. (Wie Sie diese beiden Faktoren ausbalancieren, hängt davon ab, wie oft der betreffende Code ausgeführt oder verbessert wird. Denken Sie daran, dass die vorzeitige Optimierung immer noch die Wurzel allen Übels ist .) Die Kürze des Codes soll die Kürze beider verbessern. kürzerer Code wird normalerweise schneller ausgeführt und ist in der Regel einfacher zu verstehen und zu warten. Wenn dies auch nicht der Fall ist, handelt es sich um ein Nettonegativ.

In dem hier gezeigten Fall wurde meiner Meinung nach die Kürze des Texts auf Kosten der Lesbarkeit als Kürze der Zeilenzahl interpretiert, was die Wartungszeit verlängern kann. (Je nach Ausführung des Castings kann die Ausführung auch länger dauern. Wenn die obige Zeile jedoch nicht millionenfach ausgeführt wird, ist dies wahrscheinlich kein Problem.) In diesem Fall beeinträchtigen die wiederholten Dezimal-Casts die Lesbarkeit, da dies schwieriger ist Sehen Sie, was die wichtigste Berechnung ist. Ich hätte folgendes geschrieben:

decimal dIn = (decimal)CostIn;
decimal dOut = (decimal)CostOut;
return dIn > 0 && CostOut > 0 ? ((dOut - dIn) / dOut) * 100 : 0;

(Bearbeiten: Dies ist derselbe Code wie bei der anderen Antwort, also los geht's.)

Ich bin ein Fan des ternären Betreibers ? :, also würde ich das belassen.

Wie fast alle obigen Antworten sollte die Lesbarkeit immer Ihr vorrangiges Ziel sein. Ich denke jedoch auch, dass die Formatierung ein effektiverer Weg sein kann, um dies über das Erstellen von Variablen und neuen Zeilen zu erreichen.

return ((decimal)CostIn > 0 && CostOut > 0) ?
       100 * ( (decimal)CostOut - (decimal)CostIn ) / (decimal)CostOut:
       0;

Ich stimme dem Argument der zyklomatischen Komplexität in den meisten Fällen sehr zu, aber Ihre Funktion scheint klein und einfach genug zu sein, um mit einem guten Testfall besser angesprochen zu werden. Aus Neugierde, warum das Bedürfnis nach Dezimalstellen?

Für mich sieht es so aus, als liege ein großes Problem mit der Lesbarkeit in der fehlenden Formatierung.

Ich würde es so schreiben:

return (decimal)CostIn > 0 && CostOut > 0 
            ? (((decimal)CostOut - (decimal)CostIn) / (decimal)CostOut) * 100 
            : 0;

Je nachdem, ob es sich bei dem Typ CostInund CostOutum einen Gleitkomma- oder einen ganzzahligen Typ handelt, sind einige der Darstellungen möglicherweise auch nicht erforderlich. Im Gegensatz zu floatund doublewird implizit für ganzzahlige Werte geworben decimal.

Code kann in Eile geschrieben werden, aber der obige Code sollte in meiner Welt mit viel besseren Variablennamen geschrieben werden.

Und wenn ich den Code richtig gelesen habe, wird versucht, eine Bruttomarge-Berechnung durchzuführen.

var totalSales = CostOut;
var totalCost = CostIn;
var profit = (decimal)(CostOut - CostIn);
var grossMargin = 0m; //profit expressed as percentage of totalSales

if(profit > 0)
{
    grossMargin = profit/totalSales*100
}

Ich gehe davon aus, dass CostIn * CostOut ganze Zahlen
sind. So würde ich es schreiben.
M (Money) ist dezimal

return CostIn > 0 && CostOut > 0 ? 100M * (CostOut - CostIn) / CostOut : 0M;

Code ist geschrieben, um von Menschen verstanden zu werden. Die Kürze in diesem Fall kostet nicht viel und erhöht die Belastung des Betreuers. Aus diesem Kürze sollten Sie unbedingt , dass erweitern entweder , indem sie den Code selbsterklärend (besser Variablennamen) oder das Hinzufügen von weiteren Kommentaren zu erklären , warum es so funktioniert.

Wenn Sie heute Code schreiben, um ein Problem zu lösen, kann dieser Code morgen ein Problem sein, wenn sich die Anforderungen ändern. Die Wartung muss immer berücksichtigt werden, und eine bessere Verständlichkeit des Codes ist unerlässlich.

Kürze ist keine Tugend mehr, wenn

• Es gibt eine Division ohne vorherige Überprüfung auf Null.
• Es erfolgt keine Überprüfung auf Null.
• Es findet keine Bereinigung statt.
• TRY CATCH versus wirft die Nahrungskette auf, wo der Fehler behoben werden kann.
• Es werden Annahmen über die Erledigungsreihenfolge von asynchronen Aufgaben getroffen
• Aufgaben mit Verzögerung statt späterer Terminverschiebung
• Unnötige E / A wird verwendet
• Kein optimistisches Update

Wenn dies die Validierungseinheitentests besteht, ist es in Ordnung, wenn eine neue Spezifikation hinzugefügt, ein neuer Test oder eine erweiterte Implementierung erforderlich ist und die Kürze des Codes "entwirrt" werden muss Problem würde entstehen.

Offensichtlich zeigt ein Fehler im Code, dass es ein weiteres Problem mit der Frage / Antwort gab, bei dem es sich um ein Versehen handelte. Die Tatsache, dass ein Fehler aufgetreten ist, gibt also Anlass zur Sorge.

Bei nichtfunktionalen Anforderungen wie "Lesbarkeit" von Code muss dieser vom Entwicklungsmanager definiert und vom Hauptentwickler verwaltet und von den Entwicklern respektiert werden, um eine ordnungsgemäße Implementierung sicherzustellen.

Versuchen Sie, eine standardisierte Implementierung von Code (Standards und Konventionen) sicherzustellen, um die "Lesbarkeit" und die einfache "Wartbarkeit" zu gewährleisten. Wenn diese Qualitätsattribute jedoch nicht definiert und erzwungen werden, erhalten Sie Code wie im obigen Beispiel.

Wenn Sie diese Art von Code nicht sehen möchten, versuchen Sie, das Team dazu zu bringen, sich auf Implementierungsstandards und Konventionen zu einigen, diese aufzuschreiben und zufällige Codeüberprüfungen oder Stichproben durchzuführen, um zu überprüfen, ob die Konvention eingehalten wird.