ArrayListe delete Methode klappt nicht

[dan1996]

Hallo ich versuche grade bei meiner ArrayListe die delete Methode zu schreiben, jedoch wenn mein Array voll ist und ich ein Wert löschen will entsteht eine
ArrayIndexOutOfBoundsException, kann mir jemand dabei helfen?


*frei = mein pointer

    public AListe<T> delete(T x) {
        if (array[length()] == x) { // wenn letzter Wert == x ist, auf null setzen
            array[length()] = null;

        } else {
            for (int i = 0; i < length(); i++) {
                if (array[i] == x) { // wert im array gefunden
                    for (int j = i; j < length() - 1; j++) {
                        array[j] = array[j + 1];
                    }
                }
                break;
            }
            
        }
        frei--;
        return this;
    }

 

[VfL_Freak]

array[length() - 1] ??

 

[kneitzel]

Also was mir auffällt: Du vergleichst Referenzen mit "==" - da solltest du equals nutzen.

Das Problem ist da aber so nicht zu erkennen für mich. Zeig einmal den ganzen Code. Was ist length(), wie ist array deklariert und initialisiert?
Also am Besten die ganze Klasse zeigen und nicht nur so kleine Ausschnitte.

Generell: Wenn Du da ein Array nutzt und das Array die Größe length() hat, dann kannst Du nicht auf array[length()] zugreifen. Der Index fängt bei 0 an, also bei 3 Elemente hast Du die Elemente 0, 1, 2 -> maximales Element ist also n-1 bei n Elementen.

 

[dan1996]

class AListe<T extends Number> {

    private int size;
    private T array[];
    private int frei = 0;

    @SuppressWarnings("unchecked")
    public AListe(int size) {
        this.size = size;
        this.array = (T[]) new Number[size];
    }

    public boolean isEmpty() {
        return frei == 0;
    }

    public AListe<T> insert(T x) { //wert am anfang einfügen
        if (isEmpty()) {
            append(x);
        } else {
            for (int i = length(); i > 0; i--) {
                array[i] = array[i - 1];
            }
            array[0] = x;
            frei++;
        }

        return this;
    }

    public AListe<T> append(T x) { //wert am Ende einfügen
        array[frei] = x;
        frei++;
        return this;
    }

    public AListe<T> delete(T x) {
        if (array[length()].equals(x)) { // wenn letzter Wert == x ist, auf null setzen
            array[length()] = null;

        } else {
            for (int i = 0; i < length(); i++) {
                if (array[i].equals(x)) { // wert im array gefunden
                    for (int j = i; j < length(); j++) {
                        array[j] = array[j + 1];
                    }
                }
                break;
            }
           
        }
        frei--;
        return this;
    }

    public T firstElement() {
        return array[0];
    }

    public int length() {
        return frei;
    }

