Frage Befolgt es Java-Konventionen, um nur einen Getter für Sammlungen bereitzustellen? [geschlossen]


Ich habe kürzlich einen Code wie folgt gefunden:

public List<Item> getItems() {
    if (items == null) {
        items = new ArrayList<Item>();
    }
    return this.items;
}

und im Wesentlichen keine Setter-Methode.

Also, wenn Sie zur ArrayList hinzufügen wollten, müssten Sie tun

foo.getItems().add(...)  

eher, als

foo.setItems(myArrayList)

Ich habe dieses Idiom vorher nicht gesehen, und ich kann nicht sagen, dass ich es mag, aber wenn ich Mapping-Code mit mapsstruct.org (großartiges Werkzeug übrigens) erzeugte, behandelt mapstruct es gut und erzeugt korrekt Code, der den Getter verwendet auch als Setter.

Ich frage mich nur - ist das eine allgemeine Redewendung, die ich irgendwie verpasst habe? Es scheint mir sinnlos, aber vielleicht ist da etwas Weisheit dahinter, die ich nicht sehe?


5
2018-04-25 10:46


Ursprung


Antworten:


Es ist üblich, aber es wird nicht empfohlen.

Das Problem liegt nicht in der faulen Initialisierung (dieser Teil ist in Ordnung), sondern in der Darstellung dessen, was ein Implementierungsdetail sein sollte. Sobald Sie das tatsächliche, änderbare (!) Listenobjekt, das Sie in Ihrem Feld speichern, verfügbar machen, kann der Aufrufer des Codes alles mit der Liste tun, auch Dinge, die Sie nicht erwarten.

Sie könnten zum Beispiel Objekte entfernen, wenn wirklich alles erlaubt ist add(). Sie könnten es aus verschiedenen Threads modifizieren, wodurch Ihr Code auf interessante und frustrierende Weise unterbrochen wird. Sie könnten es sogar zu einem rohen werfen List und füllen Sie es mit völlig verschiedenen Arten von Objekten, um Ihren Code zu werfen ClassCastExceptions.

Mit anderen Worten, es macht es unmöglich, Klasseninvarianten zu erzwingen.

Beachten Sie, dass zwei Dinge zusammenarbeiten, um dieses Problem zu verursachen:

  1. Die Belichtung eines in einem Feld gespeicherten Objekts.
  2. Und die Tatsache, dass das Objekt veränderbar ist.

Wenn eines dieser beiden nicht wahr ist, gibt es kein Problem. Also das ist in Ordnung:

public String getFoo() {
   return this.foo;
}

weil String ist unveränderlich. Und das ist auch in Ordnung:

public List<String> getFooList() {
   return new ArrayList<>( this.fooList );
}

Weil Sie jetzt eine defensive Kopie und nicht das eigentliche Objekt zurückgeben. (Wenn jedoch die Elemente der Listen veränderbar wären, würden Sie wieder in Schwierigkeiten geraten.)


Es gibt eine subtilere Variation für dieses Problem ...

Stellen Sie sich dieses Szenario vor:

public class Foo { 
   private List<String> list;
   public Foo( List<String> list ) {
     this.list = list; // Don't do this
   }
   ...
}

Das sieht vollkommen harmlos aus und man sieht es an vielen Stellen. Allerdings gibt es auch hier einen versteckten Haken: Wenn Sie vor dem Speichern der Liste keine Kopie erstellen, befinden Sie sich genau in der gleichen Situation. Du kannst jemanden nicht davon abhalten:

List<String> list = new ArrayList<>();
list.add( "nice item" );
Foo foo = new Foo( list );
list.add( "hahahaha" );
list.add( "i've just added more items to your list and you don't know about it." );
list.add( "i'm an evil genius" );

Sie sollten also defensive Kopien erstellen, bevor Sie einem Feld ein veränderbares Objekt zuweisen und es zurückgeben.


Wenn es so gefährlich ist, veränderliche Felder einer Klasse zu entlarven, warum machen die Leute dann nicht ständig Defensivkopien?

Abgesehen davon, dass man einfach nicht weiß, warum es keine gute Idee ist, gibt es zwei große Kategorien von Ausreden.

  1. Performance. Das Kopieren eines Objekts bei jedem Aufruf eines Getters ist teuer. Das ist natürlich richtig, aber es ist nicht immer so teuer, wie Sie denken. Und wenn Sie zu hohe Kosten für defensive Kopien bezahlen, gibt es normalerweise Auswege: zum Beispiel indem Sie Ihre Klassen als unveränderlich gestalten. Wenn alle deine Klassen unveränderlich sind, brauchst du keine Verteidigungskopien.
  2. Nur ich werde diesen Code nennen und ich werde ihn nicht missbrauchen. Versprechen. Dies ist wiederum etwas, das je nach Situation gültig sein könnte. Wenn Sie selbst eine kleine eigenständige Anwendung schreiben, ist es wahrscheinlich wahr, dass Sie sie nicht missbrauchen werden. Oder wenn Sie eine kleine Bibliothek schreiben, aber nur Details zu Klassen offen legen, die nicht Teil der öffentlichen API sind. In den meisten anderen Fällen kann man jedoch nicht sicher sein, dass irgendjemand es irgendwo missbrauchen wird. Und wenn das passiert, wird Ihr Code wahrscheinlich nicht mit einem Knall brechen. Es beginnt normalerweise nur leicht ... gelegentlich seltsame Dinge. Und das ist der schlimmste Fehler, den es zu finden versucht.

7
2018-04-25 10:54



Es gibt zwei Dinge in Ihrem Code:

  1. Lazy Initialisierung, was ein gemeinsamer Ansatz ist.
  2. Einen Verweis auf ein veränderliches Inneres zurückgeben ArrayListDas ist nicht in Ordnung, weil es bricht Verkapselung. Wenn Sie Elemente zu dieser Liste hinzufügen möchten, sollten Sie diese veröffentlichen addItem() Methode nur.

3
2018-04-25 10:50



Dies ist ein gängiges Idiom, aber ich würde es nicht empfehlen, da der Anrufer so etwas tun kann:

final List<Item> items = bean.getItems();
final List notItems = (List) items;
notItems.add(new NotAnItem());

Der Aufrufer kann auch einen Verweis auf den List und versehentlich später mutieren.

Ich würde empfehlen, dieses bestimmte Idiom zu vermeiden und unveränderlich zu machen Ansichten von Gettern. Dies wird auf lange Sicht zu viel stabileren Code führen.

Damit:

public List<Item> getItems() {
    if (items == null) {
        return Collections.emptyList();
    }
    return Collections.unmodifiableList(this.items);
}

Und

public void setItems(final List<Item> items) {
    if (items == null) {
        throw new NullPointerException("items is null");
    }
    this.items = new ArrayList<>(items);
}

1
2018-04-25 11:01