Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

This is my version of snakes in C++ using the ncurses library. I would like to hear from you how this piece of code can be improved and general advice for future projects regarding coding and efficiency. I have tried writing this code in pure OOP, let me know about that too.

#include<ncurses.h>
#include<unistd.h>
#include<time.h>
#include<stdlib.h>

class snake                         //to store the x and y coordinates of each snake part
{
    int x, y;
    char ch;
    public:
    snake(){ x= y= 0; ch='O';}
    snake(int a, int b)
    {
        x= a; y= b; ch= 'O';
    }
    snake(const snake &ekans)
    {
        ch= ekans.ch;
        x= ekans.x;
        y= ekans.y;
    }
    void setCh(char x)
    {
        ch= x;
    }
    char getCh()
    {
        return ch;
    }
    int getX()
    {
        return x;
    }
    void setX(int no)
    {
        x= no;
    }
    int getY()
    {
        return y;
    }
    void setY(int no)
    {
         y= no;
    }
};

class list                          //to store each snake body          
{
    node* head;
    int length;
    public:
    list()
    {
        head= NULL;
        length= 0;
    }
    void add(snake s)                   //adds at the end
    {
        node *n= new node;
        n->setSnake(s);
        n->setNext(head);
        head= n;
        length++;
    }
    int listLength()
    {
        return length;
    }
    snake get(int n)
    {
        node *temp =head;
        int count= 1;
        while(count!= n && temp!= NULL)
        { 
            count++;
            temp= temp->getNext();
        } 
        return temp->getSnake();
   }
   void remove()                            //removes the first element
   {  
        node *temp= head->getNext();
        node *t2= head;
        while(temp->getNext()!=NULL)
        {
            temp=temp->getNext();
            t2= t2->getNext();
        }
        t2->setNext(NULL);
        delete temp;
        length--;
   }
   void display()
   {
        int i= 0;
        node *temp= head;
        while(temp!= NULL)
        {
            mvaddch(10,10+i,temp->getSnake().getCh());
            mvprintw(20,5,"THIS LOVE");
            refresh();
            temp= temp->getNext();
        }
   }
   ~list()
   {
        while(head!= NULL)
        {
            node* n= head;
            head= head->getNext();
            delete n;
        }
        length= 0;
    }
};

class game
{   
    int score, max_y, max_x, food_x, food_y, direction;
    list l;
    public:
    game()
    {
        score= max_y= max_x= food_x= food_y= 0;
        direction= 2;
    }
    void launch();
    void play();
    void map();
    void genFood();
    void setSnake();
    void moveSnake(int dir);
    bool check();
};

void game:: launch()
{
    initscr();                      //initialize ncurses
    cbreak();                       //no line buffering
    curs_set(FALSE);
    keypad(stdscr, TRUE);
    noecho();
    refresh();
    attron(A_BOLD);
    mvprintw(LINES/2-2, COLS/2-10, "SNAKES WELCOMES YOU");
    mvprintw(LINES/2, COLS/2-12, "PRESS ENTER TO CONTINUE");
    int ch;
    if((ch= getch())==10)
    {
        getmaxyx(stdscr, max_y, max_x);
        play();
    }
    else 
    {
        clear();
        mvprintw(LINES/2, COLS/2- 16, "YOU DID NOT EVEN GIVE ME A CHANCE......GOODBYE");
        refresh();
        sleep(2);
    }
    attroff(A_BOLD);
    endwin();
}

void game:: genFood()
{
    srand(time(NULL));
    food_y= random()%(max_y-6)+4;
    food_x= random()%(max_x-4)+2;
}

void game:: map()
{
    box(stdscr, 0, 0);
    mvprintw(1,1, "SCORE: ");
    mvprintw(1,8, "%d",score);      
    mvprintw(2,1, "Press q to quit");
    refresh();
}

void game:: setSnake()
{
    clear();
    map();
    for(int i= 0; i< 8; i++)
    {
        snake s((COLS/2)-8+i, LINES/2);
        l.add(s);
        mvprintw(s.getY(),s.getX(),"%c",s.getCh());
    }
    mvprintw(food_y, food_x, "F");
    refresh();
}