    public int size() {
        return size;
    }

 

[kneitzel]

Also ich schaue es mir gerade an um es zu verstehen. Die Namen sind in meinen Augen schlecht gewählt ... Es ist nicht intuitiv, dass length() frei zurück gibt. "frei" wäre sowas wie "nextfreeElement" oder so...

public AListe<T> insert(T x) {
        if (isEmpty()) {
            append(x);
        } else {
            for (int i = length(); i > 0; i--) {
                array[i] = array[i - 1];
            }
            array[0] = x;
            frei++;
        }

        return this;
    }

Die isEmpty Prüfung braucht es nicht. idEmpty ist dann wahr, wenn frei == 0, damit hast du eine Schleife, die nie durchlaufen wird (i wird ja auf 0 gesetzt) und daher hast Du direkt die Aktivität von Append (dessen Return-Wert du ehh ignorierst) ==> Also auch doppelter code!

Wenn length() = frei = Nächstes freies Element:
Dann ist dein Delete Unsinn: Du prüfst, ob auf dem nächsten freien Element das gesuchte Element ist. Da müsste aber doch ehh null drin stehen... Und die "Verbesserung" mit dem equals führt jetzt zu einer NPE, weil Du auf null ein equals aufrufst. (oder sollte ich mich da jetzt irren?)

So wie bei dem insert brauchst du da auch gar keine Sonderbehandlung.
Du machst eine Schleife von 0 bis <length. So Du das Element noch nicht gefunden hast, prüfst Du nur, ob das Element da ist.
Sobald Du das Element gefunden hast:
Bist du auf dem letzten Element (size prüfen!), dann schreibst Du in das aktuelle Element null, ansonsten kopierst Du nur das nächste Element auf das aktuelle Element.

So wäre ein möglicher Code, den ich mir gerade ohne viel drüber nachzudenken überlegt habe.

Ansonsten noch:
Generell musst Du natürlich bei einem Insert prüfen, ob length() schon size() ist. Dann wäre das Array voll und du kannst nichts mehr einfügen.

 

[dan1996]

ich danke dir schonmal, die Sachen die du bemängelt hast, wie Name, isEmpty, append, length und insert standen bei mir so in den Vorlesungsfolien und ich wollte eigentlich nur noch eine delete Methode in die Klasse hinzufügen. Ja genau mit insert stimmt auch, nur dass ich das noch nicht dazu geschrieben habe

 

[kneitzel]

Wenn so Namen vorgegeben wurden, dann ist dem (leider) so. Das betrifft halt nicht die Funktionalität sondern geht halt in den Bereich Clean Code und Lesbarkeit des Codes.

Die Anmerkungen zu der Delete Methode waren verständlich? (Und das mit dem "Unsinn" nicht persönlich nehmen - das klassifiziert Dich nicht Deine Arbeit oder Person sondern lediglich den Code an der Stelle. Und das da nicht alles perfekt ist, hast Du ja auch gemerkt sonst hätte Du ja nicht die Thematik hier gepostet. Das nur am Rande, weil mir das gerade noch einmal ins Auge fiel. Blöd / Unhöflich formuliert von mir, aber ich habe mir zu wenig Zeit genommen und mit Blick auf deinem Code einfach paar Sachen runter geschrieben.)

 

[dan1996]

Alles gut gar kein Thema, bei der Stelle:

Bist du auf dem letzten Element (size prüfen!), dann schreibst Du in das aktuelle Element null

wenn ich mein Element, was ich ja löschen will, auf null schreibe, muss ich doch alle Werte danach um ein index nach links verschieben, was ja mein Problem ist.. Es funktioniert irgendwie nicht

 

[kneitzel]

ja, das musst Du. Aber wenn das Array voll ist, dann kommst Du irgendwann zum letzten Element.
Bei dem letzten Element kannst Du natürlich keinen Nachfolger mehr kopieren sondern statt dessen kannst Du nur noch null rein schreiben.

Also Du hast 10 Elemente. Beim 10. Element kannst Du nicht mehr versuchen, das 11. Element in das 10. Element zu schreiben. Du musst erkennen: "Ich bin am Ende angekommen" und dann das Feld einfach frei machen (null zuweisen).

 

[dan1996]

habe ich grade einen Denkfehler oder warum klappt das nicht, aber erstmal Danke für deine Hilfe

public AListe<T> delete(T x) {

        if (array[length()] == x) { // wenn letzter Wert == x ist, auf null setzen
            array[length()] = null;

        } else {

            for (int i = 0; i < length(); i++) {
                if (array[i] == x) { // wert im array gefunden

                    for (int j = i; j < length(); j++) {

                        if (j == size()) {
                            array[j] = null;
                        } else {
                            array[j] = array[j + 1];
                        }
                    }
                }
            }
        }
        frei--;
        return this;

 

[kneitzel]

Versuch es mal in der Art und Weise (ungeprüft und ggf. mit Tippfehlern)

public AListe<T> delete(T x) {
/*
        if (array[length()] == x) { // wenn letzter Wert == x ist, auf null setzen
            array[length()] = null;

        } else {
*/
            boolean gefunden = false;
            int current = 0;
            while (current < length()) {
                if (gefunden) {
                    if (current == size()-1) { // -1! Bei 10 Elementen ist das letzte die 9!
                        array[current] = null;
                    } else {
                        array[current] = array[current + 1];
                    }
                    current++;
                } else if (array[current].equals(x) {
                    gefunden = true;
                    // Wir gehen nicht weiter! Im nächsten Durchgang wird das Element überschrieben.
                } else {
                    current++;
                }
            }
//        }
        if (gefunden) frei--;
        return this;
}

Also der erste Part muss raus da der so kein Sinn gemacht hat.

Dann waren die zwei schleifen blöd, denn wenn du in der inneren bis zum Ende gelaufen bist, dann bist du danach in der äußeren auch noch weiter gelaufen. Da aber jetzt nicht immer erhöht wird habe ich keine for Schleife mehr sondern eine while Schleife.

Ein Fehler war noch die Erkennung des letzten Elements. Das letzte Element ist size() - 1 und nicht size()!

Und er reduziert frei nur, wenn er auch ein Element gefunden hat...

Dann ist der Ablauf einfach:
- Er geht in einer Schleife alle Elemente durch:
-- Hat er das Element bereits gefunden, dann nehmen wir nur noch das nächste Element oder eben null, wenn es das nicht gibt. Und gehen weiter.
-- Wenn er das Element noch nicht gefunden hat, dann prüft er das aktuelle Element. Ist es das, dann hat er es gefunden und er kann ohne weiter vor zu rücken einfach erneut den Schleifen-Körper durchlaufen womit er den ersten Punkt ansteuert.
-- Wenn er das Element noch nicht gefunden hat und das aktuelle Element nicht das gesuchte ist, dann geht er ein Element weiter.
- am Ende: Wenn das Element gefunden wurde, dann wurde ja ein Element entfernt - daher ist frei um eins zu reduzieren.

Das return this macht nicht wirklich Sinn. Ich hätte da eine void oder boolean Methode erwartet. Bei boolean könnte man gefunden zurück geben.

 

[dan1996]

ich danke dir, es funktioniert alles einwandfrei, danke!!!
Das Return this wollten die in den Vorlesungsfolien haben, habe auch keine Ahnung warum

 

[kneitzel]

Naja ...damit kann man so Aufrufe aneinander reihen. Wenn man immer die eigentliche Instanz hat, dann schreibt man sowas wie:

meineListe
  .insert(1)
  .insert(2)
  .delete(3)
  .print()
  .delete(2)
  .insert(14);

Das ist recht bekannt bei dem "Builder Pattern".Um komplexe Konstruktoren zu vermeiden wird da auf eine Builder klasse zurück gegriffen. Da macht es dann auch tatsächlich Sinn:

Adresse adresse = Adresse.builder()
                    .name("Anton Maier")
                    .strasse("Musterstrasse 17")
                    .plz("12345")
                    .ort("Musterhausen")
                    .land("Musterland")
                    .build();

Außerhalb von Buildern ist das aber zumindest unüblich.