Code Critique

[abcdefghijklmnopqrstuvwxy]

I hope no one is code shy... Please critique my code as harshly as possible.

Output: mastermind.cpp. (The driver program) The output is the board, complete with one tac of each color drawn to the default tac location.

mastermind.cpp

code:

#include "SDL_MainWindow.hpp" #include "MasterMind.hpp" void initialize_tacs(std::vector<Tac>*, Board&); void display_tacs(std::vector<Tac>*, SDL_Surface*); int main(int args, char** argv) {   try   {     SDL_Window window(SCREEN_WIDTH, SCREEN_HEIGHT, BPP,                     SDL_HWSURFACE | SDL_DOUBLEBUF, "MasterMind");     Board board;       std::vector<Tac>* pTacs = new std::vector<Tac>;     pTacs->reserve(board.size());     initialize_tacs(pTacs, board);     bool quit = false;     while (quit == false)     {       SDL_Event event;       while (SDL_PollEvent(&event))       {          if (event.type == SDL_QUIT)           quit = true;         if (event.type == SDL_MOUSEBUTTONDOWN)         {         }       }       display_tacs(pTacs, board.me());       board.drawOn(window.me());            SDL_Flip(window.me());     }   }   catch(...)   {     cerr << "An error occurred" << endl;   }   return 0; } void initialize_tacs(std::vector<Tac>* pTacs, Board& board) {   for (int i = 0; i < 8 ; i++)   {     Tac tac((Color)i, board);  //i represents the color constant.     pTacs->push_back(tac);   }   return; } void display_tacs(std::vector<Tac>* pTacs, SDL_Surface* board) {   for (std::vector<Tac>::iterator i(pTacs->begin()); i != pTacs->end(); ++i)   {     i->drawOn(board);   }   return; }

"MasterMind.hpp"

code:

#include <string> #include <vector> #include <SDL/SDL_image.h> #include <SDL/SDL.h> enum Color {BLACK, WHITE, RED, BLUE, YELLOW, PURPLE, GREEN, BROWN, ORANGE}; const std::string black="black_tac.gif", white="white_tac.gif",                   red="red_tac.gif", blue="blue_tac.gif",                   yellow="yellow_tac.gif", purple="purple_tac.gif",                   green="green_tac.gif", brown="brown_tac.gif"; const std::string boardFile = "main_board.jpg", cordFile = "holes.cfg", tacFile = "tac.cfg"; const int SCREEN_WIDTH = 800, SCREEN_HEIGHT = 900, BPP = 32; const int RADIUS = 18,  //the radius from a point which is the center of a hole on the board.           TAC_RADIUS = 30; using std::cerr; using std::endl; SDL_Surface* loadImage(std::string filename); class Point {  public:   int x, y;   Point(int x_cord = 0, int y_cord = 0): x(x_cord), y(y_cord)   {}   ~Point()   {}   Point& operator=(Point const &other_cord)   {     this->x = other_cord.x;     this->y = other_cord.y;     return *this;   } }; class Hole {  private:   Point center;   bool status;   int ID;  public:   Hole();   Hole(Point& cord, int id): center(cord), ID(id)   {}   ~Hole()   {}   void setPointandID(Point cord, int id) { center = cord;  ID = id; }   void setSatusOccupied() { status = true; }   bool isOccupied() const { return status; }   bool isOnHole(const Point&) const;   int getID() const { return ID; }   Point getCenter() const { return center; } }; //my implementation of a has-a relationship, where board has-a Point... class Board {  private:   SDL_Surface* board;   const int columns;   const int rows;   std::vector<Hole> guess_holes;   std::vector<Hole> answer_holes;  public:   Board();     ~Board();   Hole& findNearestHole() const; //need a sort algo for that...   void drawOn(SDL_Surface*);   SDL_Surface* me() const { return board; }   int size() const { return columns*rows; }     //public data   std::vector<Hole> tacDefaults; }; /* vector <int>::iterator vIter; standard way to iterate over a vector and insert values...    for ( vIter = vec.begin ( ) ; vIter != vec.end ( ); vIter++) */ class Tac {  private:   Point location;   SDL_Surface* tac;   Color color;  public:   Tac(Color, Board&);   SDL_Surface* getSurface() const { return tac; }   void drawOn(SDL_Surface*); };

"MasterMind.cpp"

code:

#include <fstream> #include <vector> #include <iostream> #include <SDL/SDL.h> #include <SDL/SDL_image.h> #include "MasterMind.hpp" SDL_Surface* loadImage(std::string filename) {     SDL_Surface* loadedImage = NULL;       SDL_Surface* optimizedImage = NULL;       loadedImage = IMG_Load(filename.c_str());     if (loadedImage != NULL)    {      optimizedImage = SDL_DisplayFormat(loadedImage); //to ensure format is same for image and surface to be blitted.      SDL_FreeSurface(loadedImage);     }    else      cerr << "Unable to initialize image," << filename << endl;    return optimizedImage; } //SDL_FreeSurface(SDL_Surface*) //this is a biggy man... /* ** Begin Hole */ Hole::Hole() { } bool Hole::isOnHole(Point const &point) const {   if (point.x <= (center.x + RADIUS) && point.x >= (center.x - RADIUS)   &&  point.y <= (center.y + RADIUS) && point.y >= (center.x - RADIUS))     return true;   return false; } /* ** End Hole */ /* **Begin Tac */ Tac::Tac(Color colour, Board& game_board): color(colour) {   switch (color)   {     case BLACK: tac = loadImage(black);                 break;     case WHITE: tac = loadImage(white);                 break;     case RED:   tac = loadImage(red);                 break;     case BLUE:  tac = loadImage(blue);                 break;     case YELLOW:tac = loadImage(yellow);                 break;     case PURPLE:tac = loadImage(purple);                 break;     case GREEN: tac = loadImage(green);                 break;     case BROWN: tac = loadImage(brown);                 break;     default: cerr << "unable to load image, invalid color specified." << endl;   }      for (std::vector<Hole>::iterator i(game_board.tacDefaults.begin()); i !=          game_board.tacDefaults.end() ; ++i)   {     if (i->getID() == colour)  //set location to default location for color.       location = i->getCenter();   } } void Tac::drawOn(SDL_Surface* dest) {   SDL_Rect cord;   cord.x = location.x;   cord.y = location.y;   SDL_BlitSurface(tac, NULL, dest, &cord); }   /* ** End Tac */ /* ** Begin Board */ Board::Board(): columns(5), rows(12) {   board = loadImage(boardFile);   std::ifstream guess_holeS (cordFile.c_str());   std::ifstream tac_defaults (tacFile.c_str());   int num,x,y;   guess_holes.reserve(rows*columns+5); //5 is the answer holes.   tacDefaults.reserve(8); //8 is the # of colors.   while (!guess_holeS.eof())   {     guess_holeS >> num;     guess_holeS >> x;     guess_holeS >> y;     Point cordinate(x,y);     Hole hole(cordinate, num);   //set cordinate and ID on hole     guess_holes.push_back(hole); //then add the hole to the vec.      }   while (!tac_defaults.eof())   {     tac_defaults >> num; //num is aka color...     tac_defaults >> x;     tac_defaults >> y;     Point cordinate(x,y);     Hole hole(cordinate, num);     tacDefaults.push_back(hole);   } }  Board::~Board() {   SDL_FreeSurface(board);   } void Board::drawOn(SDL_Surface* dest) {   SDL_Rect location;   location.x = 0;   location.y = 0;   SDL_BlitSurface(board, NULL, dest, &location); } /* ** End Board */

"SDL_MainWindow.hpp"

code:

#include <iostream> #include <string> #include <SDL/SDL.h> class SDL_Window {   private:     SDL_Surface* pMainWindow;     const int  _BPP_;     const int SCRHEIGHT;     const int SCRWIDTH;     const int INIT_OPTIONS;   public:   SDL_Window(int,int,int,int,std::string);     ~SDL_Window();     SDL_Surface* me() const { return pMainWindow; } };

"SDL_MainWindow.cpp"

code:

#include <SDL/SDL.h> #include <SDL/SDL_image.h> #include "SDL_MainWindow.hpp" #include <string> using std::cerr; using std::endl; SDL_Window::SDL_Window(int sw, int sh, int bpp, int options, std::string):_BPP_(bpp), SCRWIDTH(sw), SCRHEIGHT(sh), INIT_OPTIONS(options) {   if (SDL_Init(SDL_INIT_AUDIO|SDL_INIT_VIDEO) < 0)   {     cerr << "Unable to init SDL: %s\n" << SDL_GetError();     return;   }   pMainWindow = SDL_SetVideoMode(sw, sh , bpp , options);   if (pMainWindow == NULL)   {     cerr << "Unable to set %dx%d video resolution: %s\n" << SDL_GetError();     return;   } } SDL_Window::~SDL_Window() {   cerr << "destructor called.";   SDL_FreeSurface(pMainWindow);  //give back memory at freestore like delete.   SDL_Quit();  //required to clean things up after a call to SDL_Init.  }

[md]

might I recomment an archive of some kind containing your code, and perhaps a make file or instructions on building it? posted code like that is hard to read; and annoying as hell to compile.

[abcdefghijklmnopqrstuvwxy]

I was hoping you could just point out any code that is not good or could be performed better. But maybe i'll end up posting an archive so you can compile it yourselves. My main weakness is vectors. I have major problems with them all the time and I never feel like I'm using them to their fullest. Currently I'm in the midst of a segmentation fault which I believe is vector related. But that bug isn't apparent in this code... I dont think... Maybe it is, but in that case it's a latent bug. Whatever this segmentation fault is, it's the hardest thing I've ever had to track down in my life... I'm using gdb to no avail...

[wtd]

code:

void initialize_tacs(std::vector<Tac>* pTacs, Board& board) {   for (int i = 0; i < 8 ; i++)   {     Tac tac((Color)i, board);  //i represents the color constant.     pTacs->push_back(tac);   }   return; }

Why are you bothering with the variable "tac", and why the return statement? Let's see what this could look like.

code:

void initialize_tacs(std::vector<Tac>* pTacs, Board& board) {   for (int i = 0; i < 8 ; i++)   {     pTacs->push_back(Tac((Color)i, board));   } }

[abcdefghijklmnopqrstuvwxy]

Cool, thanks. I didn't think of that. Question though, is it better to create a std::vector<Tac*> or a std::vector<Tac>*. Someone mentioned a performance increase with the former, but I cannot see how.

[wtd]

They are utterly different types.

[abcdefghijklmnopqrstuvwxy]

Yes and I'm asking which is better. I could use either one to make my program work.

So there is no confusion: I know that std::vector<Tac*> is a vector of pointers to Tac objects and std::vector<Tac>* is a pointer to a vector which contains Tac objects.

[wtd]

I guess it just seems like apples and oranges to me. Choose whichever more clearly solves the problem.

[abcdefghijklmnopqrstuvwxy]

Here's an archive with my project. Currently there is a segmentation bug that I cannot locate.

To compile just type make. But you need SDL installed and you also need SDL_image which is an extension.