Magic Values

7

Letzte Woche hab ich ja schon etwas über Code Smells geschrieben. Einen weit verbreiteten Code Smell, nämlich die Magic Values, habe ich letztens mal wieder angetroffen und so nehme ich das mal zum Anlass, darüber etwas zu schreiben.

Der Begriff „Magic Value“ oder „Magic Number“ hat mehrere Bedeutungen. Hier geht es um den Code Smell der Magic Values, also insbesondere nicht um besondere Werte (d.h. Bytefolgen) als Erkennungszeichen in Dateiheadern o.ä.

Es geht also um Code, den man häufig vorfindet, der aber verbesserungswürdig ist. Hierzu sehen wir uns ein paar Beispiele an und werden unterschiedliche Arten von Magic Values ausmachen können:

Magic Values 1. Art: Unveränderliche Werte

1
... days * 1440 ...

Warum nun days mit 1440 multipliziert wird, wird aus dem Code nicht klar.

Besser:

1
2
3
4
5
6
7
// mit einer Funktion ist es klarer
function daysToMinutes(ADays: Integer): Integer;
begin
  return ADays * 1440;
end function;

... daysToMinutes(days) ...

Aber auch das geht besser:

1
2
3
4
5
6
7
8
function daysToMinutes(ADays: Integer): Integer;
const
  MINUTES_PER_DAY = 1440; // = 24 * 60
begin
  return ADays * MINUTES_PER_DAY;
end function;

... daysToMinutes(days) ...

Darüber ob man nun

1
2
3
4
5
6
7
  MINUTES_PER_DAY = 1440; // = 24 * 60
// oder
  MINUTES_PER_DAY = 24 * 60;
// oder
  HOURS_PER_DAY = 24;
  MINUTES_PER_HOUR = 60;
  MINUTES_PER_DAY = HOURS_PER_DAY * MINUTES_PER_HOUR;

schreiben sollte, kann man streiten.

Magic Values 2. Art: Potenziell veränderliche Werte

Ein weiteres Beispiel:
In einem Code, der mit Spielkarten hantiert (ein Skat-Programm oder was weiß ich), könnte folgender Code stehen:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
var
  cards: array[0..31] of Karte;
...

// BubbleSort:
for i := 0 to 30 do
begin
  for j := i to 30 do
  begin
    if j > j+1 then
    begin
      // vertauschen:
      tmp := cards[j];
      cards[j] := cards[j+1];
      cards[j+1] := tmp;
    end if;
  end for;
end for;

Wo die 30 her kommt, ist aus dem Code wieder mal nicht ersichtlich. Und es gibt noch ein weiteres Problem mit diesem Code: Die Anzahl der Minuten pro Tag (obiges Beispiel) wird sich in absehbarer Zeit nicht ändern. Hier ist es jedoch anders. Vielleicht wollen wir auch mal größere Kartenstapel sortieren können. Jetzt sind es zwar 32, aber, wenn wir den gleichen Code in einem Poker-Programm verwenden wollen, müssen wir auf einmal mit 52 Karten zurecht kommen. In diesem Fall müssten wir unseren Code an mehreren Stellen ändern, was aufwändig und fehleranfällig ist.

Besser also, man definiert eine Konstante für die Anzahl der Karten. Bei Arrays kann man außerdem auch ganz ohne feste Zahlen und Konstanten oft mit der Größe des Arrays selbst arbeiten. Genau genommen ist für die obige BubbleSort-Implementierung ja die Größe des Arrays relevant und nicht die Anzahl der Karten (auch, wenn das in diesem Fall das selbe ist).

Besser geht es so:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
const
  NUMBER_OF_CARDS = 32;
var
  cards: array[0..NUMBER_OF_CARDS -1] of Card;
...

// BubbleSort:
for i := 0 to high(cards) -1 do
begin
  for j := i to high(cards) -1 do
  begin
    if j > j +1 then
    begin
      // vertauschen:
      tmp := cards[j];
      cards[j] := cards[j+1];
      cards[j+1] := tmp;
    end if;
  end for;
end for;

high() ermittelt den höchsten Index in einem Array. Steht diese Funktion nicht zur Verfügung, kann man den Ausdruck auch mit length() beschrieben: length(cards) -2

In einem konkreten Kontext müsste man außerdem überlegen, ob es sich vielleicht lohnt, statt der Konstanten eine Variable einzuführen. So könnte man hier also zur Laufzeit entscheiden, wie viele Karten auf dem Stapel liegen. Auch, wenn das jetzt vielleicht noch nicht gebraucht wird, könnte es später einmal gebraucht werden und dann ist es gut, schon so eine Variable zu haben. Man muss sie ja noch nicht öffentlich machen.

