Schlagwort: POLS

Schiache Pfuscher-Code, im Vergleich zu sauberer Professioneller-Code #1

Für J.E.
Wünsche dir alles Gute und viel Erfolg bei deinem Studium.

Diesen Code gab es tatsächlich, und ist nicht aus der Luft gegriffen.
Schaue dir diesen Code mal gründlich in Ruhe an. Was fällt dir auf?

/// <summary>
/// Gibt einen wert zurück
/// </summary>
/// <param name="name"></param>
/// <returns></returns>
public object GetValue(string name) 
{
    try
    {
        if (_DataSet == null) return null;

        if (_DataSet.Tables.Contains("Settings"))
        {
            return Convert.ChangeType(
                _DataSet.Tables["Settings"].Rows[0][name],
                _DataSet.Tables["Settings"].Columns[name].DataType);
        }
    }
    catch(Exception ex)
    {
        return null;
    }

    return "";
}
  1. Kommentar:
    • Ist nicht auf Englisch
    • Sagt nichts Neues aus, somit sagt nichts aus
  2. Rückgabe-Typ:
    • Unklarer/Vager Rückgabe-Typ (ein Objekt kann alles Mögliche sein: Animal, bool, Gott, Welt, …). Der Aufrufer muss Unboxing betreiben (falls der Wert nicht NULL ist, wie wir sehen werden)
  3. Der Name der Methode:
    • ist weder klar noch eindeutig
    • lässt vermuten, dass keine Ausnahmefehler abgefangen/behandelt werden
    • Der Aufrufer wird selbst, nicht notwendigerweise, Try-Catch-Block herumbauen
  4. NULL-Rückgabe:
    • Was macht der Aufrufer, wenn er für Einstellungsname „enabled“, true oder false erwartet, jedoch einen NULL bekommt?
    • Was machen 100 aufrufende Stellen im Code? 1000?
    • Wie oft muss aufrufender Code, die NULL-Eventualität prüfen, um darauf richtig zu reagieren?
    • Was passiert mit 100 aufrufende Stellen im Code, wenn diese Methode ausgebessert wird?
  5. Mehrfaches Erzeugen und Verwenden von inhaltlich gleichem String:
    • Erzeugen von Strings ist teuer. Warum also dreimal das Gleiche?
    • Was passiert, wenn der DB-Entwickler die Tabelle in „Config“ umbenennt? Wie findet man diese Stellen überall im Code? Wie oft muss man dann der Name aktualisieren?
  6. Rückgabe-Typ: Angenommen „DataType“ = Single oder Double …
    • Der float/double Wert wird geboxt, damit der Aufrufer es wieder unboxt? Boxing/Unboxing ist ein teurer Spaß!
  7. Try-Catch-Block:
    • Wozu? Was soll schiefgehen? Welcher Ausnahmefehler soll da geworfen werden? Eine DB-Tabelle mit Einstellungen taucht (in ein ordentliches Schema und SW-System) nicht einfach auf, und verschwindet einfach so!
  8. Exception-Typ und Variable:
    • Wozu? Was wird mit den Informationen in „ex“ gemacht?
  9. Rückgabe-Typ: wurde schon weiter oben thematisiert
  10. Rückgabe-Typ:
    • Aha! Ach, so ist das! Warum wird auf einmal eine leere Zeichenkette zurückgeliefert? Welcher Aufrufer rechnet damit?
    • Was macht ein Aufrufer, der, zum Beispiel auf die true/false Werte wartet, mit einer leeren Zeichenkette?

So ein Code kann man auch sauber und professionell gestalten

Principle Of Least Surprise : POLS

