patterncppMinor
SlidingNumberTile Program Efficiency
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:
The natural way to write is:
Apply the same logic everywhere in the code.
Unnecessary evaluations
In this code, there are two unnecessary evaluations:
The first unnecessary evaluation is
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:
Apply the same logic everywhere in the code.
Avoid
It's good to avoid calling
Pointless code
This is strange and pointless:
Magic numbers
The numbers in these statements are "magic":
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.
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
systemIt'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.