Wann ist ein Parameter Check sinnvoll

[perlenfischer1984]

Hallo ich habe mal eine generelle Frage :

public class Competition {

    public Competition(String competitionName) {
        // check parameter competitionName
      
    }
}

public class DataManager {

   public void createCompetition(String competitionName){

     // check parameter competitionName
     Competition c = new Competition(competitionName);
   }

}

Der DatenManager ist die generelle Datenklasse. Es erzeugt ein Competition mit dem competitionName. Nun meine Frage. In der Funktionen createCompetition und im Constructor wird der Parameter competitionName geprüft und im Fehlerfall eine Exception geworfen (ist hier nicht im Code). Ist dies sinnvoll in jeder Funktion den Parameter zu prüfen oder nur im Construcor.

 

[Flown]

Es kommt immer darauf an. Wird dein Parameter am Anfang oder mitten in der/m Methode/Konstruktor verarbeitet? Wenn er am Anfang abgearbeitet ist und der Sub-call - wie bei dir - den Parameter prüft, dann brauchst du ihn nicht erneut zu prüfen. Doch wenn er nicht am Anfang geprüft wird, dann sollte auf jedenfall gecheckt werden. Das verhindert unnötiges Debuggen! Aber das allerwichtigste ist und bleibt: DOKUMENTATION! Dann sieht das ganze auch so aus.

class Competition {
  
  public Competition(String competitionName) {
    Objects.requireNonNull(competitionName);
    // ...
  }
}

class DataManager {
  /**
   * Creates a new {@link Competition} and ... .
   * 
   * @param competitionName
   *          name of the new {@link Competition}
   * @throws NullPointerException
   *           if {@code competionName} is {@code null}
   */
  public void createCompetition(String competitionName) {
    Competition c = new Competition(competitionName);
    // ...
  }
  
}

 

[kneitzel]

Diese Antwort hat bei mir einiges Nachdenken und Nachforschen ausgelöst. Ich will meine aktuelle Sicht einfach einmal kurz darstellen:

- Ich habe bisher immer einen manuellen Check auf == null durchgeführt und dann eine IllegalArgumentException geworfen. Zu der Frage NPE vs. IAE gibt es einiges zu lesen und ich denke, ich werde zukünftig auch zu der NPE tendieren. Auch der Aufruf mit Objects.requireNonNull war mir bisher unbekannt und ist eine gute Sache, die ich zukünftig dann auch nutzen werde.

- Ich selbst prüfe generell die Argumente. In jeder Funktion. Egal ob "public API" oder "internal API" oder was in Argumentationen gerne angebracht wird.
a) Es ist doch egal, ob jemand anderes die Klassen / Funktionen nutzt oder ich. Wenn da kein null Wert rein darf, dann darf das nicht sein. Wenn da dann irgendwo ehh eine NPE auftritt, dann ist es ein Fall von Glück gehabt. Aber evtl. läuft ja alles durch (dann schafft es ein Bug ggf. bis zum Kunden) oder evtl. werden gar Daten korrumpiert (Worst Case!) Daher: Daten werden immer so gut wie möglich geprüft.
b) Das Argument bezüglich "das Argument wird ja im nächsten Call dann mit geprüft" mag zwar jetzt stimmig sein. Aber wer weiss, was die Zukunft bringt? Dann wird Competition geändert und nimmt plötzlich auch einen null Wert (aber bei der aufrufenden Methode ist das immer noch fatal!) oder es ändert sich sonst irgendwas und plötzlich fehlt dieser check wirklich...

Aber ich bin auch durch .Net geprägt und die Code Analysis Tools, die wir einsetzen, verlangen einen Check der Argumente ansonsten gibt es eine Warnung (Die unsere Build-Tools als Error werten und daher eine Nacharbeitung erfordern).

 

[Flown]