void game::play()
{
    bool b;
    genFood();
    setSnake();
    mvprintw(8, COLS/2-20, "??????????READY??????????????");
    refresh();
    sleep(1);
    int ch= 0;
    timeout(50);                            //adjust speed of the game
    while((ch=getch())!= 'q')
    {
        switch(ch)
        {
            case KEY_UP: direction= 1; 
                        break;
            case KEY_DOWN: direction= 3;
                        break;
            case KEY_RIGHT: direction= 2;
                        break;
            case KEY_LEFT: direction= 4;
                        break;
        }
       clear();
       map();
       mvprintw(food_y, food_x,"F");
       moveSnake(direction);
       refresh();
       b= check();
       if(!b)
       {
            clear();
            mvprintw(max_y/2-2, max_x/2-8, "GAME OVER");
            mvprintw(max_y/2, max_x/2-10, "YOUR SCORE %d",score);
            refresh();
            sleep(2);
            break;
        }
    }
}

bool game:: check()
{
    bool b= false;
    snake s= l.get(1);
    int head_x= s.getX();
    int head_y= s.getY();
    if(head_x== max_x-1 || head_y== 1 || head_x== 1 || head_y== max_y-1)
        return false;
    else 
    {
        int len= l.listLength();
        for(int i= 4; i< len; i++)
        {
            s= l.get(i);
            if(head_x== s.getX() && head_y== s.getY())
            {
                b= true;
                break;
            }
        }
        if(b)
            return false;
        else
            return true;
    }  
}

void game:: moveSnake(int dir)
{
    snake k;
    k= l.get(1);
    int x= k.getX(), y= k.getY();
    //mvprintw(6,1, "BEFORE x= %d, y= %d, dir=%d",x,y,dir);
    if(dir== 1) y--;
    else if(dir== 2)    x++;
    else if(dir== 3)    y++;
    else x--;
    //mvprintw(7,1, "AFTER x=%d, y= %d", x, y);
    snake s(x,y);
    l.add(s);
    if(x==food_x && y==food_y)
    {
        score++;
        genFood();
        mvprintw(food_y, food_x, "F");
    }
    else
        l.remove();
    int len= l.listLength();
    for(int i= 1; i<= len; i++)
    {
        k= l.get(i);
        mvaddch(k.getY(), k.getX(), k.getCh());
        refresh();
    }
}

int main()
{
    game g;
    g.launch();
    return 0;
}    
share|improve this question
1  
Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. – Vogel612 Feb 28 at 14:36
up vote 16 down vote accepted
    if(b)
        return false;
    else
        return true;

With the ! operator, you can negate a boolean value.

So by saying !b, you can swap the cases around:

    if(!b)
        return true;
    else
        return false;

But then you have "if it is true that b is not true, return true, else, return false". At that point, you might as well return "whether it is true that b is not true".

Like so:

    return !b;

Aside from that, maybe b isn't really the right name. How about collided? Or maybe collisionFound.

bool game:: check()
{
    bool collisionFound= false;
    snake s= l.get(1);
    int head_x= s.getX();
    int head_y= s.getY();
    if(head_x== max_x-1 || head_y== 1 || head_x== 1 || head_y== max_y-1)
        return false;
    else 
    {
        int len= l.listLength();
        for(int i= 4; i< len; i++)
        {
            s= l.get(i);
            if(head_x== s.getX() && head_y== s.getY())
            {
                collisionFound = true;
                break;
            }
        }
        return !collisionFound;
    }  
}

Maybe you should label the function checkCollision. It's weird that you're returning a negative, though. Let's see where you're using this...

b= check();
if(!b)

Huh...

There's no need to store to b here, you can just say if(!checkCollision()). But that looks weird, game-over if not collision. It's because you've made check more of a check if snake is okay function, but it really checks for collisions and then negates the result. So you'd be better off by removing the negation:

bool game:: checkForCollision()
{
    bool collisionFound= false;
    snake s= l.get(1);
    int head_x= s.getX();
    int head_y= s.getY();
    if(head_x== max_x-1 || head_y== 1 || head_x== 1 || head_y== max_y-1)
        return true;
    else 
    {
        int len= l.listLength();
        for(int i= 4; i< len; i++)
        {
            s= l.get(i);
            if(head_x== s.getX() && head_y== s.getY())
            {
                collisionFound = true;
                break;
            }
        }
        return collisionFound;
    }  
}

And then using like so:

   if(checkForCollision())
   {
        clear();
        mvprintw(max_y/2-2, max_x/2-8, "GAME OVER");
        mvprintw(max_y/2, max_x/2-10, "YOUR SCORE %d",score);
        refresh();
        sleep(2);
        break;
    }

