HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

SlidingNumberTile Program Efficiency

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
slidingnumbertileefficiencyprogram

Problem

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

#include 
#include 
#include 
#include  
#include 
#include 

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  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++;
        }
    }
}

Solution

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.

Code Snippets

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

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

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

Context

StackExchange Code Review Q#122937, answer score: 5

Revisions (0)

No revisions yet.