Alles zu prüfen ist nicht immer das gewünschte. Es kommt immer darauf an ob null ein Fehlerfall ist, oder nicht. Im obigen Beispiel ist DataManager#createCompetition null-tolerant, doch der Sub-Call nicht. Wenn in späterer Folge sich das ändert und der Subcall auch null-tolerant wird, dann ändert sich nur die Java-Doc und nicht der Code... und falls man das zu ändern vergisst -> egal. Darum ist es immer use-case abhängig und es sollte nicht immer, alles geprüft werden.

 

[kneitzel]

Also das ist in meinen Augen eben nicht egal. Das Verhalten ist klar beschrieben und bei der Entwicklung von DataManager#createCompetition muss man als Entwickler klar festlegen, was erlaubte Werte sind und was nicht.
Wenn null keinen Sinn macht, dann verbiete ich das. Wenn null Sinn macht, dann habe ich bei der Implementation sicher zu stellen, dass es wirklich funktioniert und habe diesen Fall auch zu testen. Solche Dinge einfach nicht zu testen (so a.la. evtl. ändert sich das Verhalten ja eines Tages) halte ich für fahrlässig, da ich als Entwickler damit das Verhalten meiner Klassen nicht sicher stelle (und auch die Klasse nicht voll teste).
Es so zu schreiben mag ja ok sein. Aber ein Unit-Test, der sicher stellt, dass da dann wirklich eine NPE geworfen wird, würde ich dann aber schon als Minimum erwarten.

Aber das ist eine Ansichtssache. Und mag sein, dass ich da durch die Arbeit im Team hier stark geprägt bin diesbezüglich.

 

[Joose]

Wenn null keinen Sinn macht, dann verbiete ich das. Wenn null Sinn macht, dann habe ich bei der Implementation sicher zu stellen, dass es wirklich funktioniert und habe diesen Fall auch zu testen.

Und das wäre meiner Meinung nach im Konstruktor von Competition (und nur dort).
Wenn die createCompetition Methode von DataManager nun ebenfalls prüft, habe ich 2 Stellen zu ändern! -> es handelt sich dabei (meiner Meinung nach) um duplicate Code

 

[kneitzel]

Ok, das Argument ist gut und gibt Deiner Position einiges mehr an Gewicht, als ich bisher gesehen habe.

 

[Flown]

Ahhh das kam jetzt falsch rüber. Bitte Tests jetzt mal beiseite schieben, das ist ein ganz anderes Thema. Bleiben wir einfach mal wo Parameter geprüft werden sollen, anhand von meinem obigen Beispiel.
Ich erweitere es um ein paar Nuancen, damit man den Sinn eventuell besser versteht.

class Competition {
  private String competitionName;
 
  /**
   * Creates a new {@code Competition} instance.
   *
   * @param competitionName
   *          name of the competition
   * @throws NullPointerException
   *           if {@code competitionName} is {@code null}
   */
  public Competition(String competitionName) {
    this.competitionName = Objects.requireNonNull(competitionName);
  }
 
  // getter/setter
}

class DataManager {
  private List<Competition> competitions = new ArrayList<>();
 
  /**
   * Creates a new {@link Competition} and adds it to the List.
   *
   * @param competitionName
   *          name of the new {@link Competition}
   */
  public void createAndAddCompetition(String competitionName) {
    Competition c = new Competition(competitionName);
    competitions.add(c);
  }
}

Hier in der Methode createAndAddCompetition bin ich null-tolerant, da ich eine Instanz erstelle und es meine Liste hinzufüge. Ich verarbeite den Parameter nicht direkt.
Im Gegensatz zu dem Konstruktor aus Competition. Hier brauch ich diesen Check und der StackTrace sollte genau diese Stelle hier ausgeben, weil genau hier erlaube ich keinen null-Wert.
Nur als Hinweis könnte ich in der Dokumentation schreiben, dass eine NPE fliegen könnte, wenn einen neuen Instanz von Competition erstellt wird.