I am starting a software engineering degree soon and have been practicing the last couple months. I have posted some code I wrote for a basic Tic Tac Toe game below (C++). I have read the worst thing to do is to get into bad habits early on, so please can you read over the code and tell me what I should be doing differently? Be as brutally honest as possible, I care more about my future than feelings. Bad practices, bad code, bad formatting, anything.

\$\begingroup\$Guys, you have all been amazing!! Thank you so so much for taking the time to read, test and reply to my post! I will be making the changes suggested in this post, be fixing the 2 embarrassing bugs and I will (if it's okay to do) repost the code to see if I got everything right. Thank you all so much!\$\endgroup\$
– DeadPixelJun 13 '15 at 7:21

4 Answers
4

Use the appropriate #includes

This code uses numeric_limits which is actually defined in <limits>. Even if your code compiles that way, it's probably only because one of the other files happened to include it. You can't and shouldn't rely on that, though.

Use objects

You have a board structure and then separate functions printBoard and checkWinner that operate on board data. With only a slight syntax change, you would have a real object instead of C-style code written in C++. You could declare a TicTacToe object and then play, print and checkWinner could all be member functions.

Separate I/O from program logic

Right now every individual function has both game logic and I/O. It's often better design to separate the two so that the game logic is independent of the I/O with the user.

Use const where practical

I would not expect the checkWinner or printBoard routines to alter the underlying board on which they operate, and indeed they do not. You should make this expectation explicit by using the const keyword:

void printBoard(const char board[3][3]);

This declares that the printBoard will not modify the passed board, making it clear to both the compiler and to the human reader of your code.

Create a function rather than repeating code

In several places in the code, a prompt is printed, then a answer read, then the input cleared. Instead of repeating it, make it into a function.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. Having it inside every function is only slightly better. For instance, it's entirely superfluous in the playGame function and should be omitted. For this program, I'd advocate removing it everywhere and using the std:: prefix where needed.

Think of the user

It's not intuitively obvious how to enter a move when the game first starts. It would be better to consider the user and to offer a prompt (at least at the beginning) showing how the squares are numbered.

Fix the bug

If a player enters a square that is already filled, the program simply hangs. That's not good behavior and must be considered a bug.

Consider altering your algorithm

Right now, the checkWinner routine is only called if more than four moves have been made. Since this code is not particularly performance sensitive, why not simply call it every time? It would make the code a little simpler to read and the extra time taken will almost certainly never be noticed.

Declare the loop exit condition at the top

The for loop inside playGame currently says this:

for (int plays = 1; plays < 10; plays++)

Reading that line, we would conclude that the play continues until plays >= 10. However, way down at the bottom of the loop is a break that occurs if one player has won. Rather than forcing the reader of the code to examine every line, it's better if you simply declare loop exit conditions completely and honestly at the top.

Put conditional statements on a separate line

The code currently includes a number of places where something like this is done:

if (l < 2) sLine += "|";

It makes things a little bit harder to read than if you put them on separate lines like this:

if (l < 2) {
sLine += "|";
}

Further, especially when you are beginning, it's useful to always put the curly braces there. Doing so will make your intentions clear to both readers of the code and the compiler and can reduce the possibility for certain kinds of subtle bugs like this:

for (int i = 0; i < 3; ++i)
f[k] = k*2;
g[k] = f*7;

By the indentation, one would expect that both statements are executed every loop iteration, but the subtle lack of braces means that the compiler will do something else entirely.

Use meaningful variable names

Your function names are descriptive and good enough, but the variable names are not so good. In particular the printBoard routine uses a loop counter named l which is a particularly bad choice of variable name. It's too easily mistaken for the digit 1 or the letter i.

Eliminate "magic numbers"

This code has a number of "magic numbers," that is, unnamed constants such as 2, 9, 10, etc. Generally it's better to avoid that and give such constants meaningful names. That way, if anything ever needs to be changed, you won't have to go hunting through the code for all instances of "3" and then trying to determine if this particular 3 is relevant to the desired change or if it is some other constant that happens to have the same value.

Eliminate return 0 at the end of main

When a C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no reason to put return 0; explicitly at the end of main.

\$\begingroup\$Yes, thank you!! That is exactly what I was looking for! Thank you so much, man! I really appreciate the time you put into your reply. I will go back and make changes based on your knowledge and will make a conscious effort to incorporate your advice into all of my future code! Again, thank you so much! Not many people would put in the effort you did for a beginner stranger.\$\endgroup\$
– DeadPixelJun 12 '15 at 16:23

1

\$\begingroup\$I appreciate the kind words, but in fact, a lot of people would put in that kind of effort for a beginner stranger. That's what CodeReview is all about! I'm sure you'll "pay it forward" as your career progresses, too.\$\endgroup\$
– EdwardJun 12 '15 at 16:30

\$\begingroup\$I am glad to hear it, I am new to the community! The bug was really stupid and embarrassing, lack of testing after I included the numeric_limits code. I hope one day I am skilled enough to make valid contributions to the community.\$\endgroup\$
– DeadPixelJun 12 '15 at 16:36

Prefer prefix increment

You'll often see people complain when you use i++ instead of ++i in for loops.

This is because these are two different operators with different semantics.

In the case of lightweight integers and pointers, there is no difference,
but there can actually be a performance penalty or even bugs due to using i++.
This is particularly true for pure input iterators, such as istream_iterator.

Avoid declaring multiple variables on the same line

int x = 0, y = 0;

Many people consider this bad style, because it gets very confusing for more complex types.

For example, with pointers:

int* x = NULL, y = NULL;

This code is incorrect, because x is a pointer, but y is a normal int. The corrected version is: