Sokoban Clone using SFML

Question

I am new to C++ and game programming. I am using SFML for graphics. I wrote a clone of Sokoban in my free time and to be honest, I am not happy about the result. I feel like I reused some code too many times. Please don't hesitate to criticize every bit of it. Also I am open to advices about the improvements and updates about the game.

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

using namespace std;

vector<char> sokobanMap[20]; // The sokoban map
int level;
int row;
int col;
int totalMoves = 0;
int currentPosX;
int currentPosY;

bool CheckWinCondition() {
    // If all boxes are placed, win
    for (int i = 0; i < row; i++) {
        for (int j = 0; j < col; j++) {
            if (sokobanMap[i][j] == '@') {
                return false;
            }
        }
    }
    return true;
}
void PushBox(int direction) {
    // If box is unplaced, when the box is pushed the new spot is a free road
    // If box is placed, new spot is a target.
    // If the spot which the box will go to is T, it becomes a placed box, vice verca.
    switch (direction)
    {
    case 0:{
        if (sokobanMap[currentPosX - 1][currentPosY]=='@') {
            sokobanMap[currentPosX - 1][currentPosY] = 'P';
            if (sokobanMap[currentPosX - 2][currentPosY] == 'T') {
                sokobanMap[currentPosX - 2][currentPosY] = 'X';
            }
            else {
                sokobanMap[currentPosX - 2][currentPosY] = '@';
            }
        }
        else {
            sokobanMap[currentPosX - 1][currentPosY] = 'R';
            if (sokobanMap[currentPosX - 2][currentPosY] == 'T') {
                sokobanMap[currentPosX - 2][currentPosY] = 'X';
            }
            else {
                sokobanMap[currentPosX - 2][currentPosY] = '@';
            }
        }
        break;
    }
    case 1: {
        if (sokobanMap[currentPosX + 1][currentPosY] == '@') {
            sokobanMap[currentPosX + 1][currentPosY] = 'P';
            if (sokobanMap[currentPosX + 2][currentPosY] == 'T') {
                sokobanMap[currentPosX + 2][currentPosY] = 'X';
            }
            else {
                sokobanMap[currentPosX + 2][currentPosY] = '@';
            }
        }
        else {
            sokobanMap[currentPosX + 1][currentPosY] = 'R';
            if (sokobanMap[currentPosX + 2][currentPosY] == 'T') {
                sokobanMap[currentPosX + 2][currentPosY] = 'X';
            }
            else {
                sokobanMap[currentPosX + 2][currentPosY] = '@';
            }
        }
        break;
    }
    case 2: {
        if (sokobanMap[currentPosX][currentPosY + 1] == '@') {
            sokobanMap[currentPosX][currentPosY + 1] = 'P';
            if (sokobanMap[currentPosX][currentPosY + 2] == 'T') {
                sokobanMap[currentPosX][currentPosY + 2] = 'X';
            }
            else {
                sokobanMap[currentPosX][currentPosY + 2] = '@';
            }
        }
        else {
            sokobanMap[currentPosX][currentPosY + 1] = 'R';
            if (sokobanMap[currentPosX][currentPosY + 2] == 'T') {
                sokobanMap[currentPosX][currentPosY + 2] = 'X';
            }
            else {
                sokobanMap[currentPosX][currentPosY + 2] = '@';
            }
        }
        break;
    }
    case 3: {
        if (sokobanMap[currentPosX][currentPosY - 1] == '@') {
            sokobanMap[currentPosX][currentPosY - 1] = 'P';
            if (sokobanMap[currentPosX][currentPosY - 2] == 'T') {
                sokobanMap[currentPosX][currentPosY - 2] = 'X';
            }
            else {
                sokobanMap[currentPosX][currentPosY - 2] = '@';
            }
        }
        else {
            sokobanMap[currentPosX][currentPosY - 1] = 'R';
            if (sokobanMap[currentPosX][currentPosY - 2] == 'T') {
                sokobanMap[currentPosX][currentPosY - 2] = 'X';
            }
            else {
                sokobanMap[currentPosX][currentPosY - 2] = '@';
            }
        }
        break;
    }
    default:
        break;
    }
}
bool CheckIfMoveIsLegal(int direction) {
    //Checks all illegal posibilities. If non of them is true, return true.
    switch (direction)
    {
    case 0: {
        if (sokobanMap[currentPosX - 1][currentPosY] == '#') {
            return false;
        }
        else if (sokobanMap[currentPosX - 1][currentPosY] == '@' && sokobanMap[currentPosX - 2][currentPosY] == '@') {
            return false;
        }
        else if (sokobanMap[currentPosX - 1][currentPosY] == '@' && sokobanMap[currentPosX - 2][currentPosY] == '#') {
            return false;
        }
        else if (sokobanMap[currentPosX - 1][currentPosY] == 'X' && sokobanMap[currentPosX - 2][currentPosY] == 'X') {
            return false;
        }
        else if (sokobanMap[currentPosX - 1][currentPosY] == 'X' && sokobanMap[currentPosX - 2][currentPosY] == '#') {
            return false;
        }
        else if (sokobanMap[currentPosX - 1][currentPosY] == 'X' && sokobanMap[currentPosX - 2][currentPosY] == '@') {
            return false;
        }
        else if (sokobanMap[currentPosX - 1][currentPosY] == '@' && sokobanMap[currentPosX - 2][currentPosY] == 'X') {
            return false;
        }
        else {
            return true;
        }
        break;
    }
    case 1: {
        if (sokobanMap[currentPosX + 1][currentPosY] == '#') {
            return false;
        }
        else if (sokobanMap[currentPosX + 1][currentPosY] == '@' && sokobanMap[currentPosX + 2][currentPosY] == '@') {
            return false;
        }
        else if (sokobanMap[currentPosX + 1][currentPosY] == '@' && sokobanMap[currentPosX + 2][currentPosY] == '#') {
            return false;
        }
        else if (sokobanMap[currentPosX + 1][currentPosY] == 'X' && sokobanMap[currentPosX + 2][currentPosY] == 'X') {
            return false;
        }
        else if (sokobanMap[currentPosX + 1][currentPosY] == 'X' && sokobanMap[currentPosX + 2][currentPosY] == '#') {
            return false;
        }
        else if (sokobanMap[currentPosX + 1][currentPosY] == 'X' && sokobanMap[currentPosX + 2][currentPosY] == '@') {
            return false;
        }
        else if (sokobanMap[currentPosX + 1][currentPosY] == '@' && sokobanMap[currentPosX + 2][currentPosY] == 'X') {
            return false;
        }
        else {
            return true;
        }
        break;
    }
    case 2: {
        if (sokobanMap[currentPosX][currentPosY + 1] == '#') {
            return false;
        }
        else if (sokobanMap[currentPosX][currentPosY + 1] == '@' && sokobanMap[currentPosX][currentPosY + 2] == '@') {
            return false;
        }
        else if (sokobanMap[currentPosX][currentPosY + 1] == '@' && sokobanMap[currentPosX][currentPosY + 2] == '#') {
            return false;
        }
        else if (sokobanMap[currentPosX][currentPosY + 1] == 'X' && sokobanMap[currentPosX][currentPosY + 2] == 'X') {
            return false;
        }
        else if (sokobanMap[currentPosX][currentPosY + 1] == 'X' && sokobanMap[currentPosX][currentPosY + 2] == '#') {
            return false;
        }
        else if (sokobanMap[currentPosX][currentPosY + 1] == 'X' && sokobanMap[currentPosX][currentPosY + 2] == '@') {
            return false;
        }
        else if (sokobanMap[currentPosX][currentPosY + 1] == '@' && sokobanMap[currentPosX][currentPosY + 2] == 'X') {
            return false;
        }
        else {
            return true;
        }
        break;
    }
    case 3: {
        if (sokobanMap[currentPosX][currentPosY - 1] == '#') {
            return false;
        }
        else if (sokobanMap[currentPosX][currentPosY - 1] == '@' && sokobanMap[currentPosX][currentPosY - 2] == '@') {
            return false;
        }
        else if (sokobanMap[currentPosX][currentPosY - 1] == '@' && sokobanMap[currentPosX][currentPosY - 2] == '#') {
            return false;
        }
        else if (sokobanMap[currentPosX][currentPosY - 1] == 'X' && sokobanMap[currentPosX][currentPosY - 2] == 'X') {
            return false;
        }
        else if (sokobanMap[currentPosX][currentPosY - 1] == 'X' && sokobanMap[currentPosX][currentPosY - 2] == '#') {
            return false;
        }
        else if (sokobanMap[currentPosX][currentPosY - 1] == 'X' && sokobanMap[currentPosX][currentPosY - 2] == '@') {
            return false;
        }
        else if (sokobanMap[currentPosX][currentPosY - 1] == '@' && sokobanMap[currentPosX][currentPosY - 2] == 'X') {
            return false;
        }
        else {
            return true;
        }
        break;
    }
    default:
        break;
    }
}
void MovePlayer(int direction) { 
    // Check if the move is legal
    // Update the counter
    // The move is legal if:
    // 1- There is a free spot in the same direction of the input.
    // 2- There is only one box in the same direction of the input.
    if (CheckIfMoveIsLegal(direction)) {
        totalMoves++;
        // If player is standing on a target, when he leaves the spot becomes Target; if not, free spot
        if (sokobanMap[currentPosX][currentPosY] == 'R') {  
            sokobanMap[currentPosX][currentPosY] = 'T';
        }
        else {
            sokobanMap[currentPosX][currentPosY] = '.';
        }

        switch (direction) {
        case 0: {
            if (sokobanMap[currentPosX - 1][currentPosY] == '@' || sokobanMap[currentPosX - 1][currentPosY] == 'X') {
                PushBox(direction);
            }
            else if (sokobanMap[currentPosX - 1][currentPosY] == 'T') {
                sokobanMap[currentPosX - 1][currentPosY] = 'R';
            }
            else {
                sokobanMap[currentPosX - 1][currentPosY] = 'P';
            }
            currentPosX = currentPosX - 1;
            break;
        }
        case 1: {
            if (sokobanMap[currentPosX + 1][currentPosY] == '@' || sokobanMap[currentPosX + 1][currentPosY] == 'X') {
                PushBox(direction);
            }
            else if (sokobanMap[currentPosX + 1][currentPosY] == 'T') {
                sokobanMap[currentPosX + 1][currentPosY] = 'R';
            }
            else {
                sokobanMap[currentPosX + 1][currentPosY] = 'P';
            }
            currentPosX = currentPosX + 1;
            break;
        }
        case 2: {
            if (sokobanMap[currentPosX][currentPosY+1] == '@' || sokobanMap[currentPosX][currentPosY+1] == 'X') {
                PushBox(direction);
            }
            else if (sokobanMap[currentPosX][currentPosY+1] == 'T') {
                sokobanMap[currentPosX][currentPosY+1] = 'R';
            }
            else {
                sokobanMap[currentPosX][currentPosY+1] = 'P';
            }
            currentPosY = currentPosY + 1;
            break;
        }
        case 3: {
            if (sokobanMap[currentPosX][currentPosY - 1] == '@' || sokobanMap[currentPosX][currentPosY - 1] == 'X') {
                PushBox(direction);
            }
            else if (sokobanMap[currentPosX][currentPosY - 1] == 'T') {
                sokobanMap[currentPosX][currentPosY - 1] = 'R';
            }
            else {
                sokobanMap[currentPosX][currentPosY - 1] = 'P';
            }
            currentPosY = currentPosY - 1;
            break;
        }
        }
    }
}