Bei Strings ist es ganz ähnlich. Hier ist die Lesbarkeit weniger das Problem, sondern vielmehr die Änderbarkeit. Haben wir einen String mehrmals im Code verwendet, müssen wir diesen ggf. mehrmals ändern. Legen wir stattdessen eine Konstante an, haben wir kein Problem damit.

Ganz ähnlich ist es mit Werten, die auf unterschiedlichen System prinzipiell unterschiedlich sein können. Ein Beispiel hierfür wäre Host, Port, Benutzername und Passwort für eine Datenbank. So etwas sollte gar nicht hartkodiert im Code stehen (also auch nicht in Konstanten), sondern aus einer Konfigurationsdatei ausgelesen werden.

Magic Values 3. Art: Gar keine Werte

In den bisherigen Beispielen waren die Integers immer wirkliche Integers und alle Strings wirkliche Strings. Das heißt alle Zahlen stellten wirklich Zahlen dar und standen nicht stellvertretend für etwas anderes. Und die Strings repräsentierten wirklich Zeichenketten. Manchmal findet man aber Code, bei dem das nicht so ist:

Der folgende Code dehnt ein Rechteck nach oben, rechts, unten oder links aus. ADrirection gibt dabei die Richtung an und APixels die Anzahl Pixel um die vergrößert werden soll. Es wird angenommen, dass sich der Nullpunkt, wie üblich bei GUIs, in der linken oberen Ecke befindet und x-Werte nach rechts, sowie y-Werte nach unten größer werden.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
method extend(ADirection: Integer; APixels: Integer);
begin
  switch(ADirection)
  begin
    case 0:
      self.top -= APixels;
      self.height += APixels;
      break;
    case 1:
      self.width += APixels;
      break;
    case 2:
      self.height += APixels;
      break;
    case 3:
      self.left -= APixels;
      self.width += APixels;
      break;
  end switch;
end method;

Der eigentliche Zahlenwert von ADirection ist hier ziemlich irrelevant. Bei 0 wird nach oben ausgedehnt, bei 1 nach rechts, bei 2 nach unten bei bei 3 nach links. Für den Compiler ist das vollkommen in Ordnung, aber der Code hat mehrere Schwachstellen:

  • Der Code ist nicht sonderlich gut lesbar. Man könnte jetzt sagen, es fehlten Kommentare, und in der Tat würden Kommentare hier einiges verbessern. Guter Code dokumentiert sich aber weitgehend selbst. Hier sind eigentlich keine Kommentare notwendig um den Code lesbar zu machen.
  • Auch die Verwendung der Methode ist nicht gut lesbar. Wer wissen möchte, was extend(3, 4); macht, muss sich die Methode erstmal ansehen.
  • Man kann sogar leicht die Parameter vertauschen. Der Compiler merkt nichts davon.
  • Was, wenn ungültige Werte angegeben werden? z.B. 4 oder 5 oder -1? Hier könnte man einwenden, dass man das ja abprüfen kann. Entweder mit Wächtern, die eine Exception werfen oder mit Assertions. Dabei würde aber einerseits immer noch keine Prüfung zur Compiletime stattfinden und andererseits würde der Code dadurch nur *noch* komplizierter.

Der Code ist also schwer zu lesen, schwer zu verwenden und verleitet dazu, Bugs zu produzieren. Schlecht. Auch nicht besser ist folgende Variante:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
method extend(ADirection: string; APixels: Integer);
begin
  switch(ADirection)
  begin
    case "Top":
      self.top -= APixels;
      self.height += APixels;
      break;
    case "Right":
      self.width += APixels;
      break;
    case "Bottom":
      self.height += APixels;
      break;
    case "Left":
      self.left -= APixels;
      self.width += APixels;
      break;
  end switch;
end method;

Jetzt ist der Code zwar besser lesbar und auch das Vertauschen der Parameter ist nicht mehr möglich. Dafür gibt es ganz tolle Möglichkeiten für das Erzeugen von schwer zu findenden Bugs:

1
2
3
4
5
6
7
extend("Botom");
extend("top");
extend("Rîght");
extend("Left");
extend("Right ");
extend("Letf");
extend("Links");

Von diesen sieben Aufrufen ist genau einer korrekt. Alle anderen führen unweigerlich zu einem Fehler (bzw. unerwünschtem Verhalten). Solche Fehler sind aber sehr schnell passiert, dafür aber u.U. nur schwer zu finden. Gaaanz schlecht.

