Kleines C++ Problem

[Ronschk]

Hi,
ich sitze jetzt schon einige Zeit davor und bekomm nicht raus, wo der Fehler liegt.

 eol=Puzzle_List.end();


        for(iter=Puzzle_List.begin(); iter!=eol; iter++){
            if (ctr>0)
                Puzzle_List.pop_front();
            for(int i=0; i<4; i++){                 //Find '*'
                for(int j=0; j<4; j++){
                    if((*iter)->get_element(i,j)=='*'){
                        x=i;
                        y=j;
                    }
                }
            }

            if(x-1>=0 &&(*iter)->last_move()!='d'){//Move '*' upwards
                Puzzle=new CPuzzle;
                *Puzzle=**iter;
                Puzzle->swap(x, x-1, y, y);
                Puzzle->set_last_move('u');
                Puzzle_List.push_back(Puzzle);

            }

            ctr++;
            cout << ctr << " " << k << endl;
        }

Das ist die vereinfachte (es gibt noch drei //Move-If-Anweisungden drin) innere Schleife die ich habe. Ich habe extra den Pointer auf das letzte Listenelement gespeichert, da die Liste innerhalb der schleife verändert wird.
Allerdings bricht die Schleife niemals ab.

(Programm:
man hat ein Puzzel gegeben, à la
1 2 3 4
6 9 7 *
5 A B 8
D E F C
und das soll man mit Hilfe von Breitensuche in die richtige Reihenfolge bringen)


€dit: Ich hab auch schon probiert mir die Größe der Liste zu speichern und dann von ne for-schleife von 0 -> listengröße zu machen und in jeder Iteration den iterator zu inkrementieren, aber das funktioniert noch viel weniger und führt zu nem Programm abbruch

 

[ROOT]

Funktionierts denn, wenn du das push_back am Ende auskommentierst?
Würd es mal debuggen um zu sehen, zu welchen Listenelementen er da genau hinspringt... eigentlich kann das ja nicht unendlich gehen, außer du hast in der Liste selbst ne Loop drin.

Hat CPuzzle nen sauberen Copy-Konstruktor?

 

[Ronschk]

Ne, dann bricht das Prog wieder ab.


Hmm ich hab mir noch überlegt einen zu machen, aber dann hab ichs sein gelassen, weil ich mir dachte, dass ich den nicht brauche... wo brauche ich den denn? Ich hab den Zuweisungsoperator natürlich neu implementiert.

 

[Gelöschtes Mitglied 160054]

Fehler bedeutet bei dir, dass er niocht so sortiert wie du es gerne hättest? Dann poste doch mal wie er das sortiert, also wie der Endzustand aussieht.

 

[Clawg]

Tipps:
- mach mal die magic numbers raus
- lies mal ein Buch über Variablenbennenung
- ship das pop an das Ende und mach den ctr raus
- was soll denn dieser hack!? "Puzzle=new CPuzzle; *Puzzle=**iter;"
- benutze keinen copy Operator, schreibe den Copy Operator in den private Teil rein. Konstruktoren bauen Objekte.
- lies http://www.parashift.com/c++-faq-lite/index.html kthx

Also ich würde in so einem Code auch keine Fehler finden können, das kann doch niemand lesen.

edit: ach gott, jetzt seh ichs erst, du modifizierst eine Liste während du über sie iterierst
Bau eine zweite Liste auf, die du in die erste einfügst. Ja, du hast "eol" benutzt, das ist aber alles andere als sauber.

Das ist die vereinfachte (es gibt noch drei //Move-If-Anweisungden drin) innere Schleife die ich habe. Ich habe extra den Pointer auf das letzte Listenelement gespeichert, da die Liste innerhalb der schleife verändert wird.
Allerdings bricht die Schleife niemals ab.

Quatsch. end() ist nicht das letzte Listenelement, end() ist ein Zustand eines Listeniterators, der aussagt, dass der Iterator auf das ENDE zeigt, also immer hinter das letzten Element. Da du hinten immer was anfügst, wird die Liste nie leer.

 

[Ronschk]

Tipps:
- mach mal die magic numbers raus

magic numbers? googlen hat mir auf die schnelle nichts gebracht, bzw der Wikipedia Artikel bringt mich in dem Zusammehang nicht weiter

- lies mal ein Buch über Variablenbennenung

Sorry, in so kleinen Programmen die ich nur einmal benutze achte ich da nicht so drauf. Sorry :P

- ship das pop an das Ende und mach den ctr raus

ich habs am Anfang gelssen, da ich mir nicht sicher war, was für ne Auswirkung es hat, wenn ich das Listenelement lösche auf dem ich bin. ... Aber eigentlich sollte es klar gehen.
ctr habe ich drinnen gelassen um die Anzahl der Vertauschungen zu zählen, aber an sich kann ich das auch anders machen, hast recht.

- was soll denn dieser hack!? "Puzzle=new CPuzzle; *Puzzle=**iter;"

Okay abgesehen von der Variablenbenennung:
ich hab ein Pointer auf eine Klasse CPuzzle, und erstelle mir ein neues Objekt von der Klasse. Und dann weise ich dem neu erstellten Objekt die Werte von dem Objekt zu, welches gerade in der Liste ist... so schlimm?

- benutze keinen copy Operator, schreibe den Copy Operator in den private Teil rein. Konstruktoren bauen Objekte.

Was das Ding tut weiß ich, nur wollte ich wissen wo in meinem Programm ich einen brauchen sollte. Und warum sollte ich den privat machen?

- lies http://www.parashift.com/c++-faq-lite/index.html kthx

Werds mir mal anschauen

Also ich würde in so einem Code auch keine Fehler finden können, das kann doch niemand lesen.

edit: ach gott, jetzt seh ichs erst, du modifizierst eine Liste während du über sie iterierst
Bau eine zweite Liste auf, die du in die erste einfügst. Ja, du hast "eol" benutzt, das ist aber alles andere als sauber.


Quatsch. end() ist nicht das letzte Listenelement, end() ist ein Zustand eines Listeniterators, der aussagt, dass der Iterator auf das ENDE zeigt, also immer hinter das letzten Element. Da du hinten immer was anfügst, wird die Liste nie leer.

Ich dachte, dass end() mir einen Pointer auf das letzte Listenelement gibt (bzw ja, das was nach dem letzten Listenelement kommt) . Also speichere ich mir wo das letzte Listenelement ist und lasse meinen Iterator bis dahin wandern...
Wie soll das denn ein Zustand vom Iterator sein, wenn end() von der Liste ist?

Bau eine zweite Liste auf, die du in die erste einfügst

Wie? Ich habe eine Liste. Nehme mir das erste Element daraus. Verändere das Element bis zu vier mal und schreib es in eine neue Liste. Dann nehme ich mir das zweite Element raus und mache dasselbe? Dann habe ich zig verschiedene Listen und die Speichere ich dann in einer Liste von Listen von Puzzlen?

 

[ViperDK]

Also glaub das ist das erste mal das ich Claw vollkommen recht geben muss

Magic Number: http://en.wikipedia.org/wiki/Magic_number_(programming)#Unnamed_numerical_constants
Damit meint Claw die "4" wo nirgends zu erkennen ist warum da ausgerechnet 4 steht.
Dafür sollte man halt eine const definieren die einen sinnvollen namen hat wie xDimensionSize etc. (jetzt mal abgesehen von der namenskonvention, ka was da in c++ gängig ist)

 

[Ronschk]

ah okay.

 

[Ronschk]

        for(LoL_iter=LoL.begin(); LoL_iter!=LoL.end(); LoL_iter++){
            Puzzle_List= new list<CPuzzle*>;

            for(iter=LoL_iter->begin(); iter!=LoL_iter->end(); iter++){

                for(int i=0; i<(*iter)->get_FieldSize_n(); i++){                 //Find '*'
                    for(int j=0; j<(*iter)->get_FieldSize_m(); j++){

                        if((*iter)->get_element(i,j)=='*'){
                            EmptyField_n=i;
                            EmptyField_m=j;
                        }
                    }
                }


                if(EmptyField_m-1>=0 &&(*iter)->last_move()!='d'){//Move '*' upwards
                    Puzzle=new CPuzzle;
                    *Puzzle=**iter;
                    Puzzle->swap(EmptyField_n, EmptyField_n-1, EmptyField_m, EmptyField_m);
                    Puzzle->set_last_move('u');
                    Puzzle_List->push_back(Puzzle);

                }

            }
            LoL.push_back(*Puzzle_List);
            LoL.pop_front();
        }

Okay. das ist jetzt mein die veränderte Version, magic numbers draußen, ein paar Variablen hoffentlich verständlicher und eine List of List (LoL) eingefügt.

Allerdings bricht das Programm jetzt wieder ab. Und zwar bei //Find '*' in der schleife.
Wenn ich mir vorher k ausgeben lasse (einfach ne Variable die nachdem ich LoL einmal durchgegangen bin inkremeniert wird), wird vor zwei mal 0 ausgegeben. Danach einmal...

 

[Starfire]

gibst du dir eigentlich nach jedem iterationsschritt die komplette matrix auf der konsole aus?

 

[Ronschk]

wenn ich nach dem LoL.pop_front() versuche mir den gesamten Inhalt von LoL ausgeben zu lassen crash das Programm nach dem ersten Puzzle ab. Allerdings wurde das Puzzle garnicht verändert.
Also ich gebe
1 2 3 4
6 9 7 *
5 A B 8
D E F C
in das Programm ein und nach LoL.pop_front() kommt dasselbe wieder raus...

 

[Clawg]

Sorry, in so kleinen Programmen die ich nur einmal benutze achte ich da nicht so drauf. Sorry :P

Du meinst, weil kleine Programme wenig fehleranfällig sind?

ich hab ein Pointer auf eine Klasse CPuzzle, und erstelle mir ein neues Objekt von der Klasse. Und dann weise ich dem neu erstellten Objekt die Werte von dem Objekt zu, welches gerade in der Liste ist... so schlimm?

Es ist verwirrend. Wer deinen Code liest (einschließlich du selbst - eine Zeit später) versteht nicht gleich, was hier passieren soll. Wenn ich sehe, dass ein Iterator einem anderen Objekt zugewiesen wird, denke ich, dass der Iterator eben mal kurz zwischengespeichert werden soll o.ä., aber nicht, dass hier groß Kopien des Objekts erstellt werden.
Klar wird es, wenn du da den ganz normalen Konstruktor von Puzzle aufrufst, z.B.:
Puzzle* puzzle = new Puzzle(i->getName(), i->getCoordinate()); o.ä.

Dein "Puzzle=new CPuzzle;" ist auch nicht Standard und widerspricht auch dem RAII Prinzip.

Was das Ding tut weiß ich, nur wollte ich wissen wo in meinem Programm ich einen brauchen sollte. Und warum sollte ich den privat machen?

a) du möchtest den Code nach einer Woche/Monat/Jahr nochmal ansehen.
b) du möchtest den Code anderen Leuten zeigen.

