6
\$\begingroup\$

I created a Sliding Number Tile Game, and am looking for input on how to make the code better organized & more efficient.

#include <iostream>
#include <random>
#include <algorithm>
#include <chrono> 
#include <windows.h>
#include <conio.h>

using namespace std;

void initializeBoard(char pB[][3]);
void printBoard(char pB[][3], bool truths[][3]);
bool isBoardSolved(char pB[][3], bool truths[][3]);
void slideTile(char pB[][3], char move);
void scrambleBoard(char pB[][3]);

int main()
{
    char pB[3][3];
    bool truths[3][3];

    initializeBoard(pB);
    isBoardSolved(pB, truths);
    printBoard(pB, truths);
    if (isBoardSolved(pB, truths) == true)
    {
        cout << "isBoardSolved(): true" << endl;
    }
    else if (isBoardSolved(pB, truths) == false)
    {
        cout << "isBoardSolved(): false" << endl;
    }
    system("PAUSE");
    cout << "Scrambling board..." << endl;
    scrambleBoard(pB);
    isBoardSolved(pB, truths);
    printBoard(pB, truths);
    cout << "Scrambling complete." << endl;
    system("PAUSE");

    system("CLS");
    printBoard(pB, truths);

    char move = ' ';
    bool check = false;
    if (check == true)
    {
        cout << "isBoardSolved(): true" << endl << endl;
    }
    else if (check == false)
    {
        cout << "isBoardSolved(): false" << endl << endl;
    }

    int key = 0;
    while ((move != 27) && (check != true))
    {
        cout << "Use arrow keys or WASD to move tiles. Press 'Esc' to quit" << endl;
        move = _getch();
        slideTile(pB, move);
        system("CLS");
        check = isBoardSolved(pB, truths);
        printBoard(pB, truths);
        if (check == true)
        {
            cout << "isBoardSolved(): true" << endl << endl;
        }
        else if (check == false)
        {
            cout << "isBoardSolved(): false" << endl << endl;
        }
    }

    if (check == true)
    {
        cout << "You won!!" << endl;
    }
    else
    {
        cout << "Game ended." << endl;
    }

    cout << "Thanks for playing!" << endl;

    return 0;
}

void initializeBoard(char pB[][3])
{
    int count = 49;
    for (int c = 0; c <= 2; c++)
    {
        for (int d = 0; d <= 2; d++)
        {
            pB[c][d] = count;
            count++;
        }
    }
    pB[2][2] = '*';
}

void printBoard(char pB[][3], bool truths[][3])
{
    HANDLE hConsole;
    hConsole = GetStdHandle(STD_OUTPUT_HANDLE);

    cout << endl;
    for (int c = 0; c <= 2; c++)
    {
        for (int d = 0; d <= 2; d++)
        {
            if (truths[c][d] == true)
            {
                SetConsoleTextAttribute(hConsole, FOREGROUND_GREEN | BACKGROUND_RED | BACKGROUND_BLUE | BACKGROUND_GREEN);
            }
            else if (truths[c][d] == false)
            {
                SetConsoleTextAttribute(hConsole, FOREGROUND_RED | BACKGROUND_RED | BACKGROUND_BLUE | BACKGROUND_GREEN);
            }
            cout << pB[c][d] << " ";
        }
        cout << endl;
    }
    cout << endl;
    SetConsoleTextAttribute(hConsole, FOREGROUND_RED | FOREGROUND_GREEN | FOREGROUND_BLUE);
}

bool isBoardSolved(char pB[][3], bool truths[][3])
{
    char correctBoard[3][3] = { { '1','2','3' },{ '4','5','6' },{ '7','8','*' } };
    bool proof = true;

    for (int c = 0; c <= 2; c++)
    {
        for (int d = 0; d <= 2; d++)
        {
            if (pB[c][d] != correctBoard[c][d])
            {
                truths[c][d] = false;
                proof = false;
            }
            else
            {
                truths[c][d] = true;
            }
        }
    }

    return proof;
}