Durch Verwendung von Aufzählungstypen (Enums) kann man die meisten der genannten Probleme recht einfach lösen:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
enum Direction = (Top, Right, Bottom, Left);

method extend(ADirection: Direction; APixels: Integer);
begin
  switch(ADirection)
  begin
    case Direction.Top:
      self.top -= APixels;
      self.height += APixels;
      break;
    case Direction.Right:
      self.width += APixels;
      break;
    case Direction.Bottom:
      self.height += APixels;
      break;
    case Direction.Left:
      self.left -= APixels;
      self.width += APixels;
      break;
    default: assert(false);
  end switch;
end method;

Der Code ist lesbarer geworden (ähnlich wie bei dem String-Beispiel), jedoch wird uns der Compiler sofort benachrichtigen, wenn wir Botom, statt Bottom schreiben. Das assert(false); im default-Zweig hilft uns für den Fall, dass Direction mal erweitert wird (es gibt ja zwei weitere Richtungen in der dritten Dimension).

Das ist schon deutlich besser. In diesem Fall würde ich aber ganz einfach mehrere Methoden nehmen:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
method extendAtTop(APixels: Integer);
begin
  self.top -= APixels;
  self.height += APixels;
end method;

method extendAtRight(APixels: Integer);
begin
  self.width += APixels;
end method;

method extendAtBottom(APixels: Integer);
begin
  self.height += APixels;
end method;

method extendAtLeft(APixels: Integer);
begin
  self.left -= APixels;
  self.width += APixels;
end method;

So ist der Code kaum länger, aber auf trivialste Art zu verstehen. Kommentare sind nicht nötig, man kann den Code kaum falsch benutzen, Tippfehler bemerkt der Compiler sofort und es gibt auch keine unzulässigen Werte, die man abprüfen müsste [1]. Hier merkt man mal wieder: Guter Code ist leicht richtig zu verwenden und nur schwer falsch. Schlechter Code ist leicht falsch zu verwenden und deshalb Brutstätte von Bugs lustigster Art.

Häufig trifft man diese verschärfte Art von Magic Values auch bei der Realisierung von Zustandsautomaten an. In so einem Fall gibt es neben der Möglichkeit Enums zu verwenden auch noch das State Pattern. In Java gibt es sogar die Möglichkeit Enum und State Pattern zu verbinden, da hier Enums Methoden haben können. Das ist aber Stoff für mindestens eins-zwei weitere Blog-Artikel.

Daneben sei noch erwähnt, dass man generell Enums oft durch abstrakte Kopplungen ersetzen kann. Also nicht nur bei Zustandsautomaten. Siehe hierzu: If-Statement Considered Harmful.

Magic Values 4. Art: Manchmal Werte (Hybridkopplung)

Eine weitere Variante der Magic Values sind die so genannten Hybridkopplungen. Hierbei handelt es sich um Parameter, die sowohl Daten (also wirkliche Werte) als auch Kontrollflussinformation (wie bei Magic Values 3. Art) beinhalten. Man findet so etwas recht häufig und manchmal kann es ja auch tatsächlich die beste Möglichkeit sein, den Smell zu ignorieren. Oft sind diese Fälle aber auch vermeidbar und entstehen aus reinem Unwissen. Auch hierzu wieder ein typisches Beispiel:

1
2
3
4
5
6
7
8
9
10
11
// Set intervall to 0 to disable
method setIntervall(AMilliSeconds: Integer);
begin
  if AMilliSeconds = 0 then
  begin
    Timer.Active := false;
  else
    Timer.Intervall := AMilliSeconds;
    Timer.Active := true;
  end if;
end;

Die Klasse verwendet intern wohl irgendeinen Timer, beispielsweise um Werte periodisch neu zu berechnen. Kleine Werte bedeuten ein schnelles Aktualisieren, große Werte ein langsames. Die 0 wird aber separat behandelt und deaktiviert die Aktualisierung. Der Parameter AMilliSeconds beinhaltet also gewissermaßen zwei Informationen: Das Intervall selbst und ob die Aktualisierung nun eingestellt werden soll oder nicht.

Das führt zu folgenden Problemen:

  • Schlechter lesbar
  • Man muss u.U. an diversen Stellen diesen Sonderfall berücksichtigen. Wenn man das nicht tut, hat man einen schwer zu findenden Bug.
  • Der Code ist nicht intuitiv benutzbar. Jemand der eigentlich die Funktionalität deaktivieren will, wird zuerst mal nach einer disable()-Methode suchen. Diese gibt es aber nicht.