If you make enough use of descriptive function names and variable names, eventually the code will read like a weird form of English.


Here you start checking snake segments starting from segment 4.

    int len= l.listLength();
    for(int i= 4; i< len; i++)

I get the logic behind it: Snake is shaped like so:

. = not snake
o = snake head
^<>v = snake body

....
.v<.
.o^.
....

It's not possible to have a collision with indexes 1, 2 and 3, right? Only a snake of a head and 4 segments or longer could collide, so why not save some performance by only checking collisions from segment 4 and onwards?

Except... what happens if we put the snake in reverse?

You get one crumpled up snake.

....
.ô<.
.^..
....

Poor ekans.

Jokes aside, you should come up with something for this case. Some snake games I played would give me an instant gameover, and some snake games I played wouldn't allow me to move backwards (because snake cannot move backwards). Not allowing the player to move backwards might be more friendly to the player, because if the player presses the wrong button they might ekans by accident.


        case KEY_UP: direction= 1; 
                    break;
        case KEY_DOWN: direction= 3;
                    break;
        case KEY_RIGHT: direction= 2;
                    break;
        case KEY_LEFT: direction= 4;
                    break;

Imagine you didn't have KEY_UP, KEY_DOWN, KEY_RIGHT and KEY_LEFT. Your code would look like this:

        case 38: direction= 1; 
                    break;
        case 40: direction= 3;
                    break;
        case 39: direction= 2;
                    break;
        case 37: direction= 4;
                    break;

That'd be hard to understand, wouldn't it!

Except... you've kinda done the same thing with direction. You should try defining constants for directions. Let's use an enum for this.

So define your directions at the top of your code:

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

Declare the game's direction variable to be a Direction:

Direction direction;

And use like so:

        case KEY_UP: direction= UP; 
                    break;
        case KEY_DOWN: direction= DOWN;
                    break;
        case KEY_RIGHT: direction= RIGHT;
                    break;
        case KEY_LEFT: direction= LEFT;
                    break;

(if you keep your classes and enums in different files, you may need to access it with ::, like Direction::LEFT).

if(dir== UP) y--;
else if(dir== RIGHT)    x++;
else if(dir== DOWN)    y++;
else x--;
share|improve this answer
    
Nice advice. Would not have spotted that. Thank You! If I had a higher rep you would have had an upvote. – user3762742 Feb 26 at 14:02
    
@user3762742 appended answer with ekans =). You should be able to upvote once you reach 15 reputation, which ought happen soon enough. – Pimgd Feb 26 at 14:06
1  
Yes, collision is possible only after the 4th index. And reverse ekans was one of the problems I was able to find with my implementation. But just finished this code, was really excited and posted for review. I will come up with something for this case. – user3762742 Feb 26 at 14:12
1  
@user3762742 well, I added more and more stuff to my review, gonna leave the rest of the code for other reviewers otherwise I might crash it into itself, saying things twice. If you've gotten a few answers and applied the feedback in your code you could post a new questions with the updated code, if you wanted to. I'd be happy to review it. Just gotta give the other reviewers a chance to take a look at your code (a day or so). – Pimgd Feb 26 at 14:31
    
Those are some nice advices. +1 for rhetorical questions – Dex' ter Feb 26 at 18:26

Efficiency

In "list" class, you implemented linked list behaviors, which has provides O(N) time complexity for get and add functions. You could use C++ containers, such as vector, which provides constant time for your add/remove/get functions, see push_back, erase, pop_back...functions.

Naming

  1. The snake class actually represents part of a snake; it is better to name it as snake_fragment.
  2. map is also a container in C++. This may cause problem when you try to use std::map without specifying name resolution. Avoid using C++ keyword and container as your function names.

    void game:: map()

Design limitation

  1. There is only 1 food can be provided in the game at once. It is possible to provide multiple sets of food at once by using array or vector.

food_x, food_y

  1. Many hard-coded items makes it less flexible to change or extend. What if you want longer or shorter snake? Does it always have to start from certain point? >
    for(int i= 0; i< 8; i++)
    

    { snake s((COLS/2)-8+i, LINES/2); l.add(s); mvprintw(s.getY(),s.getX(),"%c",s.getCh()); }

Implementation

It seems only the first fragment of the snake is moved.

