Null in Collections - Welches Vorgehen bevorzugt ihr?

[Gast]

Methode A:
Auf keinen Fall. Damit ignoriert man nur frühere Fehler. Das geht einfach garnicht.
Mein Kollege, der ansonsten echt nicht so schlecht ist, hatte kürzlich eine Collection voller Nullreferenzen und sie auf höheren Schichten der Software ständig abgefragt, ob ein Item == null ist. Dafür hätte ich ihn am liebsten aus dem Fenster geworfen. Es darf nicht sein, dass null-Items in eine Collection gelangen aus Gründen mangelhafter Programmierung in anderen Teilen der Software.
Nachtrag: Siehe LINQ: Keine Nullreferenzen, weder die Collection an sich noch die Items. So muss es sein.

Methode B:
Evtl. die Collection nicht ungeschützt nach außen geben und eine ReadOnlyCollection in Betracht ziehen. Ansonsten die Methode der Wahl. Wer eine Null-Referenz einfügt, soll direkt in die Welt der Schmerzen verfrachtet werden und seine Bugs bei den frühstmöglichen Testläufen zu spüren bekommen.

Methode C:
Für Projekte an denen man alleine arbeitet und weiß was man tut.
Nachtrag: Evtl. auch ok für Teamprojekte, wenn man davon ausgehen kann, dass der Code getestet wird. Wenn es Code im Kern der Software ist der bei so gut wie jedem Start durchgearbeitet wird, macht sich ein entsprechender Fehler bemerkbar und man kann auf die Ursache schließen.

[Firefighter]

Na Methode C finde ich geht auch gar nicht. Nur ueber die Dokumentation die Fehlbenutzung zu regeln geht in keinen Fall. Andere Moeglichkeit waere die Liste nach aussen gar nicht zu propagieren sondern ein passendes Interface anzubieten was die Add-Operationen direkt ueberwacht und mit einer ArgumentNullException quitiert sobald jemand auf die Idee kommt null-Items einzufuegen.

[Gast]

Firefighter schrieb:

Andere Moeglichkeit waere die Liste nach aussen gar nicht zu propagieren sondern ein passendes Interface anzubieten was die Add-Operationen direkt ueberwacht und mit einer ArgumentNullException quitiert sobald jemand auf die Idee kommt null-Items einzufuegen.

Imho eine der Schwächen von C#. Man kann Properties mit private set gegen Austausch schützen, nicht aber gegen Änderungen von außen. Da wären wir wieder bei der ReadOnlyCollection, aber wirklich oft habe ich die noch nicht gesehen.

[Dravere]

@μ,
Da du bereits die ReadOnlyCollection erwähnst, wärst du somit gar nicht dafür zu haben, dass man eine Collection per Setter setzen könnte?

Wäre somit sowas eher auszuschliessen?

class ItemManager
{
  private IList<Item> m_items;

  public IList<Item> ItemCollection
  {
    get { return m_items; }
    set
    {
      if(value == null) throw new ArgumentNullException("value");
      m_items = value;
    }
  }

  public ItemManager()
  {
    m_items = new List<Item>();
  }

  public void DoSomething()
  {
    foreach(var item in m_items)
    {
      if(item == null)
      {
        continue;
      }

      // do something with item
    }
  }
}

Dadurch wären natürlich nur noch Methode A und C möglich. Ausser man würde sowas machen:

class ItemManager
{
  private NullFreeCollection<Item> m_items;

  public NullFreeCollection<Item> ItemCollection
  {
    get { return m_items; }
    set
    {
      if(value == null) throw new ArgumentNullException("value");
      m_items = value;
    }
  }

  public ItemManager()
  {
    m_items = new NullFreeCollection<Item>();
  }

  public void DoSomething()
  {
    foreach(var item in m_items)
    {
      // do something with item
    }
  }
}

Der Setter für Collection gefällt mir grundsätzlich, weil er sich schön einreiht mit anderen Properties und die Collection damit kein Spezialfall darstellt, wo man normalerweise keinen Setter hat. Auf der anderen Seite gibt man doch einiges an Kontrolle ab oder müsste die Collection zuerst kopieren oder neu verpacken. Die Kapselung scheint stark darunter zu leiden.

