4. Bitte um Bewertung meines Codes(SNAKE)

Bitte um Bewertung meines Codes(SNAKE)

  • B

    Hallo, ich habe vor kurzem mein Snake-Spiel fast fertig programmmiert. Die Grundeigenschaften, wie das zufällige erstellen von futter, die steuerung, das wachsen der Snake usw. sind drin. Aber ich bin noch totaler Anfänger was Objektorientierte Programmierung GUI's angeht und suche nach Tipps für ein besseres Design. Deshalb wollte ich euch darum bitten den Code zu bewerten, ob ihr vllt. Verbesserungsvorschläge habt.
    Wäre wirklich cool, wenn sich jemand die Mühe geben würde (:

    Hier sind erstmal alle Header:
    food.h

    #ifndef FOOD
#define FOOD

#include <QGraphicsRectItem>
#include <QObject>
#include <vector>
#include <stdlib.h>

class Food : public QObject, public QGraphicsRectItem{

public: Food();
        void setRandomCoordinates();
};

#endif // FOOD

    mainwindow.h

    #ifndef MAINWINDOW
#define MAINWINDOW
#include <QApplication>
#include <QGraphicsScene>
#include <QGraphicsView>
#include "snakebody.h"
#include "movementorganizer.h"
#include "food.h"
#include <QKeyEvent>
#include <vector>

class Mainwindow{

public: Mainwindow();

};

#endif // MAINWINDOW

    movementorganizer.h

    #ifndef MOVEMENTORGANIZER
#define MOVEMENTORGANIZER
#include "snakebody.h"
#include "food.h"
#include <QGraphicsRectItem>
#include <QObject>
#include <QKeyEvent>
#include <vector>

enum direction{LEFT,RIGHT,UP,DOWN};

class MovementOrganizer : public QObject, public QGraphicsRectItem {

Q_OBJECT

public:
        MovementOrganizer(std::vector<SnakeBody*>a, Food* collidingObject);
        void keyPressEvent(QKeyEvent * event);
        void setNachfolgerCoordinates(std::vector<SnakeBody*>parts, int index );
        void createNewBodyPart(std::vector<SnakeBody*>*a);
        bool bitYourself(std::vector<SnakeBody*>*a);

        void setMoveDirectory(direction i);

 std::vector<SnakeBody*> pparts;
 Food* collidingWith;
 direction move_directory;

 public slots: void move();
};

#endif // MOVEMENTORGANIZER

    snakebody.h

    #ifndef SNAKEBODY
#define SNAKEBODY
#include <QGraphicsRectItem>
#include <QObject>
#include <vector>

class SnakeBody : public QObject, public QGraphicsRectItem{

public: SnakeBody();

};

#endif // SNAKEBODY

    Jetzt kommen die Quelldateien:

    food.cpp

    #include "food.h"
#include <QGraphicsScene>
#include <QKeyEvent>

Food::Food()
{
    this->setRect(0,0,100,100);
    this->setBrush(*new QBrush(Qt::red));
}

void Food::setRandomCoordinates()
{
    int rand_x = rand() % 7;
    int rand_y = rand()% 5;

    this->setPos(rand_x*100, rand_y*100);
}

    mainwindow.cpp

    #include "mainwindow.h"

Mainwindow::Mainwindow()
{
    // fenstereinstellungen
    QGraphicsScene * scene = new QGraphicsScene();
    scene->setSceneRect(0,0,800,600);
    QGraphicsView * view = new QGraphicsView(scene);
    view->show();
    view->setFixedSize(800,600);
    view->setHorizontalScrollBarPolicy(Qt::ScrollBarAlwaysOff);
    view->setVerticalScrollBarPolicy(Qt::ScrollBarAlwaysOff);

    // schlangenteile initialisieren
    SnakeBody * part = new SnakeBody();
    part->setPos(0,100);
    SnakeBody * part2 = new SnakeBody();
    part2->setPos(-100,100);
    SnakeBody * player = new SnakeBody();
    player->setPos(100,100);
    std::vector<SnakeBody*> v;
    v.push_back(player);
    v.push_back(part);
    v.push_back(part2);
    Food * collidingObject = new Food();
    collidingObject->setPos(200,200);

    // movementorganizer initialisieren
    MovementOrganizer * mo = new MovementOrganizer(v, collidingObject);
    mo->setFlag(QGraphicsItem::ItemIsFocusable);
    mo->setFocus();

    // items der scene hinzufügen
    scene->addItem(player);
    scene->addItem(part);
    scene->addItem(part2);
    scene->addItem(collidingObject);
    scene->addItem(mo);

}

    movementorganizer.cpp

    #include "movementorganizer.h"
#include <QKeyEvent>
#include <QGraphicsScene>
#include <QDebug>
#include <QTimer>

MovementOrganizer::MovementOrganizer(std::vector<SnakeBody *> a, Food *collidingObject)
{
    for(unsigned int i = 0; i< a.size(); i++){
        pparts.push_back(a[i]);
    }

    collidingWith = collidingObject;
    QTimer * timer = new QTimer();
    connect(timer,SIGNAL(timeout()),this,SLOT(move()));
            timer->start(500);
}

void MovementOrganizer::keyPressEvent(QKeyEvent* event){

    switch(event->key()){
    case Qt::Key_Left:
        this->setMoveDirectory(LEFT);
        break;
    case Qt::Key_Right:
        this->setMoveDirectory(RIGHT);
        break;
       case Qt::Key_Up:
        this->setMoveDirectory(UP);
    break;
    case Qt::Key_Down:
    this->setMoveDirectory(DOWN);

    break;

    }
}

void MovementOrganizer::setNachfolgerCoordinates(std::vector<SnakeBody *>  parts, int index)
{
    parts[index]->setY(parts[index-1]->y());
    parts[index]->setX(parts[index-1]->x());
}

void MovementOrganizer::createNewBodyPart(std::vector<SnakeBody *>* a)
{
    qDebug() << "new BodyPart created" << pparts.size() ;
    a->push_back(new SnakeBody());
    scene()->addItem((*a)[a->size()-1]);

}

bool MovementOrganizer::bitYourself(std::vector<SnakeBody *> *a)
{   bool output = false;
    for(unsigned int i=0; i< a->size(); i++){

        if(i!= 0 && pparts[0]->x() == pparts[i]->x() && pparts[0]->y()==pparts[i]->y()){
            output = true;
            qDebug() << "You bit yourself!";
        }
    }
    return true;
}

void MovementOrganizer::setMoveDirectory(direction i)
{
    switch(i){
    case 0: move_directory = LEFT;
        break;
    case 1: move_directory = RIGHT;
        break;
    case 2: move_directory = UP;
        break;
    case 3: move_directory = DOWN;
        break;
    default: move_directory = RIGHT;
    break;

    }
}

void MovementOrganizer::move()
{
    if(this->move_directory == LEFT){

        for(int i=pparts.size()-1; i>=0; i--){
            // es wird auf i==0 überprüft um den schlangen kopf zu ermitteln, der die bewegung seiner hinterteile bestimmt.
            if(i == 0 && pparts[0]->x()>0){
                pparts[i]->setX(pparts[i]->x()-100);
                 bitYourself(&pparts);
            }
            else if(i!=0 && pparts[0]->x()>0){
                setNachfolgerCoordinates(pparts, i);
            }
            if(i == 0 && pparts[0]->x()== collidingWith->x() && pparts[0]->y()== collidingWith->y()){
              createNewBodyPart(&pparts);
              collidingWith->setRandomCoordinates();
             }

            }
    }

    if(this->move_directory == RIGHT){
        for(int i=pparts.size()-1; i>=0; i--){

            if(i == 0 && pparts[0]->x()<700){
                pparts[i]->setX(pparts[i]->x()+100);
                bitYourself(&pparts);
            }
            else if(i!=0 && pparts[0]->x()<700){
                 setNachfolgerCoordinates(pparts, i);
            }
            if(i == 0 && pparts[0]->x()== collidingWith->x() && pparts[0]->y()== collidingWith->y()){
            createNewBodyPart(&pparts);
            collidingWith->setRandomCoordinates();
            }
        }

    }

    if(this->move_directory == UP){

        for(int i=pparts.size()-1; i>=0; i--){

            if(i == 0 && pparts[0]->y()>0){
                pparts[i]->setY(pparts[i]->y()-100);
                bitYourself(&pparts);
            }
            else if(i!=0 && pparts[0]->y()>0){
                 setNachfolgerCoordinates(pparts, i);
            }
            if(i == 0 && pparts[0]->x()== collidingWith->x() && pparts[0]->y()== collidingWith->y()){
                createNewBodyPart(&pparts);
                collidingWith->setRandomCoordinates();
            }
        }

    }

    if(this->move_directory == DOWN){

        for(int i=pparts.size()-1; i>=0; i--){

            if(i == 0 && pparts[0]->y()<500){
             pparts[i]->setY(pparts[i]->y()+100);
              bitYourself(&pparts);
            }
            else if(i!=0 && pparts[0]->y()<500){
             setNachfolgerCoordinates(pparts, i);
            }
            if(i == 0 && pparts[0]->x()== collidingWith->x() && pparts[0]->y()== collidingWith->y()){
             createNewBodyPart(&pparts);
             collidingWith->setRandomCoordinates();
            }
        }

    }

}

    snakebody.cpp

    #include "snakebody.h"
#include <QGraphicsScene>
#include <QKeyEvent>
#include <QGraphicsScene>
#include <QDebug>
#include "food.h"

SnakeBody::SnakeBody()
{
    this->setRect(0,0,100,100);
}

    main.cpp

    #include <QApplication>
#include <QGraphicsScene>
#include <QGraphicsView>
#include "snakebody.h"
#include "movementorganizer.h"
#include "food.h"
#include <QKeyEvent>
#include <vector>
#include "mainwindow.h"

int main(int argc, char *argv[])
{
    QApplication a(argc, argv);

    Mainwindow window;
    return a.exec();
}

    Für die, die sich wundern, warum ich movementOrganizer von QGraphicsItemsRect erben gelassen habe. Ich war auf der suche nach einer Klasse von der aus ich die keyPressed Events verwenden kann. Am Anfang habe ich erst nur ein Kästchen bewegt später habe ich eine ganze Klasse geschrieben um die Bewegungen zu regeln. Und irgendwie hat es nur geklappt, wenn ich von Rect geerbt habe. Deshalb bin ich dabei geblieben.

    Mit freundlichen Grüßen BlackJoe

    0
  • M

    Du solltest den Code eher C++ oder rund um Programmierung reinstellen, nicht viele schauen hier rein. Andererseits ist der Code doch so Qt spezifisch, dass sich den auch in anderen Unterforen wahrscheinlich nicht so viele anschauen werden.

    Nur paar Sachen, die mir spontan auffallen:

    1. Dir fehlen anscheinend noch paar Grundlagen, sowas ist natürlich ganz übel: *new QBrush(Qt::red). Du hast die Speicherverwaltung in C++ nicht verstanden, schaus dir nochmal an.

    2. Objekte übergibt man am besten als Referenzen auf konstante Objekte als Parameter und nicht als Kopie. Evtl. hast du da noch dieselben Wissenslücken wie bei Punkt 1. Vor allem beim std::vector machts keinen Sinn, den zu kopieren. Auch wenn man den verändern will, dann auch am besten als Referenz (nicht const) und nicht als Zeiger übergeben.

    3. Es ist grundsätzlich nie eine gute Idee, irgendwelche Schichten zu vermischen, in deinem Fall (und überhaupt ist das der häufigste Fall) Logik und GUI. Du hast quasi nur GUI Klassen, die von QGraphicsRectItem abgeleitet sind. Damit kannst du kaum was wiederverwenden. Wenn du später ein Web-Snake machen willst, oder ein 3D-Snake, musst du alles kopieren oder umbauen. Hättest du die Logik rausgezogen, könnte man die Logik wiederverwenden, auch wenn man später als GUI 3D statt Qt verwenden will. Ist jetzt bei den paar Zeilen Code nicht schlimm, aber die Tendenz ist abzusehen und das ist sehr oft ein gravierendes Problem.

    4. Ich vermute in dem Code Memory Leaks, bin mir auswendig aber nicht sicher, ob QGraphicsScene nicht die Ownership übernimmt.

    5. Vermeide Denglisch, sowas ist Code, bei dem ich normalerweise überhaupt keine Lust mehr habe, weiterzulesen: setNachfolgerCoordinates.

    An der Struktur an sich könnte man wahrscheinlich auch noch vieles verbessern oder anders machen, aber da will ich mich jetzt nicht reindenken.

    0
  • B

    Alles klar. Danke für die Tipps. 🙂
    Ich denke ich werde den Code nochmal überarbeiten und einiges Nacharbeiten.

    0
  • M

    Noch was...

    Richtung heißt Direction und nicht Directory und beißen heißt bite.

    Bei setMoveDirection gibst du ja schon das enum rein (ab C++11 gibts typsicherere enum classes), da brauchst du kein switch-case, du brauchst nur den Parameter speichern.

    p.s. Ich korrigiere dein Englisch übrigens, weil das wichtig ist 😉 Nicht unbedingt in den paar Zeilen Code. Aber wir haben in der Arbeit z.B. 6 Mio Zeilen Code + dutzende Millionen Zeilen 3rd party Code. Und man "schaut sich das nicht durch", man muss suchen. Und wenn ich meine, da war irgendwo was mit setMoveDirection und finde das nicht, weil du das setMoveDirectory genannt hast, kostet das Zeit. Und je komplexer und umfangreicher der Code wird, desto mehr Zeit kostet sowas.

    0
Anmelden zum Antworten