Snake++ game (in C++ with SDL)












2












$begingroup$


Yes, It's called Snake++. I build it to learn C++.
What features and techniques can be improved? I used SDL for some basic rendering, but my concern is more about the language use.



Things I'm very concerned about:




  1. Generating a new food means trying random positions over and over again till one is free. This is going to become a problem very soon. What data structures can I use here?

  2. Am I using references to their full potential and avoiding unnecessary copying?


Main.cpp



#include <iostream>
#include "Game.hpp"

using namespace std;

int main(int argc, char * argv)
{
Game game = Game();
Game().Run();
cout << "Game has terminated successfully, score: " << game.GetScore()
<< ", size: " << game.GetSize() << endl;
return 0;
}


Game.hpp



#pragma once
#include <vector>
#include "SDL.h"
#include "SDL_image.h"

class Game
{

public:

Game();
void Run();
int GetScore();
int GetSize();

private:

bool running = false;
bool alive = false;
int fps = 0;

static const int FRAME_RATE = 1000 / 60;
static const int SCREEN_WIDTH = 640;
static const int SCREEN_HEIGHT = 640;
static const int GRID_WIDTH = 32;
static const int GRID_HEIGHT = 32;

SDL_Window * window = nullptr;
SDL_Renderer * renderer = nullptr;

enum class Block { head, body, food, empty };
enum class Move { up, down, left, right };

Move last_dir = Move::up;
Move dir = Move::up;

struct { float x = GRID_WIDTH / 2, y = GRID_HEIGHT / 2; } pos;

SDL_Point head = { static_cast<int>(pos.x), static_cast<int>(pos.y) };
SDL_Point food;
std::vector<SDL_Point> body;

Block grid[GRID_WIDTH][GRID_HEIGHT];

float speed = 0.5f;
int growing = 0;
int score = 0;
int size = 1;

void ReplaceFood();
void GrowBody(int quantity);
void UpdateWindowTitle();
void GameLoop();
void Render();
void Update();
void PollEvents();
void Close();

};


Game.cpp



#include <iostream>
#include <string>
#include <ctime>

#include "SDL.h"
#include "Game.hpp"

using namespace std;

Game::Game()
{
for (int i = 0; i < GRID_WIDTH; ++i)
for (int j = 0; j < GRID_HEIGHT; ++j)
{
grid[i][j] = Block::empty;
}

srand(static_cast<unsigned int>(time(0)));
}

void Game::Run()
{
// Initialize SDL
if (SDL_Init(SDL_INIT_VIDEO) < 0)
{
cerr << "SDL could not initialize! SDL_Error: " << SDL_GetError() << endl;
exit(EXIT_FAILURE);
}

// Create Window
window = SDL_CreateWindow("Snake Game", SDL_WINDOWPOS_CENTERED, SDL_WINDOWPOS_CENTERED,
SCREEN_WIDTH, SCREEN_HEIGHT, SDL_WINDOW_SHOWN);

if (window == NULL)
{
cout << "Window could not be created! SDL_Error: " << SDL_GetError() << endl;
exit(EXIT_FAILURE);
}

// Create renderer
renderer = SDL_CreateRenderer(window, -1, SDL_RENDERER_ACCELERATED);
if (renderer == NULL)
{
cout << "Renderer could not be created! SDL_Error: " << SDL_GetError() << endl;
exit(EXIT_FAILURE);
}

alive = true;
running = true;
ReplaceFood();
GameLoop();
}

void Game::ReplaceFood()
{
int x, y;
while (true)
{
x = rand() % GRID_WIDTH;
y = rand() % GRID_HEIGHT;

if (grid[x][y] == Block::empty)
{
grid[x][y] = Block::food;
food.x = x;
food.y = y;
break;
}
}
}

void Game::GameLoop()
{
Uint32 before, second = SDL_GetTicks(), after;
int frame_time, frames = 0;

while (running)
{
before = SDL_GetTicks();

PollEvents();
Update();
Render();

frames++;
after = SDL_GetTicks();
frame_time = after - before;

if (after - second >= 1000)
{
fps = frames;
frames = 0;
second = after;
UpdateWindowTitle();
}

if (FRAME_RATE > frame_time)
{
SDL_Delay(FRAME_RATE - frame_time);
}
}

}

void Game::PollEvents()
{
SDL_Event e;
while (SDL_PollEvent(&e))
{
if (e.type == SDL_QUIT)
{
running = false;
}
else if (e.type == SDL_KEYDOWN)
{
switch (e.key.keysym.sym)
{
case SDLK_UP:
if (last_dir != Move::down || size == 1)
dir = Move::up;
break;

case SDLK_DOWN:
if (last_dir != Move::up || size == 1)
dir = Move::down;
break;

case SDLK_LEFT:
if (last_dir != Move::right || size == 1)
dir = Move::left;
break;

case SDLK_RIGHT:
if (last_dir != Move::left || size == 1)
dir = Move::right;
break;
}
}
}
}

int Game::GetSize()
{
return size;
}

void Game::GrowBody(int quantity)
{
growing += quantity;
}

void Game::Update()
{
if (!alive)
return;

switch (dir)
{
case Move::up:
pos.y -= speed;
pos.x = floorf(pos.x);
break;

case Move::down:
pos.y += speed;
pos.x = floorf(pos.x);
break;

case Move::left:
pos.x -= speed;
pos.y = floorf(pos.y);
break;

case Move::right:
pos.x += speed;
pos.y = floorf(pos.y);
break;
}

// Wrap
if (pos.x < 0) pos.x = GRID_WIDTH - 1;
else if (pos.x > GRID_WIDTH - 1) pos.x = 0;

if (pos.y < 0) pos.y = GRID_HEIGHT - 1;
else if (pos.y > GRID_HEIGHT - 1) pos.y = 0;

int new_x = static_cast<int>(pos.x);
int new_y = static_cast<int>(pos.y);

// Check if head position has changed
if (new_x != head.x || new_y != head.y)
{
last_dir = dir;

// If we are growing, just make a new neck
if (growing > 0)
{
size++;
body.push_back(head);
growing--;
grid[head.x][head.y] = Block::body;
}
else
{
// We need to shift the body
SDL_Point free = head;
vector<SDL_Point>::reverse_iterator rit = body.rbegin();
for ( ; rit != body.rend(); ++rit)
{
grid[free.x][free.y] = Block::body;
swap(*rit, free);
}

grid[free.x][free.y] = Block::empty;
}

}

head.x = new_x;
head.y = new_y;

Block & next = grid[head.x][head.y];
// Check if there's food over here
if (next == Block::food)
{
score++;
ReplaceFood();
GrowBody(1);
}
// Check if we're dead
else if (next == Block::body)
{
alive = false;
}

next = Block::head;
}

int Game::GetScore()
{
return score;
}

void Game::UpdateWindowTitle()
{
string title = "Snakle++ Score: " + to_string(score) + " FPS: " + to_string(fps);
SDL_SetWindowTitle(window, title.c_str());
}

void Game::Render()
{
SDL_Rect block;
block.w = SCREEN_WIDTH / GRID_WIDTH;
block.h = SCREEN_WIDTH / GRID_HEIGHT;

// Clear screen
SDL_SetRenderDrawColor(renderer, 0x1E, 0x1E, 0x1E, 0xFF);
SDL_RenderClear(renderer);

// Render food
SDL_SetRenderDrawColor(renderer, 0xFF, 0xCC, 0x00, 0xFF);
block.x = food.x * block.w;
block.y = food.y * block.h;
SDL_RenderFillRect(renderer, &block);

// Render snake's body
SDL_SetRenderDrawColor(renderer, 0xFF, 0xFF, 0xFF, 0xFF);
for (SDL_Point & point : body)
{
block.x = point.x * block.w;
block.y = point.y * block.h;
SDL_RenderFillRect(renderer, &block);
}

// Render snake's head
block.x = head.x * block.w;
block.y = head.y * block.h;
if (alive) SDL_SetRenderDrawColor(renderer, 0x00, 0x7A, 0xCC, 0xFF);
else SDL_SetRenderDrawColor(renderer, 0xFF, 0x00, 0x00, 0xFF);
SDL_RenderFillRect(renderer, &block);

// Update Screen
SDL_RenderPresent(renderer);
}

void Game::Close()
{
SDL_DestroyWindow(window);
SDL_Quit();
}









share|improve this question











$endgroup$

















    2












    $begingroup$


    Yes, It's called Snake++. I build it to learn C++.
    What features and techniques can be improved? I used SDL for some basic rendering, but my concern is more about the language use.



    Things I'm very concerned about:




    1. Generating a new food means trying random positions over and over again till one is free. This is going to become a problem very soon. What data structures can I use here?

    2. Am I using references to their full potential and avoiding unnecessary copying?


    Main.cpp



    #include <iostream>
    #include "Game.hpp"

    using namespace std;

    int main(int argc, char * argv)
    {
    Game game = Game();
    Game().Run();
    cout << "Game has terminated successfully, score: " << game.GetScore()
    << ", size: " << game.GetSize() << endl;
    return 0;
    }


    Game.hpp



    #pragma once
    #include <vector>
    #include "SDL.h"
    #include "SDL_image.h"

    class Game
    {

    public:

    Game();
    void Run();
    int GetScore();
    int GetSize();

    private:

    bool running = false;
    bool alive = false;
    int fps = 0;

    static const int FRAME_RATE = 1000 / 60;
    static const int SCREEN_WIDTH = 640;
    static const int SCREEN_HEIGHT = 640;
    static const int GRID_WIDTH = 32;
    static const int GRID_HEIGHT = 32;

    SDL_Window * window = nullptr;
    SDL_Renderer * renderer = nullptr;

    enum class Block { head, body, food, empty };
    enum class Move { up, down, left, right };

    Move last_dir = Move::up;
    Move dir = Move::up;

    struct { float x = GRID_WIDTH / 2, y = GRID_HEIGHT / 2; } pos;

    SDL_Point head = { static_cast<int>(pos.x), static_cast<int>(pos.y) };
    SDL_Point food;
    std::vector<SDL_Point> body;

    Block grid[GRID_WIDTH][GRID_HEIGHT];

    float speed = 0.5f;
    int growing = 0;
    int score = 0;
    int size = 1;

    void ReplaceFood();
    void GrowBody(int quantity);
    void UpdateWindowTitle();
    void GameLoop();
    void Render();
    void Update();
    void PollEvents();
    void Close();

    };


    Game.cpp



    #include <iostream>
    #include <string>
    #include <ctime>

    #include "SDL.h"
    #include "Game.hpp"

    using namespace std;

    Game::Game()
    {
    for (int i = 0; i < GRID_WIDTH; ++i)
    for (int j = 0; j < GRID_HEIGHT; ++j)
    {
    grid[i][j] = Block::empty;
    }

    srand(static_cast<unsigned int>(time(0)));
    }

    void Game::Run()
    {
    // Initialize SDL
    if (SDL_Init(SDL_INIT_VIDEO) < 0)
    {
    cerr << "SDL could not initialize! SDL_Error: " << SDL_GetError() << endl;
    exit(EXIT_FAILURE);
    }

    // Create Window
    window = SDL_CreateWindow("Snake Game", SDL_WINDOWPOS_CENTERED, SDL_WINDOWPOS_CENTERED,
    SCREEN_WIDTH, SCREEN_HEIGHT, SDL_WINDOW_SHOWN);

    if (window == NULL)
    {
    cout << "Window could not be created! SDL_Error: " << SDL_GetError() << endl;
    exit(EXIT_FAILURE);
    }

    // Create renderer
    renderer = SDL_CreateRenderer(window, -1, SDL_RENDERER_ACCELERATED);
    if (renderer == NULL)
    {
    cout << "Renderer could not be created! SDL_Error: " << SDL_GetError() << endl;
    exit(EXIT_FAILURE);
    }

    alive = true;
    running = true;
    ReplaceFood();
    GameLoop();
    }

    void Game::ReplaceFood()
    {
    int x, y;
    while (true)
    {
    x = rand() % GRID_WIDTH;
    y = rand() % GRID_HEIGHT;

    if (grid[x][y] == Block::empty)
    {
    grid[x][y] = Block::food;
    food.x = x;
    food.y = y;
    break;
    }
    }
    }

    void Game::GameLoop()
    {
    Uint32 before, second = SDL_GetTicks(), after;
    int frame_time, frames = 0;

    while (running)
    {
    before = SDL_GetTicks();

    PollEvents();
    Update();
    Render();

    frames++;
    after = SDL_GetTicks();
    frame_time = after - before;

    if (after - second >= 1000)
    {
    fps = frames;
    frames = 0;
    second = after;
    UpdateWindowTitle();
    }

    if (FRAME_RATE > frame_time)
    {
    SDL_Delay(FRAME_RATE - frame_time);
    }
    }

    }

    void Game::PollEvents()
    {
    SDL_Event e;
    while (SDL_PollEvent(&e))
    {
    if (e.type == SDL_QUIT)
    {
    running = false;
    }
    else if (e.type == SDL_KEYDOWN)
    {
    switch (e.key.keysym.sym)
    {
    case SDLK_UP:
    if (last_dir != Move::down || size == 1)
    dir = Move::up;
    break;

    case SDLK_DOWN:
    if (last_dir != Move::up || size == 1)
    dir = Move::down;
    break;

    case SDLK_LEFT:
    if (last_dir != Move::right || size == 1)
    dir = Move::left;
    break;

    case SDLK_RIGHT:
    if (last_dir != Move::left || size == 1)
    dir = Move::right;
    break;
    }
    }
    }
    }

    int Game::GetSize()
    {
    return size;
    }

    void Game::GrowBody(int quantity)
    {
    growing += quantity;
    }

    void Game::Update()
    {
    if (!alive)
    return;

    switch (dir)
    {
    case Move::up:
    pos.y -= speed;
    pos.x = floorf(pos.x);
    break;

    case Move::down:
    pos.y += speed;
    pos.x = floorf(pos.x);
    break;

    case Move::left:
    pos.x -= speed;
    pos.y = floorf(pos.y);
    break;

    case Move::right:
    pos.x += speed;
    pos.y = floorf(pos.y);
    break;
    }

    // Wrap
    if (pos.x < 0) pos.x = GRID_WIDTH - 1;
    else if (pos.x > GRID_WIDTH - 1) pos.x = 0;

    if (pos.y < 0) pos.y = GRID_HEIGHT - 1;
    else if (pos.y > GRID_HEIGHT - 1) pos.y = 0;

    int new_x = static_cast<int>(pos.x);
    int new_y = static_cast<int>(pos.y);

    // Check if head position has changed
    if (new_x != head.x || new_y != head.y)
    {
    last_dir = dir;

    // If we are growing, just make a new neck
    if (growing > 0)
    {
    size++;
    body.push_back(head);
    growing--;
    grid[head.x][head.y] = Block::body;
    }
    else
    {
    // We need to shift the body
    SDL_Point free = head;
    vector<SDL_Point>::reverse_iterator rit = body.rbegin();
    for ( ; rit != body.rend(); ++rit)
    {
    grid[free.x][free.y] = Block::body;
    swap(*rit, free);
    }

    grid[free.x][free.y] = Block::empty;
    }

    }

    head.x = new_x;
    head.y = new_y;

    Block & next = grid[head.x][head.y];
    // Check if there's food over here
    if (next == Block::food)
    {
    score++;
    ReplaceFood();
    GrowBody(1);
    }
    // Check if we're dead
    else if (next == Block::body)
    {
    alive = false;
    }

    next = Block::head;
    }

    int Game::GetScore()
    {
    return score;
    }

    void Game::UpdateWindowTitle()
    {
    string title = "Snakle++ Score: " + to_string(score) + " FPS: " + to_string(fps);
    SDL_SetWindowTitle(window, title.c_str());
    }

    void Game::Render()
    {
    SDL_Rect block;
    block.w = SCREEN_WIDTH / GRID_WIDTH;
    block.h = SCREEN_WIDTH / GRID_HEIGHT;

    // Clear screen
    SDL_SetRenderDrawColor(renderer, 0x1E, 0x1E, 0x1E, 0xFF);
    SDL_RenderClear(renderer);

    // Render food
    SDL_SetRenderDrawColor(renderer, 0xFF, 0xCC, 0x00, 0xFF);
    block.x = food.x * block.w;
    block.y = food.y * block.h;
    SDL_RenderFillRect(renderer, &block);

    // Render snake's body
    SDL_SetRenderDrawColor(renderer, 0xFF, 0xFF, 0xFF, 0xFF);
    for (SDL_Point & point : body)
    {
    block.x = point.x * block.w;
    block.y = point.y * block.h;
    SDL_RenderFillRect(renderer, &block);
    }

    // Render snake's head
    block.x = head.x * block.w;
    block.y = head.y * block.h;
    if (alive) SDL_SetRenderDrawColor(renderer, 0x00, 0x7A, 0xCC, 0xFF);
    else SDL_SetRenderDrawColor(renderer, 0xFF, 0x00, 0x00, 0xFF);
    SDL_RenderFillRect(renderer, &block);

    // Update Screen
    SDL_RenderPresent(renderer);
    }

    void Game::Close()
    {
    SDL_DestroyWindow(window);
    SDL_Quit();
    }









    share|improve this question











    $endgroup$















      2












      2








      2


      1



      $begingroup$


      Yes, It's called Snake++. I build it to learn C++.
      What features and techniques can be improved? I used SDL for some basic rendering, but my concern is more about the language use.



      Things I'm very concerned about:




      1. Generating a new food means trying random positions over and over again till one is free. This is going to become a problem very soon. What data structures can I use here?

      2. Am I using references to their full potential and avoiding unnecessary copying?


      Main.cpp



      #include <iostream>
      #include "Game.hpp"

      using namespace std;

      int main(int argc, char * argv)
      {
      Game game = Game();
      Game().Run();
      cout << "Game has terminated successfully, score: " << game.GetScore()
      << ", size: " << game.GetSize() << endl;
      return 0;
      }


      Game.hpp



      #pragma once
      #include <vector>
      #include "SDL.h"
      #include "SDL_image.h"

      class Game
      {

      public:

      Game();
      void Run();
      int GetScore();
      int GetSize();

      private:

      bool running = false;
      bool alive = false;
      int fps = 0;

      static const int FRAME_RATE = 1000 / 60;
      static const int SCREEN_WIDTH = 640;
      static const int SCREEN_HEIGHT = 640;
      static const int GRID_WIDTH = 32;
      static const int GRID_HEIGHT = 32;

      SDL_Window * window = nullptr;
      SDL_Renderer * renderer = nullptr;

      enum class Block { head, body, food, empty };
      enum class Move { up, down, left, right };

      Move last_dir = Move::up;
      Move dir = Move::up;

      struct { float x = GRID_WIDTH / 2, y = GRID_HEIGHT / 2; } pos;

      SDL_Point head = { static_cast<int>(pos.x), static_cast<int>(pos.y) };
      SDL_Point food;
      std::vector<SDL_Point> body;

      Block grid[GRID_WIDTH][GRID_HEIGHT];

      float speed = 0.5f;
      int growing = 0;
      int score = 0;
      int size = 1;

      void ReplaceFood();
      void GrowBody(int quantity);
      void UpdateWindowTitle();
      void GameLoop();
      void Render();
      void Update();
      void PollEvents();
      void Close();

      };


      Game.cpp



      #include <iostream>
      #include <string>
      #include <ctime>

      #include "SDL.h"
      #include "Game.hpp"

      using namespace std;

      Game::Game()
      {
      for (int i = 0; i < GRID_WIDTH; ++i)
      for (int j = 0; j < GRID_HEIGHT; ++j)
      {
      grid[i][j] = Block::empty;
      }

      srand(static_cast<unsigned int>(time(0)));
      }

      void Game::Run()
      {
      // Initialize SDL
      if (SDL_Init(SDL_INIT_VIDEO) < 0)
      {
      cerr << "SDL could not initialize! SDL_Error: " << SDL_GetError() << endl;
      exit(EXIT_FAILURE);
      }

      // Create Window
      window = SDL_CreateWindow("Snake Game", SDL_WINDOWPOS_CENTERED, SDL_WINDOWPOS_CENTERED,
      SCREEN_WIDTH, SCREEN_HEIGHT, SDL_WINDOW_SHOWN);

      if (window == NULL)
      {
      cout << "Window could not be created! SDL_Error: " << SDL_GetError() << endl;
      exit(EXIT_FAILURE);
      }

      // Create renderer
      renderer = SDL_CreateRenderer(window, -1, SDL_RENDERER_ACCELERATED);
      if (renderer == NULL)
      {
      cout << "Renderer could not be created! SDL_Error: " << SDL_GetError() << endl;
      exit(EXIT_FAILURE);
      }

      alive = true;
      running = true;
      ReplaceFood();
      GameLoop();
      }

      void Game::ReplaceFood()
      {
      int x, y;
      while (true)
      {
      x = rand() % GRID_WIDTH;
      y = rand() % GRID_HEIGHT;

      if (grid[x][y] == Block::empty)
      {
      grid[x][y] = Block::food;
      food.x = x;
      food.y = y;
      break;
      }
      }
      }

      void Game::GameLoop()
      {
      Uint32 before, second = SDL_GetTicks(), after;
      int frame_time, frames = 0;

      while (running)
      {
      before = SDL_GetTicks();

      PollEvents();
      Update();
      Render();

      frames++;
      after = SDL_GetTicks();
      frame_time = after - before;

      if (after - second >= 1000)
      {
      fps = frames;
      frames = 0;
      second = after;
      UpdateWindowTitle();
      }

      if (FRAME_RATE > frame_time)
      {
      SDL_Delay(FRAME_RATE - frame_time);
      }
      }

      }

      void Game::PollEvents()
      {
      SDL_Event e;
      while (SDL_PollEvent(&e))
      {
      if (e.type == SDL_QUIT)
      {
      running = false;
      }
      else if (e.type == SDL_KEYDOWN)
      {
      switch (e.key.keysym.sym)
      {
      case SDLK_UP:
      if (last_dir != Move::down || size == 1)
      dir = Move::up;
      break;

      case SDLK_DOWN:
      if (last_dir != Move::up || size == 1)
      dir = Move::down;
      break;

      case SDLK_LEFT:
      if (last_dir != Move::right || size == 1)
      dir = Move::left;
      break;

      case SDLK_RIGHT:
      if (last_dir != Move::left || size == 1)
      dir = Move::right;
      break;
      }
      }
      }
      }

      int Game::GetSize()
      {
      return size;
      }

      void Game::GrowBody(int quantity)
      {
      growing += quantity;
      }

      void Game::Update()
      {
      if (!alive)
      return;

      switch (dir)
      {
      case Move::up:
      pos.y -= speed;
      pos.x = floorf(pos.x);
      break;

      case Move::down:
      pos.y += speed;
      pos.x = floorf(pos.x);
      break;

      case Move::left:
      pos.x -= speed;
      pos.y = floorf(pos.y);
      break;

      case Move::right:
      pos.x += speed;
      pos.y = floorf(pos.y);
      break;
      }

      // Wrap
      if (pos.x < 0) pos.x = GRID_WIDTH - 1;
      else if (pos.x > GRID_WIDTH - 1) pos.x = 0;

      if (pos.y < 0) pos.y = GRID_HEIGHT - 1;
      else if (pos.y > GRID_HEIGHT - 1) pos.y = 0;

      int new_x = static_cast<int>(pos.x);
      int new_y = static_cast<int>(pos.y);

      // Check if head position has changed
      if (new_x != head.x || new_y != head.y)
      {
      last_dir = dir;

      // If we are growing, just make a new neck
      if (growing > 0)
      {
      size++;
      body.push_back(head);
      growing--;
      grid[head.x][head.y] = Block::body;
      }
      else
      {
      // We need to shift the body
      SDL_Point free = head;
      vector<SDL_Point>::reverse_iterator rit = body.rbegin();
      for ( ; rit != body.rend(); ++rit)
      {
      grid[free.x][free.y] = Block::body;
      swap(*rit, free);
      }

      grid[free.x][free.y] = Block::empty;
      }

      }

      head.x = new_x;
      head.y = new_y;

      Block & next = grid[head.x][head.y];
      // Check if there's food over here
      if (next == Block::food)
      {
      score++;
      ReplaceFood();
      GrowBody(1);
      }
      // Check if we're dead
      else if (next == Block::body)
      {
      alive = false;
      }

      next = Block::head;
      }

      int Game::GetScore()
      {
      return score;
      }

      void Game::UpdateWindowTitle()
      {
      string title = "Snakle++ Score: " + to_string(score) + " FPS: " + to_string(fps);
      SDL_SetWindowTitle(window, title.c_str());
      }

      void Game::Render()
      {
      SDL_Rect block;
      block.w = SCREEN_WIDTH / GRID_WIDTH;
      block.h = SCREEN_WIDTH / GRID_HEIGHT;

      // Clear screen
      SDL_SetRenderDrawColor(renderer, 0x1E, 0x1E, 0x1E, 0xFF);
      SDL_RenderClear(renderer);

      // Render food
      SDL_SetRenderDrawColor(renderer, 0xFF, 0xCC, 0x00, 0xFF);
      block.x = food.x * block.w;
      block.y = food.y * block.h;
      SDL_RenderFillRect(renderer, &block);

      // Render snake's body
      SDL_SetRenderDrawColor(renderer, 0xFF, 0xFF, 0xFF, 0xFF);
      for (SDL_Point & point : body)
      {
      block.x = point.x * block.w;
      block.y = point.y * block.h;
      SDL_RenderFillRect(renderer, &block);
      }

      // Render snake's head
      block.x = head.x * block.w;
      block.y = head.y * block.h;
      if (alive) SDL_SetRenderDrawColor(renderer, 0x00, 0x7A, 0xCC, 0xFF);
      else SDL_SetRenderDrawColor(renderer, 0xFF, 0x00, 0x00, 0xFF);
      SDL_RenderFillRect(renderer, &block);

      // Update Screen
      SDL_RenderPresent(renderer);
      }

      void Game::Close()
      {
      SDL_DestroyWindow(window);
      SDL_Quit();
      }









      share|improve this question











      $endgroup$




      Yes, It's called Snake++. I build it to learn C++.
      What features and techniques can be improved? I used SDL for some basic rendering, but my concern is more about the language use.



      Things I'm very concerned about:




      1. Generating a new food means trying random positions over and over again till one is free. This is going to become a problem very soon. What data structures can I use here?

      2. Am I using references to their full potential and avoiding unnecessary copying?


      Main.cpp



      #include <iostream>
      #include "Game.hpp"

      using namespace std;

      int main(int argc, char * argv)
      {
      Game game = Game();
      Game().Run();
      cout << "Game has terminated successfully, score: " << game.GetScore()
      << ", size: " << game.GetSize() << endl;
      return 0;
      }


      Game.hpp



      #pragma once
      #include <vector>
      #include "SDL.h"
      #include "SDL_image.h"

      class Game
      {

      public:

      Game();
      void Run();
      int GetScore();
      int GetSize();

      private:

      bool running = false;
      bool alive = false;
      int fps = 0;

      static const int FRAME_RATE = 1000 / 60;
      static const int SCREEN_WIDTH = 640;
      static const int SCREEN_HEIGHT = 640;
      static const int GRID_WIDTH = 32;
      static const int GRID_HEIGHT = 32;

      SDL_Window * window = nullptr;
      SDL_Renderer * renderer = nullptr;

      enum class Block { head, body, food, empty };
      enum class Move { up, down, left, right };

      Move last_dir = Move::up;
      Move dir = Move::up;

      struct { float x = GRID_WIDTH / 2, y = GRID_HEIGHT / 2; } pos;

      SDL_Point head = { static_cast<int>(pos.x), static_cast<int>(pos.y) };
      SDL_Point food;
      std::vector<SDL_Point> body;

      Block grid[GRID_WIDTH][GRID_HEIGHT];

      float speed = 0.5f;
      int growing = 0;
      int score = 0;
      int size = 1;

      void ReplaceFood();
      void GrowBody(int quantity);
      void UpdateWindowTitle();
      void GameLoop();
      void Render();
      void Update();
      void PollEvents();
      void Close();

      };


      Game.cpp



      #include <iostream>
      #include <string>
      #include <ctime>

      #include "SDL.h"
      #include "Game.hpp"

      using namespace std;

      Game::Game()
      {
      for (int i = 0; i < GRID_WIDTH; ++i)
      for (int j = 0; j < GRID_HEIGHT; ++j)
      {
      grid[i][j] = Block::empty;
      }

      srand(static_cast<unsigned int>(time(0)));
      }

      void Game::Run()
      {
      // Initialize SDL
      if (SDL_Init(SDL_INIT_VIDEO) < 0)
      {
      cerr << "SDL could not initialize! SDL_Error: " << SDL_GetError() << endl;
      exit(EXIT_FAILURE);
      }

      // Create Window
      window = SDL_CreateWindow("Snake Game", SDL_WINDOWPOS_CENTERED, SDL_WINDOWPOS_CENTERED,
      SCREEN_WIDTH, SCREEN_HEIGHT, SDL_WINDOW_SHOWN);

      if (window == NULL)
      {
      cout << "Window could not be created! SDL_Error: " << SDL_GetError() << endl;
      exit(EXIT_FAILURE);
      }

      // Create renderer
      renderer = SDL_CreateRenderer(window, -1, SDL_RENDERER_ACCELERATED);
      if (renderer == NULL)
      {
      cout << "Renderer could not be created! SDL_Error: " << SDL_GetError() << endl;
      exit(EXIT_FAILURE);
      }

      alive = true;
      running = true;
      ReplaceFood();
      GameLoop();
      }

      void Game::ReplaceFood()
      {
      int x, y;
      while (true)
      {
      x = rand() % GRID_WIDTH;
      y = rand() % GRID_HEIGHT;

      if (grid[x][y] == Block::empty)
      {
      grid[x][y] = Block::food;
      food.x = x;
      food.y = y;
      break;
      }
      }
      }

      void Game::GameLoop()
      {
      Uint32 before, second = SDL_GetTicks(), after;
      int frame_time, frames = 0;

      while (running)
      {
      before = SDL_GetTicks();

      PollEvents();
      Update();
      Render();

      frames++;
      after = SDL_GetTicks();
      frame_time = after - before;

      if (after - second >= 1000)
      {
      fps = frames;
      frames = 0;
      second = after;
      UpdateWindowTitle();
      }

      if (FRAME_RATE > frame_time)
      {
      SDL_Delay(FRAME_RATE - frame_time);
      }
      }

      }

      void Game::PollEvents()
      {
      SDL_Event e;
      while (SDL_PollEvent(&e))
      {
      if (e.type == SDL_QUIT)
      {
      running = false;
      }
      else if (e.type == SDL_KEYDOWN)
      {
      switch (e.key.keysym.sym)
      {
      case SDLK_UP:
      if (last_dir != Move::down || size == 1)
      dir = Move::up;
      break;

      case SDLK_DOWN:
      if (last_dir != Move::up || size == 1)
      dir = Move::down;
      break;

      case SDLK_LEFT:
      if (last_dir != Move::right || size == 1)
      dir = Move::left;
      break;

      case SDLK_RIGHT:
      if (last_dir != Move::left || size == 1)
      dir = Move::right;
      break;
      }
      }
      }
      }

      int Game::GetSize()
      {
      return size;
      }

      void Game::GrowBody(int quantity)
      {
      growing += quantity;
      }

      void Game::Update()
      {
      if (!alive)
      return;

      switch (dir)
      {
      case Move::up:
      pos.y -= speed;
      pos.x = floorf(pos.x);
      break;

      case Move::down:
      pos.y += speed;
      pos.x = floorf(pos.x);
      break;

      case Move::left:
      pos.x -= speed;
      pos.y = floorf(pos.y);
      break;

      case Move::right:
      pos.x += speed;
      pos.y = floorf(pos.y);
      break;
      }

      // Wrap
      if (pos.x < 0) pos.x = GRID_WIDTH - 1;
      else if (pos.x > GRID_WIDTH - 1) pos.x = 0;

      if (pos.y < 0) pos.y = GRID_HEIGHT - 1;
      else if (pos.y > GRID_HEIGHT - 1) pos.y = 0;

      int new_x = static_cast<int>(pos.x);
      int new_y = static_cast<int>(pos.y);

      // Check if head position has changed
      if (new_x != head.x || new_y != head.y)
      {
      last_dir = dir;

      // If we are growing, just make a new neck
      if (growing > 0)
      {
      size++;
      body.push_back(head);
      growing--;
      grid[head.x][head.y] = Block::body;
      }
      else
      {
      // We need to shift the body
      SDL_Point free = head;
      vector<SDL_Point>::reverse_iterator rit = body.rbegin();
      for ( ; rit != body.rend(); ++rit)
      {
      grid[free.x][free.y] = Block::body;
      swap(*rit, free);
      }

      grid[free.x][free.y] = Block::empty;
      }

      }

      head.x = new_x;
      head.y = new_y;

      Block & next = grid[head.x][head.y];
      // Check if there's food over here
      if (next == Block::food)
      {
      score++;
      ReplaceFood();
      GrowBody(1);
      }
      // Check if we're dead
      else if (next == Block::body)
      {
      alive = false;
      }

      next = Block::head;
      }

      int Game::GetScore()
      {
      return score;
      }

      void Game::UpdateWindowTitle()
      {
      string title = "Snakle++ Score: " + to_string(score) + " FPS: " + to_string(fps);
      SDL_SetWindowTitle(window, title.c_str());
      }

      void Game::Render()
      {
      SDL_Rect block;
      block.w = SCREEN_WIDTH / GRID_WIDTH;
      block.h = SCREEN_WIDTH / GRID_HEIGHT;

      // Clear screen
      SDL_SetRenderDrawColor(renderer, 0x1E, 0x1E, 0x1E, 0xFF);
      SDL_RenderClear(renderer);

      // Render food
      SDL_SetRenderDrawColor(renderer, 0xFF, 0xCC, 0x00, 0xFF);
      block.x = food.x * block.w;
      block.y = food.y * block.h;
      SDL_RenderFillRect(renderer, &block);

      // Render snake's body
      SDL_SetRenderDrawColor(renderer, 0xFF, 0xFF, 0xFF, 0xFF);
      for (SDL_Point & point : body)
      {
      block.x = point.x * block.w;
      block.y = point.y * block.h;
      SDL_RenderFillRect(renderer, &block);
      }

      // Render snake's head
      block.x = head.x * block.w;
      block.y = head.y * block.h;
      if (alive) SDL_SetRenderDrawColor(renderer, 0x00, 0x7A, 0xCC, 0xFF);
      else SDL_SetRenderDrawColor(renderer, 0xFF, 0x00, 0x00, 0xFF);
      SDL_RenderFillRect(renderer, &block);

      // Update Screen
      SDL_RenderPresent(renderer);
      }

      void Game::Close()
      {
      SDL_DestroyWindow(window);
      SDL_Quit();
      }






      c++ beginner snake-game sdl






      share|improve this question















      share|improve this question













      share|improve this question




      share|improve this question








      edited 17 mins ago









      200_success

      129k15153415




      129k15153415










      asked 6 hours ago









      Afonso MatosAfonso Matos

      38528




      38528






















          2 Answers
          2






          active

          oldest

          votes


















          3












          $begingroup$

          Object Usage



          This code:



          Game game = Game();
          Game().Run();
          cout << "Game has terminated successfully, score: " << game.GetScore()
          << ", size: " << game.GetSize() << endl;


          ...isn't doing what I'm pretty sure you think it is. This much: Game game = Game(); creates an object named game which is of type Game. But, I'd prefer to use just Game game;, which accomplishes the same thing more easily.



          Then you do: Game().Run();. This creates another (temporary) Game object, and invokes the Run member function on that temporary Game object (so the Game object named game that you just creates sits idly by, doing nothing).



          Then you do:



          cout << "Game has terminated successfully, score: " << game.GetScore()
          << ", size: " << game.GetSize() << endl;


          ...which tries to print the score accumulated in the object named game--but game hasn't run. Only the temporary object has run (so by rights, the score you display should always be 0).



          If I were doing this, I'd probably do something more like:



          Game game;
          game.run();
          cout << "Game has terminated successfully, score: " << game.GetScore()
          << ", size: " << game.GetSize() << endl;



          using namespace std; isn't just using; it's abusing!



          I'd (strongly) advise against using namespace std;. A using directive for another namespace can be all right, but std:: contains a huge amount of stuff, some of it with very common names that are likely to conflict with other code. Worse, every new release of the C++ standard adds still more "stuff" to std. It's generally preferable to just qualify names when you use them, so (for example) the cout shown above would be more like:



          std::cout << "Game has terminated successfully, score: " << game.GetScore()
          << ", size: " << game.GetSize() << std::endl;


          Avoid std::endl



          I'd advise avoiding std::endl in general. Along with writing a new-line to the stream, it flushes the stream. You want the new-line, but almost never want to flush the stream, so it's generally better to just write a n. On the rare occasion that you actually want the flush, do it explicitly: std::cout << 'n' << std::flush;.



          Avoid the C random number generation routines



          C's srand()/rand() have quite a few problems. I'd generally advise using the new routines in <random> instead. This is kind of a pain (seeding the new generators well is particularly painful) but they generally produce much higher quality randomness, are much more friendly to multi-threading, and using them well will keep the cool C++ programmers (now there's an oxymoron) from calling you names.



          avoid exit()



          When writing C++, it's generally better to avoid using exit. Calling it generally prevents destructors for objects on the stack from running, so you can't get a clean shutdown.



          As a general rule, I'd add a try/catch block in main, and where you're currently calling exit(), throw an object derived from std::exception. In your case, std::runtime_error probably make sense.



          if (renderer == NULL)
          {
          throw std::runtime_error(""Renderer could not be created!");
          }


          In main:



          try {
          game.Run();
          std::cout << "Game has terminated successfully, score: " << game.GetScore()
          << ", size: " << game.GetSize() << 'n';
          }
          catch (std::exception const &e) {
          std::cerr << e.what();
          }





          share|improve this answer









          $endgroup$





















            1












            $begingroup$

            using namespace std; is a bad practice. I'm glad to see you didn't use it in the header. It is still preferable not to use it at all. Please see this post for more information.





            int main(int argc, char * argv)


            It is preferred to use the empty parameter main: int main() if you are not going to use the command line arguments anyway.





            return 0 at the end of main is unnecessary and will be supplied by the compiler.





            Bug



            int main(int argc, char * argv)
            {
            Game game = Game();
            Game().Run();
            cout << "Game has terminated successfully, score: " << game.GetScore()
            << ", size: " << game.GetSize() << endl;
            return 0;
            }


            Game().Run() calls the Game constructor and creates a second instance of Game and the calls Run() on that instance. Your postgame stats likely aren't working correctly. No?





            Don't use std::endl. Prefer 'n' instead. std::endl flushes the stream, which if you wanted to do you could do manually << 'n' << std::flush and it would be more explicit what you were trying to accomplish.



            Read more here.





            static const int FRAME_RATE     = 1000 / 60;
            static const int SCREEN_WIDTH = 640;
            static const int SCREEN_HEIGHT = 640;
            static const int GRID_WIDTH = 32;
            static const int GRID_HEIGHT = 32;


            constexpr is preferred for constants that are known at compile time. They would need moved outside of the class but could still be in the Game header file.



            ALL_CAPS names are also typically used for macros. Better use snake_case, camelCase, or PascalCase. (I prefer snake_case for global constants but that's just me.)





            struct { float x = GRID_WIDTH / 2, y = GRID_HEIGHT / 2; } pos;

            SDL_Point head = { static_cast<int>(pos.x), static_cast<int>(pos.y) };


            Here you define a float that is the result of the division of two ints (which won't return a float) and then immediately cast the result to int. It's also mildly worth mentioning that your values divide cleanly (which is why you didn't notice any errors.) I see pos cast to int a few other times. Just make it an int





            Block grid[GRID_WIDTH][GRID_HEIGHT]; prefer std::array when you know the size at compile time to C style arrays. standard containers are easier to use with standard algorithms.





            srand(static_cast<unsigned int>(time(0)));



            srand is not a very good PRNG. Learn the <random> header.





            bool running = false;
            bool alive = false;


            you define and initialize these to false only to make them true before they can be used properly. Just initialize them to true to begin with. Also prefer brace initialization.



            bool running{ true };
            bool alive{ true };




            prefer nullptr to NULL. NULL is a macro and can be silently converted to int at unintended times.





            ReplaceFood() Once again avoid rand. But have you considered maintaining an std::vector<Point> of empty Points. Then you can randomly index from the vector to find the location of the next food. You will have to add the previous location of the tail back to the vector on each frame.





            Uint32 before, second = SDL_GetTicks(), after;
            int frame_time, frames = 0;


            Don't declare multiple variable on single lines. prefer 1:1 lines to variables. Especially when some are assigned and some are not. This can get confusing to read. is frame_time assigned 0? I know the answer but its not obvious just by looking at it.





            Don't take a parameter in your GrowBody() function. You only ever grow your snake by one. Just increment the size internally and move on. Only grow by a size provided by a parameter if there is a possibility of different sizes being passed in as arguments.





            Your Update() function is a bit on the larger side. I would break it into two or three helper functions. Maybe the move + wrap in one function and the checks for the head in another.



            Then you'd have



            void Game::Update()
            {
            MoveWithWrap();
            CheckHead();
            }



            I also see that you did indeed need floats for the position so I will return to that. I'm not sure I would change the way you do the pos struct after all but I would seriously think about it.



            The way to get the result as a float requires casting before the division call:



            struct { float x = static_cast<float>(GRID_WIDTH) / 2, y = static_cast<float>(GRID_HEIGHT) / 2; } pos;





            share|improve this answer









            $endgroup$













              Your Answer





              StackExchange.ifUsing("editor", function () {
              return StackExchange.using("mathjaxEditing", function () {
              StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
              StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
              });
              });
              }, "mathjax-editing");

              StackExchange.ifUsing("editor", function () {
              StackExchange.using("externalEditor", function () {
              StackExchange.using("snippets", function () {
              StackExchange.snippets.init();
              });
              });
              }, "code-snippets");

              StackExchange.ready(function() {
              var channelOptions = {
              tags: "".split(" "),
              id: "196"
              };
              initTagRenderer("".split(" "), "".split(" "), channelOptions);

              StackExchange.using("externalEditor", function() {
              // Have to fire editor after snippets, if snippets enabled
              if (StackExchange.settings.snippets.snippetsEnabled) {
              StackExchange.using("snippets", function() {
              createEditor();
              });
              }
              else {
              createEditor();
              }
              });

              function createEditor() {
              StackExchange.prepareEditor({
              heartbeatType: 'answer',
              autoActivateHeartbeat: false,
              convertImagesToLinks: false,
              noModals: true,
              showLowRepImageUploadWarning: true,
              reputationToPostImages: null,
              bindNavPrevention: true,
              postfix: "",
              imageUploader: {
              brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
              contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
              allowUrls: true
              },
              onDemand: true,
              discardSelector: ".discard-answer"
              ,immediatelyShowMarkdownHelp:true
              });


              }
              });














              draft saved

              draft discarded


















              StackExchange.ready(
              function () {
              StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f212296%2fsnake-game-in-c-with-sdl%23new-answer', 'question_page');
              }
              );

              Post as a guest















              Required, but never shown

























              2 Answers
              2






              active

              oldest

              votes








              2 Answers
              2






              active

              oldest

              votes









              active

              oldest

              votes






              active

              oldest

              votes









              3












              $begingroup$

              Object Usage



              This code:



              Game game = Game();
              Game().Run();
              cout << "Game has terminated successfully, score: " << game.GetScore()
              << ", size: " << game.GetSize() << endl;


              ...isn't doing what I'm pretty sure you think it is. This much: Game game = Game(); creates an object named game which is of type Game. But, I'd prefer to use just Game game;, which accomplishes the same thing more easily.



              Then you do: Game().Run();. This creates another (temporary) Game object, and invokes the Run member function on that temporary Game object (so the Game object named game that you just creates sits idly by, doing nothing).



              Then you do:



              cout << "Game has terminated successfully, score: " << game.GetScore()
              << ", size: " << game.GetSize() << endl;


              ...which tries to print the score accumulated in the object named game--but game hasn't run. Only the temporary object has run (so by rights, the score you display should always be 0).



              If I were doing this, I'd probably do something more like:



              Game game;
              game.run();
              cout << "Game has terminated successfully, score: " << game.GetScore()
              << ", size: " << game.GetSize() << endl;



              using namespace std; isn't just using; it's abusing!



              I'd (strongly) advise against using namespace std;. A using directive for another namespace can be all right, but std:: contains a huge amount of stuff, some of it with very common names that are likely to conflict with other code. Worse, every new release of the C++ standard adds still more "stuff" to std. It's generally preferable to just qualify names when you use them, so (for example) the cout shown above would be more like:



              std::cout << "Game has terminated successfully, score: " << game.GetScore()
              << ", size: " << game.GetSize() << std::endl;


              Avoid std::endl



              I'd advise avoiding std::endl in general. Along with writing a new-line to the stream, it flushes the stream. You want the new-line, but almost never want to flush the stream, so it's generally better to just write a n. On the rare occasion that you actually want the flush, do it explicitly: std::cout << 'n' << std::flush;.



              Avoid the C random number generation routines



              C's srand()/rand() have quite a few problems. I'd generally advise using the new routines in <random> instead. This is kind of a pain (seeding the new generators well is particularly painful) but they generally produce much higher quality randomness, are much more friendly to multi-threading, and using them well will keep the cool C++ programmers (now there's an oxymoron) from calling you names.



              avoid exit()



              When writing C++, it's generally better to avoid using exit. Calling it generally prevents destructors for objects on the stack from running, so you can't get a clean shutdown.



              As a general rule, I'd add a try/catch block in main, and where you're currently calling exit(), throw an object derived from std::exception. In your case, std::runtime_error probably make sense.



              if (renderer == NULL)
              {
              throw std::runtime_error(""Renderer could not be created!");
              }


              In main:



              try {
              game.Run();
              std::cout << "Game has terminated successfully, score: " << game.GetScore()
              << ", size: " << game.GetSize() << 'n';
              }
              catch (std::exception const &e) {
              std::cerr << e.what();
              }





              share|improve this answer









              $endgroup$


















                3












                $begingroup$

                Object Usage



                This code:



                Game game = Game();
                Game().Run();
                cout << "Game has terminated successfully, score: " << game.GetScore()
                << ", size: " << game.GetSize() << endl;


                ...isn't doing what I'm pretty sure you think it is. This much: Game game = Game(); creates an object named game which is of type Game. But, I'd prefer to use just Game game;, which accomplishes the same thing more easily.



                Then you do: Game().Run();. This creates another (temporary) Game object, and invokes the Run member function on that temporary Game object (so the Game object named game that you just creates sits idly by, doing nothing).



                Then you do:



                cout << "Game has terminated successfully, score: " << game.GetScore()
                << ", size: " << game.GetSize() << endl;


                ...which tries to print the score accumulated in the object named game--but game hasn't run. Only the temporary object has run (so by rights, the score you display should always be 0).



                If I were doing this, I'd probably do something more like:



                Game game;
                game.run();
                cout << "Game has terminated successfully, score: " << game.GetScore()
                << ", size: " << game.GetSize() << endl;



                using namespace std; isn't just using; it's abusing!



                I'd (strongly) advise against using namespace std;. A using directive for another namespace can be all right, but std:: contains a huge amount of stuff, some of it with very common names that are likely to conflict with other code. Worse, every new release of the C++ standard adds still more "stuff" to std. It's generally preferable to just qualify names when you use them, so (for example) the cout shown above would be more like:



                std::cout << "Game has terminated successfully, score: " << game.GetScore()
                << ", size: " << game.GetSize() << std::endl;


                Avoid std::endl



                I'd advise avoiding std::endl in general. Along with writing a new-line to the stream, it flushes the stream. You want the new-line, but almost never want to flush the stream, so it's generally better to just write a n. On the rare occasion that you actually want the flush, do it explicitly: std::cout << 'n' << std::flush;.



                Avoid the C random number generation routines



                C's srand()/rand() have quite a few problems. I'd generally advise using the new routines in <random> instead. This is kind of a pain (seeding the new generators well is particularly painful) but they generally produce much higher quality randomness, are much more friendly to multi-threading, and using them well will keep the cool C++ programmers (now there's an oxymoron) from calling you names.



                avoid exit()



                When writing C++, it's generally better to avoid using exit. Calling it generally prevents destructors for objects on the stack from running, so you can't get a clean shutdown.



                As a general rule, I'd add a try/catch block in main, and where you're currently calling exit(), throw an object derived from std::exception. In your case, std::runtime_error probably make sense.



                if (renderer == NULL)
                {
                throw std::runtime_error(""Renderer could not be created!");
                }


                In main:



                try {
                game.Run();
                std::cout << "Game has terminated successfully, score: " << game.GetScore()
                << ", size: " << game.GetSize() << 'n';
                }
                catch (std::exception const &e) {
                std::cerr << e.what();
                }





                share|improve this answer









                $endgroup$
















                  3












                  3








                  3





                  $begingroup$

                  Object Usage



                  This code:



                  Game game = Game();
                  Game().Run();
                  cout << "Game has terminated successfully, score: " << game.GetScore()
                  << ", size: " << game.GetSize() << endl;


                  ...isn't doing what I'm pretty sure you think it is. This much: Game game = Game(); creates an object named game which is of type Game. But, I'd prefer to use just Game game;, which accomplishes the same thing more easily.



                  Then you do: Game().Run();. This creates another (temporary) Game object, and invokes the Run member function on that temporary Game object (so the Game object named game that you just creates sits idly by, doing nothing).



                  Then you do:



                  cout << "Game has terminated successfully, score: " << game.GetScore()
                  << ", size: " << game.GetSize() << endl;


                  ...which tries to print the score accumulated in the object named game--but game hasn't run. Only the temporary object has run (so by rights, the score you display should always be 0).



                  If I were doing this, I'd probably do something more like:



                  Game game;
                  game.run();
                  cout << "Game has terminated successfully, score: " << game.GetScore()
                  << ", size: " << game.GetSize() << endl;



                  using namespace std; isn't just using; it's abusing!



                  I'd (strongly) advise against using namespace std;. A using directive for another namespace can be all right, but std:: contains a huge amount of stuff, some of it with very common names that are likely to conflict with other code. Worse, every new release of the C++ standard adds still more "stuff" to std. It's generally preferable to just qualify names when you use them, so (for example) the cout shown above would be more like:



                  std::cout << "Game has terminated successfully, score: " << game.GetScore()
                  << ", size: " << game.GetSize() << std::endl;


                  Avoid std::endl



                  I'd advise avoiding std::endl in general. Along with writing a new-line to the stream, it flushes the stream. You want the new-line, but almost never want to flush the stream, so it's generally better to just write a n. On the rare occasion that you actually want the flush, do it explicitly: std::cout << 'n' << std::flush;.



                  Avoid the C random number generation routines



                  C's srand()/rand() have quite a few problems. I'd generally advise using the new routines in <random> instead. This is kind of a pain (seeding the new generators well is particularly painful) but they generally produce much higher quality randomness, are much more friendly to multi-threading, and using them well will keep the cool C++ programmers (now there's an oxymoron) from calling you names.



                  avoid exit()



                  When writing C++, it's generally better to avoid using exit. Calling it generally prevents destructors for objects on the stack from running, so you can't get a clean shutdown.



                  As a general rule, I'd add a try/catch block in main, and where you're currently calling exit(), throw an object derived from std::exception. In your case, std::runtime_error probably make sense.



                  if (renderer == NULL)
                  {
                  throw std::runtime_error(""Renderer could not be created!");
                  }


                  In main:



                  try {
                  game.Run();
                  std::cout << "Game has terminated successfully, score: " << game.GetScore()
                  << ", size: " << game.GetSize() << 'n';
                  }
                  catch (std::exception const &e) {
                  std::cerr << e.what();
                  }





                  share|improve this answer









                  $endgroup$



                  Object Usage



                  This code:



                  Game game = Game();
                  Game().Run();
                  cout << "Game has terminated successfully, score: " << game.GetScore()
                  << ", size: " << game.GetSize() << endl;


                  ...isn't doing what I'm pretty sure you think it is. This much: Game game = Game(); creates an object named game which is of type Game. But, I'd prefer to use just Game game;, which accomplishes the same thing more easily.



                  Then you do: Game().Run();. This creates another (temporary) Game object, and invokes the Run member function on that temporary Game object (so the Game object named game that you just creates sits idly by, doing nothing).



                  Then you do:



                  cout << "Game has terminated successfully, score: " << game.GetScore()
                  << ", size: " << game.GetSize() << endl;


                  ...which tries to print the score accumulated in the object named game--but game hasn't run. Only the temporary object has run (so by rights, the score you display should always be 0).



                  If I were doing this, I'd probably do something more like:



                  Game game;
                  game.run();
                  cout << "Game has terminated successfully, score: " << game.GetScore()
                  << ", size: " << game.GetSize() << endl;



                  using namespace std; isn't just using; it's abusing!



                  I'd (strongly) advise against using namespace std;. A using directive for another namespace can be all right, but std:: contains a huge amount of stuff, some of it with very common names that are likely to conflict with other code. Worse, every new release of the C++ standard adds still more "stuff" to std. It's generally preferable to just qualify names when you use them, so (for example) the cout shown above would be more like:



                  std::cout << "Game has terminated successfully, score: " << game.GetScore()
                  << ", size: " << game.GetSize() << std::endl;


                  Avoid std::endl



                  I'd advise avoiding std::endl in general. Along with writing a new-line to the stream, it flushes the stream. You want the new-line, but almost never want to flush the stream, so it's generally better to just write a n. On the rare occasion that you actually want the flush, do it explicitly: std::cout << 'n' << std::flush;.



                  Avoid the C random number generation routines



                  C's srand()/rand() have quite a few problems. I'd generally advise using the new routines in <random> instead. This is kind of a pain (seeding the new generators well is particularly painful) but they generally produce much higher quality randomness, are much more friendly to multi-threading, and using them well will keep the cool C++ programmers (now there's an oxymoron) from calling you names.



                  avoid exit()



                  When writing C++, it's generally better to avoid using exit. Calling it generally prevents destructors for objects on the stack from running, so you can't get a clean shutdown.



                  As a general rule, I'd add a try/catch block in main, and where you're currently calling exit(), throw an object derived from std::exception. In your case, std::runtime_error probably make sense.



                  if (renderer == NULL)
                  {
                  throw std::runtime_error(""Renderer could not be created!");
                  }


                  In main:



                  try {
                  game.Run();
                  std::cout << "Game has terminated successfully, score: " << game.GetScore()
                  << ", size: " << game.GetSize() << 'n';
                  }
                  catch (std::exception const &e) {
                  std::cerr << e.what();
                  }






                  share|improve this answer












                  share|improve this answer



                  share|improve this answer










                  answered 2 hours ago









                  Jerry CoffinJerry Coffin

                  28k461126




                  28k461126

























                      1












                      $begingroup$

                      using namespace std; is a bad practice. I'm glad to see you didn't use it in the header. It is still preferable not to use it at all. Please see this post for more information.





                      int main(int argc, char * argv)


                      It is preferred to use the empty parameter main: int main() if you are not going to use the command line arguments anyway.





                      return 0 at the end of main is unnecessary and will be supplied by the compiler.





                      Bug



                      int main(int argc, char * argv)
                      {
                      Game game = Game();
                      Game().Run();
                      cout << "Game has terminated successfully, score: " << game.GetScore()
                      << ", size: " << game.GetSize() << endl;
                      return 0;
                      }


                      Game().Run() calls the Game constructor and creates a second instance of Game and the calls Run() on that instance. Your postgame stats likely aren't working correctly. No?





                      Don't use std::endl. Prefer 'n' instead. std::endl flushes the stream, which if you wanted to do you could do manually << 'n' << std::flush and it would be more explicit what you were trying to accomplish.



                      Read more here.





                      static const int FRAME_RATE     = 1000 / 60;
                      static const int SCREEN_WIDTH = 640;
                      static const int SCREEN_HEIGHT = 640;
                      static const int GRID_WIDTH = 32;
                      static const int GRID_HEIGHT = 32;


                      constexpr is preferred for constants that are known at compile time. They would need moved outside of the class but could still be in the Game header file.



                      ALL_CAPS names are also typically used for macros. Better use snake_case, camelCase, or PascalCase. (I prefer snake_case for global constants but that's just me.)





                      struct { float x = GRID_WIDTH / 2, y = GRID_HEIGHT / 2; } pos;

                      SDL_Point head = { static_cast<int>(pos.x), static_cast<int>(pos.y) };


                      Here you define a float that is the result of the division of two ints (which won't return a float) and then immediately cast the result to int. It's also mildly worth mentioning that your values divide cleanly (which is why you didn't notice any errors.) I see pos cast to int a few other times. Just make it an int





                      Block grid[GRID_WIDTH][GRID_HEIGHT]; prefer std::array when you know the size at compile time to C style arrays. standard containers are easier to use with standard algorithms.





                      srand(static_cast<unsigned int>(time(0)));



                      srand is not a very good PRNG. Learn the <random> header.





                      bool running = false;
                      bool alive = false;


                      you define and initialize these to false only to make them true before they can be used properly. Just initialize them to true to begin with. Also prefer brace initialization.



                      bool running{ true };
                      bool alive{ true };




                      prefer nullptr to NULL. NULL is a macro and can be silently converted to int at unintended times.





                      ReplaceFood() Once again avoid rand. But have you considered maintaining an std::vector<Point> of empty Points. Then you can randomly index from the vector to find the location of the next food. You will have to add the previous location of the tail back to the vector on each frame.





                      Uint32 before, second = SDL_GetTicks(), after;
                      int frame_time, frames = 0;


                      Don't declare multiple variable on single lines. prefer 1:1 lines to variables. Especially when some are assigned and some are not. This can get confusing to read. is frame_time assigned 0? I know the answer but its not obvious just by looking at it.





                      Don't take a parameter in your GrowBody() function. You only ever grow your snake by one. Just increment the size internally and move on. Only grow by a size provided by a parameter if there is a possibility of different sizes being passed in as arguments.





                      Your Update() function is a bit on the larger side. I would break it into two or three helper functions. Maybe the move + wrap in one function and the checks for the head in another.



                      Then you'd have



                      void Game::Update()
                      {
                      MoveWithWrap();
                      CheckHead();
                      }



                      I also see that you did indeed need floats for the position so I will return to that. I'm not sure I would change the way you do the pos struct after all but I would seriously think about it.



                      The way to get the result as a float requires casting before the division call:



                      struct { float x = static_cast<float>(GRID_WIDTH) / 2, y = static_cast<float>(GRID_HEIGHT) / 2; } pos;





                      share|improve this answer









                      $endgroup$


















                        1












                        $begingroup$

                        using namespace std; is a bad practice. I'm glad to see you didn't use it in the header. It is still preferable not to use it at all. Please see this post for more information.





                        int main(int argc, char * argv)


                        It is preferred to use the empty parameter main: int main() if you are not going to use the command line arguments anyway.





                        return 0 at the end of main is unnecessary and will be supplied by the compiler.





                        Bug



                        int main(int argc, char * argv)
                        {
                        Game game = Game();
                        Game().Run();
                        cout << "Game has terminated successfully, score: " << game.GetScore()
                        << ", size: " << game.GetSize() << endl;
                        return 0;
                        }


                        Game().Run() calls the Game constructor and creates a second instance of Game and the calls Run() on that instance. Your postgame stats likely aren't working correctly. No?





                        Don't use std::endl. Prefer 'n' instead. std::endl flushes the stream, which if you wanted to do you could do manually << 'n' << std::flush and it would be more explicit what you were trying to accomplish.



                        Read more here.





                        static const int FRAME_RATE     = 1000 / 60;
                        static const int SCREEN_WIDTH = 640;
                        static const int SCREEN_HEIGHT = 640;
                        static const int GRID_WIDTH = 32;
                        static const int GRID_HEIGHT = 32;


                        constexpr is preferred for constants that are known at compile time. They would need moved outside of the class but could still be in the Game header file.



                        ALL_CAPS names are also typically used for macros. Better use snake_case, camelCase, or PascalCase. (I prefer snake_case for global constants but that's just me.)





                        struct { float x = GRID_WIDTH / 2, y = GRID_HEIGHT / 2; } pos;

                        SDL_Point head = { static_cast<int>(pos.x), static_cast<int>(pos.y) };


                        Here you define a float that is the result of the division of two ints (which won't return a float) and then immediately cast the result to int. It's also mildly worth mentioning that your values divide cleanly (which is why you didn't notice any errors.) I see pos cast to int a few other times. Just make it an int





                        Block grid[GRID_WIDTH][GRID_HEIGHT]; prefer std::array when you know the size at compile time to C style arrays. standard containers are easier to use with standard algorithms.





                        srand(static_cast<unsigned int>(time(0)));



                        srand is not a very good PRNG. Learn the <random> header.





                        bool running = false;
                        bool alive = false;


                        you define and initialize these to false only to make them true before they can be used properly. Just initialize them to true to begin with. Also prefer brace initialization.



                        bool running{ true };
                        bool alive{ true };




                        prefer nullptr to NULL. NULL is a macro and can be silently converted to int at unintended times.





                        ReplaceFood() Once again avoid rand. But have you considered maintaining an std::vector<Point> of empty Points. Then you can randomly index from the vector to find the location of the next food. You will have to add the previous location of the tail back to the vector on each frame.





                        Uint32 before, second = SDL_GetTicks(), after;
                        int frame_time, frames = 0;


                        Don't declare multiple variable on single lines. prefer 1:1 lines to variables. Especially when some are assigned and some are not. This can get confusing to read. is frame_time assigned 0? I know the answer but its not obvious just by looking at it.





                        Don't take a parameter in your GrowBody() function. You only ever grow your snake by one. Just increment the size internally and move on. Only grow by a size provided by a parameter if there is a possibility of different sizes being passed in as arguments.





                        Your Update() function is a bit on the larger side. I would break it into two or three helper functions. Maybe the move + wrap in one function and the checks for the head in another.



                        Then you'd have



                        void Game::Update()
                        {
                        MoveWithWrap();
                        CheckHead();
                        }



                        I also see that you did indeed need floats for the position so I will return to that. I'm not sure I would change the way you do the pos struct after all but I would seriously think about it.



                        The way to get the result as a float requires casting before the division call:



                        struct { float x = static_cast<float>(GRID_WIDTH) / 2, y = static_cast<float>(GRID_HEIGHT) / 2; } pos;





                        share|improve this answer









                        $endgroup$
















                          1












                          1








                          1





                          $begingroup$

                          using namespace std; is a bad practice. I'm glad to see you didn't use it in the header. It is still preferable not to use it at all. Please see this post for more information.





                          int main(int argc, char * argv)


                          It is preferred to use the empty parameter main: int main() if you are not going to use the command line arguments anyway.





                          return 0 at the end of main is unnecessary and will be supplied by the compiler.





                          Bug



                          int main(int argc, char * argv)
                          {
                          Game game = Game();
                          Game().Run();
                          cout << "Game has terminated successfully, score: " << game.GetScore()
                          << ", size: " << game.GetSize() << endl;
                          return 0;
                          }


                          Game().Run() calls the Game constructor and creates a second instance of Game and the calls Run() on that instance. Your postgame stats likely aren't working correctly. No?





                          Don't use std::endl. Prefer 'n' instead. std::endl flushes the stream, which if you wanted to do you could do manually << 'n' << std::flush and it would be more explicit what you were trying to accomplish.



                          Read more here.





                          static const int FRAME_RATE     = 1000 / 60;
                          static const int SCREEN_WIDTH = 640;
                          static const int SCREEN_HEIGHT = 640;
                          static const int GRID_WIDTH = 32;
                          static const int GRID_HEIGHT = 32;


                          constexpr is preferred for constants that are known at compile time. They would need moved outside of the class but could still be in the Game header file.



                          ALL_CAPS names are also typically used for macros. Better use snake_case, camelCase, or PascalCase. (I prefer snake_case for global constants but that's just me.)





                          struct { float x = GRID_WIDTH / 2, y = GRID_HEIGHT / 2; } pos;

                          SDL_Point head = { static_cast<int>(pos.x), static_cast<int>(pos.y) };


                          Here you define a float that is the result of the division of two ints (which won't return a float) and then immediately cast the result to int. It's also mildly worth mentioning that your values divide cleanly (which is why you didn't notice any errors.) I see pos cast to int a few other times. Just make it an int





                          Block grid[GRID_WIDTH][GRID_HEIGHT]; prefer std::array when you know the size at compile time to C style arrays. standard containers are easier to use with standard algorithms.





                          srand(static_cast<unsigned int>(time(0)));



                          srand is not a very good PRNG. Learn the <random> header.





                          bool running = false;
                          bool alive = false;


                          you define and initialize these to false only to make them true before they can be used properly. Just initialize them to true to begin with. Also prefer brace initialization.



                          bool running{ true };
                          bool alive{ true };




                          prefer nullptr to NULL. NULL is a macro and can be silently converted to int at unintended times.





                          ReplaceFood() Once again avoid rand. But have you considered maintaining an std::vector<Point> of empty Points. Then you can randomly index from the vector to find the location of the next food. You will have to add the previous location of the tail back to the vector on each frame.





                          Uint32 before, second = SDL_GetTicks(), after;
                          int frame_time, frames = 0;


                          Don't declare multiple variable on single lines. prefer 1:1 lines to variables. Especially when some are assigned and some are not. This can get confusing to read. is frame_time assigned 0? I know the answer but its not obvious just by looking at it.





                          Don't take a parameter in your GrowBody() function. You only ever grow your snake by one. Just increment the size internally and move on. Only grow by a size provided by a parameter if there is a possibility of different sizes being passed in as arguments.





                          Your Update() function is a bit on the larger side. I would break it into two or three helper functions. Maybe the move + wrap in one function and the checks for the head in another.



                          Then you'd have



                          void Game::Update()
                          {
                          MoveWithWrap();
                          CheckHead();
                          }



                          I also see that you did indeed need floats for the position so I will return to that. I'm not sure I would change the way you do the pos struct after all but I would seriously think about it.



                          The way to get the result as a float requires casting before the division call:



                          struct { float x = static_cast<float>(GRID_WIDTH) / 2, y = static_cast<float>(GRID_HEIGHT) / 2; } pos;





                          share|improve this answer









                          $endgroup$



                          using namespace std; is a bad practice. I'm glad to see you didn't use it in the header. It is still preferable not to use it at all. Please see this post for more information.





                          int main(int argc, char * argv)


                          It is preferred to use the empty parameter main: int main() if you are not going to use the command line arguments anyway.





                          return 0 at the end of main is unnecessary and will be supplied by the compiler.





                          Bug



                          int main(int argc, char * argv)
                          {
                          Game game = Game();
                          Game().Run();
                          cout << "Game has terminated successfully, score: " << game.GetScore()
                          << ", size: " << game.GetSize() << endl;
                          return 0;
                          }


                          Game().Run() calls the Game constructor and creates a second instance of Game and the calls Run() on that instance. Your postgame stats likely aren't working correctly. No?





                          Don't use std::endl. Prefer 'n' instead. std::endl flushes the stream, which if you wanted to do you could do manually << 'n' << std::flush and it would be more explicit what you were trying to accomplish.



                          Read more here.





                          static const int FRAME_RATE     = 1000 / 60;
                          static const int SCREEN_WIDTH = 640;
                          static const int SCREEN_HEIGHT = 640;
                          static const int GRID_WIDTH = 32;
                          static const int GRID_HEIGHT = 32;


                          constexpr is preferred for constants that are known at compile time. They would need moved outside of the class but could still be in the Game header file.



                          ALL_CAPS names are also typically used for macros. Better use snake_case, camelCase, or PascalCase. (I prefer snake_case for global constants but that's just me.)





                          struct { float x = GRID_WIDTH / 2, y = GRID_HEIGHT / 2; } pos;

                          SDL_Point head = { static_cast<int>(pos.x), static_cast<int>(pos.y) };


                          Here you define a float that is the result of the division of two ints (which won't return a float) and then immediately cast the result to int. It's also mildly worth mentioning that your values divide cleanly (which is why you didn't notice any errors.) I see pos cast to int a few other times. Just make it an int





                          Block grid[GRID_WIDTH][GRID_HEIGHT]; prefer std::array when you know the size at compile time to C style arrays. standard containers are easier to use with standard algorithms.





                          srand(static_cast<unsigned int>(time(0)));



                          srand is not a very good PRNG. Learn the <random> header.





                          bool running = false;
                          bool alive = false;


                          you define and initialize these to false only to make them true before they can be used properly. Just initialize them to true to begin with. Also prefer brace initialization.



                          bool running{ true };
                          bool alive{ true };




                          prefer nullptr to NULL. NULL is a macro and can be silently converted to int at unintended times.





                          ReplaceFood() Once again avoid rand. But have you considered maintaining an std::vector<Point> of empty Points. Then you can randomly index from the vector to find the location of the next food. You will have to add the previous location of the tail back to the vector on each frame.





                          Uint32 before, second = SDL_GetTicks(), after;
                          int frame_time, frames = 0;


                          Don't declare multiple variable on single lines. prefer 1:1 lines to variables. Especially when some are assigned and some are not. This can get confusing to read. is frame_time assigned 0? I know the answer but its not obvious just by looking at it.





                          Don't take a parameter in your GrowBody() function. You only ever grow your snake by one. Just increment the size internally and move on. Only grow by a size provided by a parameter if there is a possibility of different sizes being passed in as arguments.





                          Your Update() function is a bit on the larger side. I would break it into two or three helper functions. Maybe the move + wrap in one function and the checks for the head in another.



                          Then you'd have



                          void Game::Update()
                          {
                          MoveWithWrap();
                          CheckHead();
                          }



                          I also see that you did indeed need floats for the position so I will return to that. I'm not sure I would change the way you do the pos struct after all but I would seriously think about it.



                          The way to get the result as a float requires casting before the division call:



                          struct { float x = static_cast<float>(GRID_WIDTH) / 2, y = static_cast<float>(GRID_HEIGHT) / 2; } pos;






                          share|improve this answer












                          share|improve this answer



                          share|improve this answer










                          answered 2 hours ago









                          bruglescobruglesco

                          1,4602721




                          1,4602721






























                              draft saved

                              draft discarded




















































                              Thanks for contributing an answer to Code Review Stack Exchange!


                              • Please be sure to answer the question. Provide details and share your research!

                              But avoid



                              • Asking for help, clarification, or responding to other answers.

                              • Making statements based on opinion; back them up with references or personal experience.


                              Use MathJax to format equations. MathJax reference.


                              To learn more, see our tips on writing great answers.




                              draft saved


                              draft discarded














                              StackExchange.ready(
                              function () {
                              StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f212296%2fsnake-game-in-c-with-sdl%23new-answer', 'question_page');
                              }
                              );

                              Post as a guest















                              Required, but never shown





















































                              Required, but never shown














                              Required, but never shown












                              Required, but never shown







                              Required, but never shown

































                              Required, but never shown














                              Required, but never shown












                              Required, but never shown







                              Required, but never shown







                              Popular posts from this blog

                              How to label and detect the document text images

                              Vallis Paradisi

                              Tabula Rosettana