Wer ist schuld - clone() oder equals()?

[Gregor]

Zeig mal das clone Deines eigenen Datentyps.

[cloneOrEquals]

Ich hab mal versucht ein Minimalbeispiel aus dem Code zu produzieren:

class MyData{
 public int i;
 public String s;

 public MyData(int i, String s){this.i = i; this.s = s;}

 /** Überflüssig? */
 public boolean equals(Object obj) {
        if(obj == null || !(obj instanceof MyData)) return false;
        if(obj == this)  return true;

        MyData md = (MyData) obj;
        if(md.i == i && md.s.equals(s))
            return true;

        return false;
 }
 /** Überflüssig? */
 protected Object clone() throws CloneNotSupportedException {
        return new MyData(i, s)
 }
}

// irgendwo
Vector<MyData> mdVector = new Vector<MyData>(10);
// Vector wird hier gefüllt
Vector<MyData> mdVector2 = (Vector<MyData>) new.clone();
// ein Element aus mdVector2 wird geändert, z.b. so
mdVector2.get(0).i = 12345678; //anderer Wert als vorher
System.out.println(mdVector2.equals(mdVector));

Hier bekomme ich immer true, obwohl mdVector2 ja offensichtlich geändert wurde?
Ich hab mir auch schon einen Ersatz für die Vector-equals() geschrieben, allerdings ohne Erfolg.
Wahrscheinlich ist die clone() oder equals() Überschreibung in MyData eh sinnlos, weil eh das macht, was die Methoden von Object schon machen.
Wo ist denn der Haken?

[cloneOrEquals]

Tippfehler: Zeile 27 soll nat. eher so aussehen

Vector<MyData> mdVector2 = (Vector<MyData>) mdVector.clone();

[Gregor]

Naja, dann ist es wohl so, dass die Objekte in der Collection selbst nicht geklont werden. Ich seh da auf den ersten Blick zumindest nichts in Deinem Code, was darauf schließen lässt, dass Du das verursachst.

...abgesehen davon:

1. Dein clone ist völlig falsch implementiert. Es gibt da eine Art Vertrag, den man beim Implementieren des clones einhalten muss, der nicht von der Sprache erzwungen wird. Wenn das nicht gemacht wird, dann kann das zu Problemen führen. Guck Dir diesbezüglich mal die Doku von Object.clone() an.

2. Eben wegen diesem Vertrag wird die Nutzung von clone insgesamt als problematisch angesehen. Ich empfehle Dir, clone insgesamt zu vermeiden.

[cloneOrEquals]

Vielen Dank für Eure Hinweise.
Ich hab den Vector jetzt so kopiert, wie unten im Code-Ausschnitt. Die equals()-Methode musste ich überschreiben, sonst bekomme ich 2x false zurück.
Eleganter habe ich es nicht hin bekommen. Wie würdet ihr es machen?

import java.util.*;

class Main{
	public static void main(String[] args){

		Vector<MyData> mdVector = new Vector<MyData>(10);
		Vector<MyData> mdVector2 = new Vector<MyData>(10);

		// mdVector füllen
		for(int i = 0; i < 10; i++) mdVector.add(new MyData(i, i + ""));

		// mdVector nach mdVector2 kopieren 
		//Vector<MyData> mdVector2 = (Vector<MyData>) mdVector.clone(); // liefert Referenzen?	
		for(int i = 0; i < mdVector.size(); i++) mdVector2.add( mdVector.get(i).copy() );			

		// test
		System.out.println(mdVector2.equals(mdVector));				
		mdVector2.get(0).s = "123"; //anderer Wert als vorher
		mdVector.get(1).s = "456";		
		System.out.println(mdVector2.equals(mdVector));

		for(int i = 0; i < mdVector.size(); i++){
			System.out.print( mdVector.get(i).s + "  ");
			System.out.println( mdVector2.get(i).s );
		}
	}
}

class MyData{
 public int i;
 public String s;

 public MyData(int i, String s){this.i = i; this.s = s;}
 public MyData copy(){ return new MyData(i, s); }

 public boolean equals(Object obj) {
        if(obj == null || !(obj instanceof MyData)) return false;
        if(obj == this)  return true;

        MyData md = (MyData) obj;
        return (md.i == i && md.s.equals(s));
 }

}

[Hilefoks]

Wenn du equals überschreibst solltest du auch hashCode() überschreiben.

[7z]

cloneOrEquals schrieb:

Vielen Dank für Eure Hinweise.
Ich hab den Vector jetzt so kopiert, wie unten im Code-Ausschnitt. Die equals()-Methode musste ich überschreiben, sonst bekomme ich 2x false zurück.
Eleganter habe ich es nicht hin bekommen. Wie würdet ihr es machen?

mit dem entsprechenden consturktor oder der addAll methode

[cloneOrEquals]

7z schrieb:

cloneOrEquals schrieb:

Vielen Dank für Eure Hinweise.
Ich hab den Vector jetzt so kopiert, wie unten im Code-Ausschnitt. Die equals()-Methode musste ich überschreiben, sonst bekomme ich 2x false zurück.
Eleganter habe ich es nicht hin bekommen. Wie würdet ihr es machen?

mit dem entsprechenden consturktor oder der addAll methode

Code?

[Hilefoks]

cloneOrEquals schrieb:

Code?

Gibt es nicht, der Vorschlag von 7z erzeugt auch nur flache Kopien, was du ja nicht möchtest.

Prinzipiell ist dein Code in Ordnung... aber:

1. hashCode() in MyData überschreiben
2. warum nimmst du eine Methode copy() - überschreib lieber die Methode clone().
3. wenn du schon dabei bist kannst du auch noch einen Copy-Konstruktor erstellen.
4. In deiner main-Methode kannst du das kopieren dann etwas "schöner" so lösen:

for(MyData data:mdVector) { mdVector2.add(data.clone()); }

oder (mit Copy-Konstruktor)

for(MyData data:mdVector) { mdVector2.add(new MyData(data)); }

MfG,
Hilefoks

[DEvent]

Macht Vector uberhaupt eine deep-copy?

Ansonsten, wurde schon geschrieben, aber naja,

wenn du equals() ueberschreibst, dann musst du auch hashCode() ueberschreiben.

[CStoll]

Gregor schrieb:

2. Eben wegen diesem Vertrag wird die Nutzung von clone insgesamt als problematisch angesehen. Ich empfehle Dir, clone insgesamt zu vermeiden.

Hilefoks schrieb:

2. warum nimmst du eine Methode copy() - überschreib lieber die Methode clone().

Es ist immer wieder schön zu sehen, wie einig sich die Java-Gemeinschaft bei solchen Grundlagenfragen ist *scnr*

[Javaner]

Es ist immer wieder schön zu sehen, wie einig sich die Java-Gemeinschaft bei solchen Grundlagenfragen ist *scnr*[/quote]

Also diejenigen, die sich hier über Java äussern, als Java-Gemeinschaft
zu bezeichnen, ist übertrieben.

[Hilefoks]

Javaner schrieb:

Also diejenigen, die sich hier über Java äussern, als Java-Gemeinschaft
zu bezeichnen, ist übertrieben.

Richtig.

CStoll schrieb:

Gregor schrieb:

2. Eben wegen diesem Vertrag wird die Nutzung von clone insgesamt als problematisch angesehen. Ich empfehle Dir, clone insgesamt zu vermeiden.

Hilefoks schrieb:

2. warum nimmst du eine Methode copy() - überschreib lieber die Methode clone().

Es ist immer wieder schön zu sehen, wie einig sich die Java-Gemeinschaft bei solchen Grundlagenfragen ist *scnr*

Dann möchte ich meine Aussage doch auch einmal begründen:

1. Die Methode zum klonen von Objekten heißt clone(). Ein Entwickler wird nicht auf die Idee kommen und schauen ob es auch eine Methode meineigenessueppchenzumclonen() gibt.
2. Gregor behauptet mit clone() müssten impliziet Verträge erfüllt werden. Und Gregor hat recht! Allerdings ist es Unfug diese Verträge nicht erfüllen zu müssen wenn man seine eigene clone() Mehtode verwendet.

Wenn man clone() ŭberschreibt, aber auch wenn man seine eigene Clone-Methode entwirft, muss man folgendes zusätzlich beachten:

1. Überschreibt man clone() muss man auch equals() überschreiben. equals() ist in Object wie folgt implementiert:

public boolean equals(Object obj) {
  return (this == obj);
}

Aus diesem Grund wär, überschreibt man equals() nicht, das original-Objekt nicht gleich dem geklonten. Ein verhalten was man ganz sicher nicht möchte.

2. Überschreibt man equals() muss man i.A. auch hashCode() überschreiben. hashCode() liefert einen möglichst eindeutige int-Wert um ein Objekt zu identifizieren (z.B. innerhalb einer HashMap). Inhaltlich gleiche Objekte sollten natürlich den gleichen int-Wert liefern. Wenn man nun aber equals() überschreibt und damit das default-Verhalten ändert, muss man auch i.A. das Verhalten von hashCode() ändern.

Ich denke es leuchtet ein das, egal mit welcher Methode man das Klonen eines Objekts implementiert, dieser Vertrag immer besteht. Den man möchte doch i.A. immer das ein geklontes Objekt gleich (sprich equals) dem original Objekt ist.

Man kann also sagen das dieser Vertrag nicht durch überschreiben von clone() erfüllt werden muss, sondern immer wenn man eine Möglichkeit zum klonen eines Objekts bereit stellt. Also auch dann wenn man einen Kopierkonstruktor erstellt oder eben auch wenn man seine clone()-Methode unsinnigerweise anders benennt. Natürlich gibt es auch Fälle in denen ein geklontes Objekt nicht gleich dem original Objekt sein soll - aber auch dann spricht nichts dagegeben clone() zu überschreiben und equals() sowie hashCode() entsprechend anzupassen.

MfG,
Hilefoks

P.S: Und natürlich sollte man, ganz unabhängig von clone(), equals() und dadurch auch hashCode() immer dann überschreiben, wenn man einen inhaltlichen Vergleich ermöglichen möchte.

[Gregor]

@Hilefoks: Das ist nicht der Vetrag, der mit clone verbunden ist. Bei clone geht es vielmehr darum, zu wissen, wie es richtig implementiert wird. Beispielsweise nutzt man in clone kein new, um sein Objekt zu erzeugen. Das macht man mit super.clone(). ...und danach überschreibt man alle Membervariablen (unter Umständen auch mit entsprechenden clones) mit den neuen Werten. Wenn Du clone anders implementierst, dann führt das zu bösartigem Code, der in anderem Code, der sich auf das korrekte Funktionieren des clones verlässt, zu Fehlern führen kann. Abgesehen davon verträgt sich clone in keinster Weise mit finalen Membervariablen. clone und final (bei Membervariablen) schließen sich praktisch gegenseitig aus.

Leider kennen viele den Vertrag von clone nicht und aus diesem Grund wird clone meistens fehlerhaft implementiert, was zu entsprechenden Bugs führen kann.

Ich empfehle, die Nutzung von Copy-Konstruktoren.