Bestimmte Programmierparadigmen/prinzipien sorgen außerdem nachweislich für eine geringere Fehlerdichte.

Ich dachte, dass end() mir einen Pointer auf das letzte Listenelement gibt (bzw ja, das was nach dem letzten Listenelement kommt) . Also speichere ich mir wo das letzte Listenelement ist und lasse meinen Iterator bis dahin wandern...
Wie soll das denn ein Zustand vom Iterator sein, wenn end() von der Liste ist?

Du arbeitest mit einer Linked-List bei der das Konzept einer identifizierbaren Position keinen Sinn ergibt. Du willst einen vector, den du über einen Index ansprechen kannst. Nachteil ist da natürlich die Performance, weil du dauernd einfügst und entfernst.
Deshalb solltest du mit 2 Listen arbeiten. Diese Arbeitsweise würde sich z.B. automatisch ergeben, wenn du das Prinzip des "const-correctness" sauber umgesetzt hättest. In-place Algorithmen sind eher zu vermeiden, eine Funktion sollte eine konstante Eingabe haben und die Arbeit auf eigenen Objekten verrichten statt in der Eingabe herumzufuhrwerken.

Wie? Ich habe eine Liste. Nehme mir das erste Element daraus. Verändere das Element bis zu vier mal und schreib es in eine neue Liste. Dann nehme ich mir das zweite Element raus und mache dasselbe? Dann habe ich zig verschiedene Listen und die Speichere ich dann in einer Liste von Listen von Puzzlen?

Du kannst Listen während des Programmablaufs aneinanderfügen. Die Eingabeliste lässt du konstant und schreibst alle Ergebnisse/Änderungen in die zweite Liste.

 

[Starfire]

mit welcher entwicklungsumgebung machst das eigentlich?
visual studio hat doch nen guten eingebauten debugger, da erkennst schon nach einem start wo das kracht und wohin die pointer zeigen.