I had a quick look and below are my first findings.
In general, code looks quite neat.
I got it to compile, but it gave me a warning (always turn on as many warnings in the compiler as possible, anything it find saves you the trouble of debugging it)
prog.cpp: In function 'bool set_tiles(Tile**)':
prog.cpp:129:1: warning: no return statement in function returning non-void [-Wreturn-type]
}
This means 'set_tiles' may return any value, including '0' (false) even if loading went well.
Easily solved with "return true;" at the end of the function.
Running it crashed immediately. It happened in "tiles[t]->show();" and this takes some steps to explain. Just going from top to bottom through that routine.
FILE* filePointer;
- filePointer=fopen("Pics/pacmanMap.txt", "r");
+ filePointer = fopen("Pics/pacmanMap.txt", "rt");
+ if (filePointer == nullptr) return false;
I used 'diff', a program to output changes between files. Lines starting with a '-' are in your file but I removed them, lines starting with '+' were added by me.
A changed line gets a -, and then a +, so you can easily compare both versions.
So above I edited the "filePointer=fopen" line, adding some spaces around the =, and a "t" with the "r" (since text files are always opened with a "t" modifier).
I also added a line that checks the return value of the 'fopen' call (always check things for errors, it will save you hours of debugging).
counter ++;
+// You don't check whether tiles[counter] exists. That can cause memory
+// corruption!
+ if (counter == TOTAL_TILES) break; // Abort reading if all tiles read.
If you reach the end of an array, tiles[counter] will appear to continue to work, but actually corrupts other values. Better check boundaries :)
This solves the problem of reading too many tiles (if your 0/1 input file is too big for some reason). My crash was however reading too few digits. That is solved as follows
Tile *tiles[ TOTAL_TILES ];
+ // Here 'tiles' is random garbage, it's better to initialize it.
+ // (I may be wrong here, maybe C++ does initialize it. However, better safe than sorry.)
+ for (int t = 0; t < TOTAL_TILES; t++) tiles[t] = nullptr;
Make sure tiles[] is fully initialized. Also
for( int t = 0; t < TOTAL_TILES; t++ )
{
- tiles[ t ]->show();
+ // And here is another problem, what if you didn't load enough tiles??
+ // Either check 'counter' after fully loading the file, or be more careful here:
+ if (tiles[t] != nullptr) tiles[t]->show();
Just skip if you run into a 'nullptr'. Alternatively, you can just 'break', and terminate the loop, since there are not going to be any more tiles to show.
After this, the window stayed up, but crashed again when typing ESC. Perhaps some other unchecked call????
Other stuff I ran into:
I hate '=' in a condition (I once spend 45 minutes finding one), so I do:
- //While going through all the characters...
- while( ( t = getc( filePointer ) )!= EOF )
+// //While going through all the characters...
+// while( ( t = getc( filePointer ) )!= EOF )
+
+// I don't like assignments inside conditions, so I do the following instead:
+ while (true)
{
+ t = getc(filePointer);
+ if (t == EOF) break;
The '0' case and the '1' case while loading look very similar, you can merge them with
- if( t == '0' )
+ if( t == '0' || t == '1' )
{
//Make the type of the tile = 0
- tileType = 0;
+ tileType = t - '0';
tiles[ counter ] = new Tile( x, y, tileType );
and deleting the '1' case.
About your error reporting. You can use a string as error message, for example
-bool init()
+// You can also pass a constant string back to the caller indicating what went wrong:
+const char *init()
{
//Initialize all SDL subsystems
if( SDL_Init( SDL_INIT_EVERYTHING ) == -1 )
{
- return false;
+ return "SDL_init failed";
}
......
//If everything initialized fine
- return true;
+ // C++11 and higher uses 'nullptr' instead of NULL.
+ return nullptr;
}
The function returns a "const char *" instead of a boolean, and you can catch and print it with
- if( init() == false )
- {
+ const char *return_value = init();
+ if (return_value != nullptr) {
+ // Make sure you add a \n at the end of the string that you print otherwise you don't get a line.
+ // I use 'stderr', as it is for error messages.
+ // The normal 'printf' uses stdout, which is for regular output.
+ fprintf(stderr, "Error: %s\n", return_value);
return 1;
}
However, something like below should also work:
//Load the files
if( load_files() == false )
{
+ printf("load_files() failed\n"); // <-- should also work
return 1;
}
I don't what platform you use, but Windows is notorious for not showing the stdout and stderr streams in the GUI.
std::string is passed differently, normally:
+// std::strings are normally passed by const reference, to avoid copying the object:
- SDL_Surface * load_image( std::string filename )
+// SDL_Surface * load_image( const std::string &filename )
//While the user hasn't quit
+ // Booleans can also be tested directly instead of comparing with false or true,
+ // like "while (!quit)"
while( quit == false )
{
SDL_Delay( 1000 / FRAMES_PER_SECOND - fps.get_ticks() - 5 );
}
+ // Does the "- 5" miss in the condition above? (which was chopped off by diff, sorry about that)
+ // Perhaps compute the value instead, and then test?
+ // int delay = 1000 / FRAMES_PER_SECOND - fps.get_ticks() - 5;
+ // if (delay > 0) SDL_Delay(delay);
Last but not least,
Tile *tiles[ TOTAL_TILES ]; // Lots of very small allocations, why not do
Tile tiles[TOTAL_TILES]; // One big array containing all 'Tile' objects.
//Tile::Tile() can be empty, move initialization eg to Tile::load(x, y, tiletype), so you can do
tiles[counter].load(x, y, tileType);
Hope this helps.