k= l.get(1);

Coding style

  1. Magic number. What does "10" mean here? Why -6, +4...?

if((ch= getch())==10)

food_y= random()%(max_y-6)+4; food_x= random()%(max_x-4)+2;

mvprintw(1,8, "%d",score);

  1. No space after name resolution operator.

    void game:: map()

  2. Meaningless variable. What does b mean?

    bool b= false;

  3. Why not consolidate x,y as a "point" data type so that the comparison is more understandable? Also, you can implement functions, such as moveRight, moveLeft... and leave the "x++", "y++"...implementation details to "snake" or "point" class? Putting "x++"...details in game class is not good for data encapsulation; game class does not move right or move left.

int head_x= s.getX(); int head_y= s.getY(); if(head_x== max_x-1 || head_y== 1 || head_x== 1 || head_y== max_y-1)

share|improve this answer
    
Welcome to Code Review! Good job on your first answer. Please consider fixing the code formatting of your answer. – SirPython Feb 28 at 2:21
    
I know about the C++ containers but I just wanted to implement my version of list( was bored). In the segment coding style you mention my use of magic nos, do I have to name every constant I use in my code( there are quite a few "magic nos" used)? Is that the standard practice? In "Implementation", can you please elaborate? – user3762742 Feb 28 at 13:10
    
@user3762742 - Yes, naming all constants is a good practice. – Piotr Aleksander Chmielowski Feb 28 at 17:41
    
About "implementation", if I understand correctly, "l" stores snake bpdy parts. it seems you only updated the first part of snake body. – chiungyi Feb 29 at 3:07

This is the updated version of my code. Used a lot of the advice given to me by @Pimgd. Changed/improved some of the function and variable names. Simplified some functions. Made the game a little for friendly. Doesn't print game over when you want to go backwards.

#include<ncurses.h>
#include<unistd.h>
#include<time.h>
#include<stdlib.h>

enum Direction {Up=1 , Right, Down, Left};      //added to improve understanding of the code

class snake                         //to store the x and y coordinates of each snake part
{
    int x, y;
    char ch;
    public:
    snake(){ x= y= 0; ch='O';}
    snake(int a, int b)
    {
        x= a; y= b; ch= 'O';
    }
    snake(const snake &ekans)
    {
        ch= ekans.ch;
        x= ekans.x;
        y= ekans.y;
    }
    void setCh(char x)
    {
        ch= x;
    }
    char getCh()
    {
        return ch;
    }
    int getX()
    {
        return x;
    }
    void setX(int no)
    {
        x= no;
    }
    int getY()
    {
        return y;
    }
    void setY(int no)
    {
        y= no;
    }
};

class node                          //to make a linked list
{   
    node* next;
    snake s;
    public:
    snake getSnake()
    {
        return s;
    }  
    void setSnake(snake ekans)
    {
        s= ekans;
    }
    void setNext(node *n)
    {
        next= n;
    }
    node* getNext()
    {
        return next;
    }
};

class list                          //to store each snake body          
{
    node* head;
    int length;
    public:
    list()
    {
        head= NULL;
        length= 0;
    }
    void add(snake s)                   //adds at the end
    {
        node *n= new node;
        n->setSnake(s);
        n->setNext(head);
        head= n;
        length++;
    }
    int listLength()
    {
        return length;
    }
    snake get(int n)
    {
        node *temp =head;
        int count= 1;
        while(count!= n && temp!= NULL)
        {
            count++;
            temp= temp->getNext();
        } 
        return temp->getSnake();
    }
    void remove()                           //removes the first element
    {
        node *temp= head->getNext();
        node *t2= head;
        while(temp->getNext()!=NULL)
        {
            temp=temp->getNext();
            t2= t2->getNext();
        }
        t2->setNext(NULL);
        delete temp;
        length--;
    }
    void display()
    {
        int i= 0;
        node *temp= head;
        while(temp!= NULL)
        {
            mvaddch(10,10+i,temp->getSnake().getCh());
            refresh();
            temp= temp->getNext();
        }
    }
    ~list()
    {
        while(head!= NULL)
        {
            node* n= head;
            head= head->getNext();
            delete n;
        }
        length= 0;
    }
};