Es wurde nun von Firefighter bereits angesprochen, dass man die Liste nach aussen gar nicht propagieren könnte. Das mag in gewissen Fällen Sinn ergeben, aber in anderen wäre es vielleicht weniger praktisch.

Hier mal ein weiteres Beispiel:

class Payment
{
  private readonly decimal m_total;
  private readonly DateTimeOffset m_payDate;

  public string Total { get { return m_total; } }
  public DateTimeOffset { get { return m_payDate; } }

  public PhoneNumber(decimal total, DateTimeOffset payDate)
  {
    m_total = total;
    m_payDate = payDate;
  }
}

class RentableObject
{
  private IList<Payment> m_payments;

  public IList<Payment> Payments
  {
    get { return m_payments; }
    set
    {
      if(value == null) throw new ArgumentNullException("value");
      m_payments = value;
    }
  }

  public RentableObject()
  {
    m_payments = new List<Payments>();
  }
}

In diesem Beispiel haben wir grundsätzlich eine äusserst lose Kopplung zwischen RentableObject und Payment . Wenn wir hier irgendwelche Algorithmen auf die Zahlungen anwenden, wird das wohl kaum in der RentableObject -Klasse passieren. Es macht hier aber wenig Sinn Null-Werte in die Liste einzufügen. Aber sollten wir das überhaupt kontrollieren? Wenn wir es nicht tun, können wir es aber auch nicht in der Schnittstelle garantieren und dadurch müsste jeder andere dies prüfen. Und es wird sicher jemand mindestens das Total berechnen wollen und/oder eine Ausgabe aller Zahlungen machen.

Ich finde es wahnsinnig praktisch, dass jeder seine eigene Liste dem RentableObject übergeben kann. Weil grundsätzlich transportiert RentableObject diese Liste nur. Die Klasse fängt damit nichts an. Auf der anderen Seite kann man nicht garantieren, dass es keine Null-Werte darin geben wird, wodurch man überall wieder zusätzliche redundante Prüfungen einführen muss.

Wenn ich also nebst RentableObject auch noch Algorithmen zur Verfügung stelle, welche auf den Zahlungen eines Mietobjecktes laufen und z.B. Superpunkte ausrechnen in Abhängigkeit der Zahlungshöhe und des Zeitpunktes, dann müsste ich in diesen Algorithmen ständig prüfen, ob es Null-Werte in der Collection hat:

class SuperPointCalculator
{
  public static decimal CalculateFor(RentableObject rentableObject)
  {
    decimal total = 0m;
    foreach(var payment in rentableObject.Payments)
    {
      if(payment == null) continue;

      if(payment.PayDate.Year == 2013)
      {
        total += payment.Total;
      }
      else
      {
        total += payment.Total * 0.5m;
      }
    }

    return total;
  }
}

Ich weiss einfach nicht, was die beste Lösung ist.

Grüssli

[Firefighter]

@Dravere: Dein Beispiel mit den Payments ist ziemlich gut. Jedoch finde ich, dass wenn du schon sagst die Klasse selbst faengt mit den Payments nichts, wozu soll ich dann die Liste nach aussen verfuegbar machen? Solange sie nur fuer interne Berechnungen bereit stehen soll kann ich doch auch genauso gut die Methoden die diese Berechnungen mach, anbieten oder etwa nicht? Man koennte das ja sogar mit deiner NullFreeCollection kombinieren. Du uebergibst eine NullFreeCollection und die Klasse stellt alle Berechnungen als Methoden bereit. Moechtest du die Berechnungen noch variieren, so kannst du mit Dependency Inversion und Interface oder sogar Functoren das ganze flexibel gestalten.

[Dravere]

Kannst du das etwas näher erläutern? Vielleicht ebenfalls mit etwas Pseudo-Code? Ich bin mir nicht ganz sicher, ob ich dich richtig verstehe.

Grüssli

[Firefighter]