void slideTile(char pB[][3], char move)
{
    int x = 0;
    int y = 0;

    for (int c = 0; c <= 2; c++)
    {
        for (int d = 0; d <= 2; d++)
        {
            if (pB[c][d] == '*')
            {
                x = c;
                y = d;
            }
        }
    }

    switch (move)
    {
    case 's':
    case 'S':
    case 80:
        if (x != 0)
        {
            pB[x][y] = pB[x - 1][y];
            pB[x - 1][y] = '*';
        }
        break;
    case 'w':
    case 'W':
    case 72:
        if (x != 2)
        {
            pB[x][y] = pB[x + 1][y];
            pB[x + 1][y] = '*';
        }
        break;
    case 'd':
    case 'D':
    case 77:
        if (y != 0)
        {
            pB[x][y] = pB[x][y - 1];
            pB[x][y - 1] = '*';
        }
        break;
    case 'a':
    case 'A':
    case 75:
        if (y != 2)
        {
            pB[x][y] = pB[x][y + 1];
            pB[x][y + 1] = '*';
        }
        break;
    case 'e':
    case 'E':
        initializeBoard(pB);
    }
}

void scrambleBoard(char pB[][3])
{
    char items[9] = { '1','2','3','4','5','6','7','8','*' };
    unsigned seed = chrono::system_clock::now().time_since_epoch().count();
    shuffle(begin(items), end(items), default_random_engine(seed));
    int count = 0;
    int counter = 0;

    while (counter == 0)
    {
        for (int c = 0; c <= 9; c++)
        {
            if (items[c] > items[c + 1])
            {
                counter++;
            }
        }

        if ((counter % 2) == 1)
        {
            shuffle(begin(items), end(items), default_random_engine(seed));
            counter = 0;
        }
    }

    for (int c = 0; c <= 2; c++)
    {
        for (int d = 0; d <= 2; d++)
        {
            pB[c][d] = items[count];
            count++;
        }
    }
}
\$\endgroup\$
5
\$\begingroup\$

Boolean comparisons

Instead of:

if (isBoardSolved(pB, truths) == true) { ... }

else if (isBoardSolved(pB, truths) == false) { ... }

The natural way to write is:

if (isBoardSolved(pB, truths)) { ... }

else if (!isBoardSolved(pB, truths)) { ... }

Apply the same logic everywhere in the code.

Unnecessary evaluations

In this code, there are two unnecessary evaluations:

if (isBoardSolved(pB, truths) == true)
{
    cout << "isBoardSolved(): true" << endl;
}
else if (isBoardSolved(pB, truths) == false)
{
    cout << "isBoardSolved(): false" << endl;
}

The first unnecessary evaluation is isBoardSolved(pB, truths), assuming the return value hasn't changed between the two calls. It could call once before the if-else, and reuse the returned value in the two branches.

But you don't need an else-if: isBoardSolved returns a bool, so it can only have two possible values, simplifying to:

if (isBoardSolved(pB, truths))
{
    cout << "isBoardSolved(): true" << endl;
}
else
{
    cout << "isBoardSolved(): false" << endl;
}

Apply the same logic everywhere in the code.

Avoid system

It's good to avoid calling system as much as possible. And it should not be used with relative paths, as that makes the program subject to path injection attacks.

Pointless code

This is strange and pointless:

bool check = false;
if (check == true)

Magic numbers

The numbers in these statements are "magic":

int count = 49;

// ...

case 's':
case 'S':
case 80:
    // ...
case 'w':
case 'W':
case 72:
    // ...
case 'd':
case 'D':
case 77:
    // ...
case 'a':
case 'A':
case 75:
    // ...

They are obviously important for some reason, but it's not clear what they are. You could give them meaning by putting them in constants with descriptive names.

\$\endgroup\$
  • \$\begingroup\$ Not only do constants make it easier to read an understand code, but where the constant is widely used it makes it easier to update a program. An example is that you use 3 in many array declarations, it would be better if 3 was changed to a constant with a meaningful name because you would only have to change the constant in one place to expand the functionality of the program. \$\endgroup\$ – pacmaninbw Mar 16 '16 at 14:10
  • \$\begingroup\$ Janos is quite correct about using system(), another reason not to use it is that anytime you make a system call, whether it is system or some other call, your program is swapped out and stops executing until the Operating System gets back to your program. \$\endgroup\$ – pacmaninbw Mar 16 '16 at 14:17
  • \$\begingroup\$ Thank you both, I followed you advice and made the suggested changes. \$\endgroup\$ – Michelfrancis Bustillos Mar 17 '16 at 15:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.