class game
{   
    int score, max_y, max_x, food_x, food_y;// direction, prev_dir;
    Direction direction, prev_dir;
    list l;
    public:
    game()
    {
        score= max_y= max_x= food_x= food_y= 0;
        prev_dir= direction= Right;
    }
    void launch();
    void play();
    void map();
    void genFood();
    void setSnake();
    void moveSnake(Direction dir);
    bool checkForCollision();       //changed
};

void game:: launch()
{
    initscr();                      //initialize ncurses
    cbreak();                       //no line buffering
    curs_set(FALSE);
    keypad(stdscr, TRUE);
    noecho();
    refresh();
    attron(A_BOLD);
    mvprintw(LINES/2-2, COLS/2-10, "SNAKES WELCOMES YOU");
    mvprintw(LINES/2, COLS/2-12, "PRESS ENTER TO CONTINUE");
    int ch;
    if((ch= getch())==10)
    {
        getmaxyx(stdscr, max_y, max_x);
        play();
    }
    else 
    {
        clear();
        mvprintw(LINES/2, COLS/2- 16, "YOU DID NOT EVEN GIVE ME A CHANCE......GOODBYE");
        refresh();
        sleep(2);
    }
    attroff(A_BOLD);
    endwin();
}  

void game:: genFood()
{
    srand(time(NULL));
    food_y= random()%(max_y-6)+4;
    food_x= random()%(max_x-4)+2;
}

void game:: map()
{
    box(stdscr, 0, 0);
    mvprintw(1,1, "SCORE: ");
    mvprintw(1,8, "%d",score);      
    mvprintw(2,1, "Press q to quit");
    refresh();
}

void game:: setSnake()
{
    clear();
    map();
    for(int i= 0; i< 8; i++)
    {
        snake s((COLS/2)-8+i, LINES/2);
        l.add(s);
        mvprintw(s.getY(),s.getX(),"%c",s.getCh());
    }
    mvprintw(food_y, food_x, "F");
    refresh();
}

void game::play()
{
    genFood();
    setSnake();
    mvprintw(8, COLS/2-20, "??????????READY??????????????");
    refresh();
    sleep(1);
    int ch= 0;
    timeout(50);                            //adjust speed of the game
    while((ch=getch())!= 'q')
    {
        switch(ch)
        {
            case KEY_UP: direction= Up; 
                    break;
            case KEY_DOWN: direction= Down;
                    break;
            case KEY_RIGHT: direction= Right;
                    break;
            case KEY_LEFT: direction= Left;
                    break;
        }
        clear();
        map();
        mvprintw(food_y, food_x,"F");
        if(direction+2== prev_dir || prev_dir+2== direction)            //added does not allow snake to go backwards
            direction= prev_dir;
        moveSnake(direction);
        refresh();
        if(checkForCollision())
        {
            clear();
            mvprintw(max_y/2-2, max_x/2-8, "GAME OVER");
            mvprintw(max_y/2, max_x/2-10, "YOUR SCORE %d",score);
            refresh();
            sleep(2);
            break;
        }
        prev_dir= direction;
    }
}

bool game:: checkForCollision()
{
    bool collided= false;
    snake s= l.get(1);
    int head_x= s.getX();
    int head_y= s.getY();
    if(head_x== max_x-1 || head_y== 1 || head_x== 1 || head_y== max_y-1)
        return true;
    else 
    {
        int len= l.listLength();
        for(int i= 4; i< len; i++)
        {
            s= l.get(i);
            if(head_x== s.getX() && head_y== s.getY())
            {
                collided= true;
                break;
            }
        }
            return collided;
    }   
}   

void game:: moveSnake(Direction dir)
{
    snake k;
    k= l.get(1);
    int x= k.getX(), y= k.getY();
    //mvprintw(6,1, "BEFORE x= %d, y= %d, dir=%d",x,y,dir);
    //getch();
    if(dir== Up)    y--;
    else if(dir== Left) x--;
    else if(dir== Down) y++;
    else x++;
    //mvprintw(7,1, "AFTER x=%d, y= %d", x, y);
    snake s(x,y);
    l.add(s);
    if(x==food_x && y==food_y)
    {
        score++; 
        genFood();
        mvprintw(food_y, food_x, "F");
    }
    else
        l.remove();
    int len= l.listLength();
    for(int i= 1; i<= len; i++)
    {
        k= l.get(i);
        mvaddch(k.getY(), k.getX(), k.getCh());
        refresh();
    }
}       

int main()
{
    game g;
    g.launch();
    return 0;
}   
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.