int main()
{



    ifstream inputFile; // Input file stream declaration
    inputFile.open("C:\\Users\\AliTeo\\Desktop\\Sokoban_levels\\Sokoban_lvl_01.txt"); // Opening the text-map

    if (!inputFile) {
        cerr << "Unable to open file Sokoban_lvl_01.txt\n"; // Handling errors
        exit(1);
    }
    /*

    First three integers:

    1-Level Number
    2-Row Number
    3-Column Number

    Row X Column character c->

    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


*/
    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);
        }
    }
    sf::RenderWindow window(sf::VideoMode(col*32, row*32), "Sokoban");
    sf::Texture wall;
    sf::Texture road; // ::TO DO::
    sf::Texture box;
    sf::Texture placedBox;
    sf::Texture player;
    sf::Texture target;

    if (!wall.loadFromFile("C:\\Users\\AliTeo\\Desktop\\Sokoban images\\wall.png")) {
        // Handle an error.
    }
    if (!box.loadFromFile("C:\\Users\\AliTeo\\Desktop\\Sokoban images\\box.png")) {
        // Handle an error.
    }
    if (!placedBox.loadFromFile("C:\\Users\\AliTeo\\Desktop\\Sokoban images\\placed_box.png")) {
        // Handle an error.
    }
    if (!player.loadFromFile("C:\\Users\\AliTeo\\Desktop\\Sokoban images\\player.png")) {
        // Handle an error.
    }
    if (!target.loadFromFile("C:\\Users\\AliTeo\\Desktop\\Sokoban images\\target.png")) {
        // Handle an error.
    }
    if (!road.loadFromFile("filename.png")) {
        // Handle an error.
    }

    // For printing on the console, for checking purposes.
    for (int i = 0; i < row; i++) {
        for (int j = 0; j < col; j++) {
            cout << sokobanMap[i][j] << " ";
        }
        cout << endl;
    }



    while (window.isOpen()) {
        sf::Event event;
        while (window.pollEvent(event)) {
            if (event.type == sf::Event::Closed) { 
                window.close(); // Close window button clicked.
            }
            else if (event.type == sf::Event::KeyPressed) {
                int direction;
                if (event.key.code == sf::Keyboard::Up) {
                    direction = 0;
                    MovePlayer(direction);
                    // Move Up
                }
                else if (event.key.code == sf::Keyboard::Down) {
                    direction = 1;
                    MovePlayer(direction);
                    // Move Down
                }
                else if (event.key.code == sf::Keyboard::Right) {
                    direction = 2;
                    MovePlayer(direction);
                    // Move Left
                }
                else if(event.key.code == sf::Keyboard::Left) {
                    direction = 3;
                    MovePlayer(direction);
                    //Move Right
                }
            }
        }
        window.clear(sf::Color::White);
        // Drawing of the map 
        int posX = 0;
        int posY = 0;
        sf::Sprite obj;
        for (int i = 0; i < row; i++) {
            for (int j = 0; j < col; j++) {
                if (sokobanMap[i][j] == '#') {
                    // Draw Wall
                    //obj.setFillColor(sf::Color::Red);
                    obj.setTexture(wall);
                    obj.setPosition(posX,posY);
                    window.draw(obj);
                }
                else if (sokobanMap[i][j] == '.') {
                    // Draw Road ::TO DO:: maybe a sprite in the future
                }
                else if (sokobanMap[i][j] == '@') {
                    // Draw Box
                    obj.setTexture(box);
                    obj.setPosition(posX, posY);
                    window.draw(obj);
                }
                else if (sokobanMap[i][j] == 'P') {
                    // Draw The Player
                    currentPosX = i;
                    currentPosY = j;
                    obj.setTexture(player);
                    obj.setPosition(posX, posY);
                    window.draw(obj);
                }
                else if (sokobanMap[i][j] == 'R') {
                    // Draw The Player On The Target
                    currentPosX = i;
                    currentPosY = j;
                    obj.setTexture(player);
                    obj.setPosition(posX, posY);
                    window.draw(obj);
                }
                else if (sokobanMap[i][j] == 'T') {
                    // Draw Target
                    obj.setTexture(target);
                    obj.setPosition(posX, posY);
                    window.draw(obj);
                }
                else if (sokobanMap[i][j] == 'X') {
                    obj.setTexture(placedBox);
                    obj.setPosition(posX, posY);
                    window.draw(obj);
                }
                posX += 32;
            }
            // Next Row
            posY += 32;
            posX = 0;
        }
        //cout << currentPosX << " " << currentPosY << endl;
        window.display();
    }

    inputFile.close();
    return 0;
}

