Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link
added 4 characters in body
Source Link
Summer
  • 2.4k
  • 2
  • 16
  • 31

The return Boolean indicates success or failure much like fstream itself does. Then when the function ends the filestream goes out of scope and your file is closed.

The return Boolean indicates success or failure much like fstream itself does. Then when the function ends the filestream goes out of scope and your file is closed.


The return Boolean indicates success or failure much like fstream itself does. Then when the function ends the filestream goes out of scope and your file is closed.

The return Boolean indicates success or failure much like fstream itself does. Then when the function ends the filestream goes out of scope and your file is closed.


Source Link
Summer
  • 2.4k
  • 2
  • 16
  • 31

#include "stdafx.h"

This is a non-standard. I know where it comes from because I use MSVC too. PCH can be useful so I am not arguing never to use it again. However, if you don't know what PCH are read this and this. Then switch your defaults on VS to not generate them until you are building projects large enough to need them.


#include <SFML/Graphics.hpp> // For graphics
#include <iostream>
#include <fstream> // Necessary header for stream
#include <vector> // For 2D map

You are including a graphics header for graphics? fstream for stream? Consider using comments to explain things that aren't readily obvious.


##Don't Use using namespace std!##


Don't use Globals. In fact there's not much C++ going on here. Use of classes would greatly improve your code here. and I will repeat this a few more times in the review but here it would help you encapsulate your global state.


###Prefer prefix over postfix###


Don't use std::endl. "\n" is shorter and doesn't flush the stream. If you do want your stream flushed then use std::flush, thereby being explicit in your intent.


inputFile.close();
return 0;

These lines are irrelevant. RAII will close your file.

main will return on its own when reaching the end of the program.

Also its kinda late to close a file. It's best to open a file, operate on the file, close the file. A function will help wrap this for you using the aforementioned RAII.

bool readFile();{
 ifstream inputFile;
 inputFile.open("C:\\Users\\AliTeo\\Desktop\\Sokoban_levels\\Sokoban_lvl_01.txt");
 if (!inputFile) {
 cerr << "Unable to open file Sokoban_lvl_01.txt\n"; // Handling errors
 return false;
 }
 inputFile >> level >> row >> col;
 char ch;
 for (int i = 0; i < row; i++) {
 for (int j = 0; j < col; j++) {
 inputFile >> ch;
 sokobanMap[i].push_back(ch);
 }
 }
 return true;
}

The return Boolean indicates success or failure much like fstream itself does. Then when the function ends the filestream goes out of scope and your file is closed.

/*
...
Meaning of the characters in the text map:
# : Wall
. : Free spot
@ : Box
X : Placed Box
P : Player Position
R : Player On a Target
T : Target
*/

This is excellent use of documentation to explain the use of otherwise cryptic symbols. However, it comes 314 lines after the first symbol appeared. The first time I read through the code much of it was cryptic to me and I had to reread it. However all of this could be cleaned up with classes.

I would probably create a Cell class that held the state of each individual tile.

Then I would create a scoped enum to track the state of each tile similar to the way you were using symbols. However, your enum could be more explicit and readable.

enum class CellType {
 wall,
 free_spot,
 box,
 placed_box,
 player_position,
 player_on_target,
 target
}

You should use a similar enum for your directions

enum class Directions {
 up,
 right,
 down,
 left
}
lang-cpp

AltStyle によって変換されたページ (->オリジナル) /