Naja du hast ja gemeint das die RentableObject eh nichts intern mit der Collection anstellt. Hier kann man jetzt natuerlich argumentieren das es dann kein Problem ist sie nach aussen hin freizumachen, aber genauso gibt es keine Argumente warum man das nicht machen sollte
Heisst also, ich verstecke die Collection einfach:

class RentableObject
{
  private IList<Payment> m_payments;

  public RentableObject(NullFreeCollection<Payment> payments)
  {
    m_payments = payments;
  }
}

Jetzt zusaetzlich biete ich saemtliche Methoden an die benoetigt werden um mit den Payments zu interagieren. Als Beispiel, Hinzufuegen und Loeschen.

Jetzt nach einer Nacht ist mir aber eingefallen das die zweite Aussage ueber DPI hier weniger schoen anwendbar ist. Also sieh das bitte als nichtig an

[Dravere]

Dadurch ist kein Linq möglich, ausser RentableObject würde IEnumerable<Payment> implementieren. Was eher seltsam wäre. Erst recht wenn RentableObejct noch andere Collections hat. Erscheint mir daher keine gute Idee zu sein. Wieso die Collection so stark kapseln, wenn sie gar nicht so stark angebunden ist?

Grüssli

[Firefighter]

Auch wieder wahr. Ich muss da in meinem Suff wohl etwas wirres geschrieben haben

[Gast]

Entschuldige die späte Antwort.

Dravere schrieb:

@μ,
Da du bereits die ReadOnlyCollection erwähnst, wärst du somit gar nicht dafür zu haben, dass man eine Collection per Setter setzen könnte?

Naja bei Fragen wie den Deinen gibt es keine absoluten Antworten, was Du natürlich weißt. Es kommt auf das Projekt und die Teamgröße an.
Trotzdem tendiere ich dazu, dass man Collections eines Objekts nicht von außen austauschen sollte. Wenn man sich außerhalb eine Referenz auf die Collection speichert ist es einfacher, wenn dieses Objekt niemals ausgetauscht wird.

Dravere schrieb:

Ich finde es wahnsinnig praktisch, dass jeder seine eigene Liste dem RentableObject übergeben kann. Weil grundsätzlich transportiert RentableObject diese Liste nur. Die Klasse fängt damit nichts an. Auf der anderen Seite kann man nicht garantieren, dass es keine Null-Werte darin geben wird, wodurch man überall wieder zusätzliche redundante Prüfungen einführen muss.

Die Liste per Konstruktor übergeben und danach nicht mehr austauschbar machen?
Prüfungen auf Nullwerte auf höheren Schichten der Software sind einfach ein Nogo wie ich finde. Das lässt sich aber nur vermeiden, wenn nicht jeder seinen Müll in die interne Collection eines Objekts einfügen kann. Die Kernfrage ist doch, wie es überhaupt zu Nullreferenzen in einer Collection kommen kann. Ist das nicht ein ganz klarer Bug an anderen Stellen?
Vielleicht geht man während der Entwicklungszeit mit Asserts darauf ein, aber in einem Produkt das ausgeliefert werden kann will ich die Gewissheit, dass keine Nullreferenzen in einer Collection sind, wenn das nicht per Design beabsichtigt wurde. Und das konnten wir bisher gut damit erreichen, indem wir solche Fälle in Exceptions laufen gelassen haben und es immer in Tests festgestellt haben.

Dravere schrieb:

(...) dann müsste ich in diesen Algorithmen ständig prüfen, ob es Null-Werte in der Collection hat:

Eben nicht. Lass die Software abrauchen. Wenn solche Dinge nicht auffallen während der Entwicklung, sollte man imho an der Qualitätsprüfung ansetzen. Es gibt Projekte in denen Unit-Tests nicht so gut funktionieren, dann geht man eben den klassischen weg und kompiliert und startet die Software (oder Teile davon in kleinen Testprojekten) und schaut was passiert.

Also. Bei sowas hier:

foreach(...)
if(payment == null) continue;

würde ich mich direkt auf Fehlersuche begeben und meine Kollegen ansprechen, warum diese Prüfung überhaupt notwendig ist. Auf die Gefahr hin mich zu wiederholen: Eine andere Stelle der Software ist verbugt und das ist reine Symptonbekämpfung.