Answer 1

#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
}

Answer 2

Don’t write using namespace std;.

You can, however, in a CPP file (not H file) or inside a function put individual using std::string; etc. (See SF.7.)

---

Put your headers in order: std first, heavy libraries next, application after that, specific to this file last. I don’t know what you have in stdafx.h, or why you listed it first.

I don’t know what sokoban is, BTW.

---

vector<char> sokobanMap[20]; // The sokoban map
int level;
int row;
int col;
int totalMoves = 0;
int currentPosX;
int currentPosY;

A bunch of global variables? Many of them are not even initialized (unless you are relying on zero initialization of statics, but you have totalMoves=0).

Maybe those things belong as instance variables of an object, and the code dealing with them would become member functions. The user I/O and other incidental code would not be in the class.

What is the significance of the magic number 20?

---

bool CheckWinCondition() {
    // If all boxes are placed, win
    for (int i = 0; i < row; i++) {
        for (int j = 0; j < col; j++) {
            if (sokobanMap[i][j] == '@') {
                return false;
            }
        }
    }
    return true;

Are row and col always indicating the legal size of the game board at this point? In that case, the individual vectors and the outer array could be sized to that, rather than being kept as separate numbers. Then you would not have to iterate via subscripts, but just use range-for to take the entire collection.

At the very least, your inner loop can be replaced with std::find_if.

---

PushBox:

I am not happy about the result. I feel like I reused some code too many times.

Indeed, it appears to follow the same form for each case. Without doing a diff, it appears that the only thing changing is the subscripts: offset by ±2,±1,0 for both x and y. Furthermore, the same values are used everywhere in the case!

As a first rough cut, simply move the common block a helper function, replacing those two values with variables computed only once. Whatever is different between uses becomes parameters.

void need_a_good_name (int Δx1, int Δx2, int Δy)
{
   const int X1 = currentPosX + Δx1;
   const int Y = currentPosY + Δy;

   auto& cell1= sokobanMap[X1][Y];
   auto& cell2= sokobanMap[X2][Y];
   if (cell1 == '@') {
        cell1 = 'P';
        cell2 = cell2 == 'T' ? 'X' : '@';
        }
    }
    else {
        cell1 = 'R';
        cell2 = cell2 == 'T' ? 'X' : '@';
    }
}

While going through that, I noticed that there were in fact two different X deltas being used in this case. I suppose for generality it needs two X and two Y deltas, but I don’t know where they go without analyzing more cases together. Leave that as an exercise for the student.

I also unraveled the inner if statements. By aliasing the cells to simple names, you can see the logic much clearer now, since you reduced the amount of repetitive noise. The inner logic assigns one of two things to the same place based on a test, so write it like that.

Now, the original place where this was taken out of can just call it with suitable values:

case 1: need_a_good_name (1,2,0);  break;

Each of the four cases is now one statement only, and you can see the whole switch/case block in its glory on the screen!

---

Well, that illustrates the general idea of how to not repeat big blobs of code.

1. things that are structurally the same: put in a function, supply the differences as parameters.
2. reusing a common subexpression, or any subexpression that is especially verbose even if just used once: put it on a separate statement and name it.

If you need to copy/paste to efficiently write your code, don’t. You just identified the piece to reuse, so reuse it properly.