Sauberer Code weil:

  1. Die Namen sind klar und eindeutig, lassen weder Vermutungen noch Überraschungen zu:
    • Get Value Or Null ⇒ liefert einen Wert oder NULL zurück, schmeißt aber keine Ausnahmefehler. Deswegen müssen an zig oder hunderte Stellen, keine Try-Catch-Blocks geschrieben werden.
    • Get Value ⇒ liefert einen Wert oder schmeißt einen Ausnahmefehler
    • Get Value Or Default (generisch) ⇒ liefert einen Wert oder den Standardwert:
      • bool: false
      • byte/sbyte/short/ushort/int/unit/long/ulong: 0
      • float/double/decimal: 0.0
      • string: null
      • schmeißt keine Ausnahmefehler
    • Get Value Or Default (generisch) ⇒ liefert einen Wert oder den von Aufrufer bestimmter Standardwert, jedoch keine Ausnahmefehler geworfen
    • Get Value (generisch) ⇒ liefert einen Wert oder wirft einen Ausnahmefehler
  2. Es werden Alternativen angeboten (siehe Unterpunkte von Punkt 1!)
  3. Der Rückgabe-Typ ist bei alternative generische Funktionen von Aufrufer festgesetzt/bestimmt und eindeutig
  4. Die Rückgabe-Typen sind alle konsistent: entweder einen Wert oder Ausnahmefehler, Wert oder NULL, Wert oder Standardwert ⇒ keine Überraschungen
  5. Bei Aufruf von generische Varianten sind keine Unboxings auf der Aufrufer-Seite notwendig

Gibt es da unten eine Methode, welche ein Kommentar braucht?

public class CleanCode
{
    public const string TNAME_SETTINGS = @"Settings";
    private DataSet _DataSet;

    // 1. Variante: Objekt als Return-Value. Ist POLS-Konform.
    // Tut das Gleiche wie oben, nur viel einfacher (ohne leere Strings)
    // Die Aufrufer müssen sich um Exceptions und NULL kümmern.
    // Ansonsten, verlässlicher als der Code von oben (trotzdem NICHT GUT!)
    public object GetValue(string pSettingName, Type pType)
    {
        var value = _DataSet.Tables[TNAME_SETTINGS].Rows[0][pSettingName];
        return Convert.ChangeType(value, pType);
    }

    // 2. Variante: Objekt als Return-Value. Ist POLS-Konform.
    public object GetValueOrNull(string pSettingName)
    {
        if (_DataSet == null
            || !_DataSet.Tables.Contains(TNAME_SETTINGS)
            || _DataSet.Tables[TNAME_SETTINGS].Rows.Count == 0
            || !_DataSet.Tables[TNAME_SETTINGS].Columns.Contains(pSettingName))
            return null;

        return _DataSet.Tables[TNAME_SETTINGS].Rows[0][pSettingName];
    }

    // 3. Variante: Generischen Typ. Ist POLS-Konform.
    // Sollte etwas nicht passen oder schiefgehen, erhält der Aufrufer
    // den Standardwert des Typs.
    public T GetValueOrDefault<T>(string pSettingName)
    {
        if (_DataSet == null
            || !_DataSet.Tables.Contains(TNAME_SETTINGS)
            || _DataSet.Tables[TNAME_SETTINGS].Rows.Count == 0
            || !_DataSet.Tables[TNAME_SETTINGS].Columns.Contains(pSettingName))
            return default;

        try
        {
            var value = _DataSet.Tables[TNAME_SETTINGS].Rows[0][pSettingName];
            return (T)Convert.ChangeType(value, typeof(T));
        }
        catch
        {
            return default;
        }
    }

    // 4. Variante: Generischen Typ. Ist POLS-Konform.
    // Die Aufrufer können selbst entscheiden welchen Wert Sie erhalten möchten,
    // falls diese an den Standardwert des Typs nicht sind.
    public T GetValueOrDefault<T>(string pSettingName, T pDefault = default)
    {
        if (_DataSet == null
            || !_DataSet.Tables.Contains(TNAME_SETTINGS)
            || _DataSet.Tables[TNAME_SETTINGS].Rows.Count == 0
            || !_DataSet.Tables[TNAME_SETTINGS].Columns.Contains(pSettingName))
            return pDefault;

        try
        {
            var value = _DataSet.Tables[TNAME_SETTINGS].Rows[0][pSettingName];
            return (T)Convert.ChangeType(value, typeof(T));
        }
        catch
        {
            return pDefault;
        }
    }

    // 5. Variante: Generischer Typ. Ist POLS-Konform.
    public T GetValue<T>(string pSettingName)
    {
        var value = _DataSet.Tables[TNAME_SETTINGS].Rows[0][pSettingName];
        return (T)Convert.ChangeType(value, typeof(T));
    }
}