Besser also man erlaubt den Wert 0 nicht und führt eine weitere Methode disable() (oder eine Property Active, o.ä.) ein. So gibt es diese Probleme gar nicht erst.

Daumenregeln

Fehlen jetzt noch die Prinzipien, gegen die Magic Values verstoßen, also das, was dafür verantwortlich ist, dass man sie „riecht“. Das kommt ein bisschen auf die konkrete Art von Magic Value an.

Generell könnte man erstmal folgende Daumenregel aufstellen:

Regel

In beliebigen Ausdrücken sollten neben den Werten 0, 1, einzelnen Chars und dem Leerstring nur Variablen und Konstanten verwendet werden.

Wie bei allen Regeln gibt es hier natürlich auch Ausnahmen. So wird eine Methode even(), die prüfen soll, ob eine Zahl gerade ist, durchaus das Literal 2 enthalten und nicht eine Konstante ZWEI. Letzteres wäre auch ziemlich sinnfrei. In diesem Fall geht es auch wirklich ganz konkret um die Zahl 2 und nicht um einen Wert, der zufällig mal eben 2 ist. Ähnlich ist es mit length(cards) -2 im obigen Beispiel. Konstanten, die so heißen wie der Wert, den sie repräsentieren, haben keinen sonderlichen Nutzen. Schafft man es hingegen, einem Wert einen abstrakten Namen zu geben, der unabhängig vom konkreten Wert ist, ist das ein ziemlich sicheres Zeichen, dass man daraus eine Konstante (oder Variable) machen sollte.

Ganz unabsichtlich habe ich somit schon eine der Daumenregeln, über die ja erst noch bloggen wollte, abgehandelt. Für diese Regel kenne ich keinen Namen. Es gibt ne Menge Leute, die genau diese Auffassung vertreten, aber ich weiß nicht, ob das mal jemand als Regel formuliert hat. Damit man drüber reden kann, würde ich einfach mal 0-1-„“-Regel („Null-Eins-Leerstring-Regel“) vorschlagen.

Wie gesagt: Unterschiedliche Arten von Magic Values verstoßen gegen unterschiedliche Prinzipien.

Bei Magic Values 1. Art:

  • Schreibe Code für Menschen, nicht für Compiler
  • 0-1-„“-Regel

Bei Magic Values 2. Art:

  • Schreibe Code für Menschen, nicht für Compiler
  • 0-1-„“-Regel
  • Don’t Repeat Yourself (DRY)

Bei Magic Values 3. Art:

  • Schreibe Code für Menschen, nicht für Compiler
  • (0-1-„“-Regel)
  • Don’t write code to find code

Bei Magic Values 4. Art:

  • Schreibe Code für Menschen, nicht für Compiler
  • Don’t write code to find code
  • Princliple of Least Surprise

Ich hoffe, ich komme bald mal dazu, zumindest eine vorläufige Liste der Prinzipien zu bloggen, über die ich noch schreiben will. Das sind ne ganze Menge. Eins ist aber schon mal sicher: Dieses Jahr fange ich nicht mehr damit an… 😉

[1] OK, hier hab ich gemogelt. Negative Werte müsste man behandeln. Zumindest width und height sollten nicht negativ werden dürfen. in einem realen Kontext müsste man das beachten, aber, für das Beispiel hier reicht es so.

7 Kommentare



  2. Hallo Christian!

    Freu mich immer wieder über neue Artikel die man anderen zeigen kann, wenn sie gerade dabei sind, in die Welt der Programmierer einzutauchen. Das hier ist einer davon 🙂

    Weiter so!


  3. Danke. 🙂

    Und auch Danke dafür, dass du mich dazu gebracht hast, mir mein eigenes Geschreibsel nochmal anzugucken. Da könnt ich vielleicht noch das ein- oder andere für meinen Vortrag auf den Delphi-Tagen gebrauchen… Ich hoffe, ich krieg das alles in der einen Stunde unter und muss nicht zu viel streichen…


  4. Bezüglich den Delphi-Tagen: Ich kann leider nicht kommen (hatte es mir eigentlich fest vorgenommen) da ich zu der Zeit nen Job habe, der mich leider auch samstags beschäftigt. Wirst du die Folien wie letztes Jahr wieder zum Download anbieten? Deine letztjährigen OOP-Folien wurden schon vom einen oder anderen Programmier-Jüngling als sehr übersichtlich und verständlich gelobt. Würde auch die Enbugging-Folien gerne weiterempfehlen.


  6. Freut mich 🙂


Schreibe einen Kommentar

Deine E-Mail-Adresse wird nicht veröffentlicht. Erforderliche Felder sind mit * markiert