Learning c++ - Please critique this code

[Voltage]

I've just finished this Tic Tac Toe console app in C++.  It works, but I'd like some ideas/advice on best practices.

The idea being that I swap out FreeBASIC as my default tool, and replace it with VS c++ in the long run.

Code: [Select]

// TicTacToe.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"
#include <iostream>
#include <string>

using namespace std;

#define TTT_X 1
#define TTT_O 2

class classTicTacToe
{
public:
int board[9];

classTicTacToe()
{
for (int a=0;a<9;a++)
board[a]=0;
}
~classTicTacToe() {}
void PutX(int x, int y)
{
if (x>=0 && x<3 && y>=0 && y<3) board[y*3+x] = TTT_X;
}
void PutO(int x, int y)
{
if (x>=0 && x<3 && y>=0 && y<3) board[y*3+x] = TTT_O;
}
void Clear(int x, int y)
{
if (x>=0 && x<3 && y>=0 && y<3) board[y*3+x] = 0;
}

int Check(int x, int y)
{
if (x>=0 && x<3 && y>=0 && y<3)
return board[y*3+x];
else
return 0;
}

int GameWon(void)
{
// Check for horizontal wins
for (int y=0;y<3;y++)
{
if (Check(0,y)==TTT_X && Check(1,y)==TTT_X && Check(2,y)==TTT_X) return TTT_X;
if (Check(0,y)==TTT_O && Check(1,y)==TTT_O && Check(2,y)==TTT_O) return TTT_O;
}

// Check for Vertical wins
for (int x=0;x<3;x++)
{
if (Check(x,0)==TTT_X && Check(x,1)==TTT_X && Check(x,2)==TTT_X) return TTT_X;
if (Check(x,0)==TTT_O && Check(x,1)==TTT_O && Check(x,2)==TTT_O) return TTT_O;
}

// Check for Diagonal wins
if (Check(0,0)==TTT_X && Check(1,1)==TTT_X && Check(2,2)==TTT_X) return TTT_X;
if (Check(0,0)==TTT_O && Check(1,1)==TTT_O && Check(2,2)==TTT_O) return TTT_O;
if (Check(2,0)==TTT_X && Check(1,1)==TTT_X && Check(0,2)==TTT_X) return TTT_X;
if (Check(2,0)==TTT_O && Check(1,1)==TTT_O && Check(0,2)==TTT_O) return TTT_O;

// else return 0
return 0;
}

int GameDrawn(void)
{
for (int y=0;y<3;y++)
for (int x=0;x<3;x++)
if (Check(x,y)==0) return 0;

// else return true
return -1;
}

char GetChar(int x, int y)
{
if (Check(x,y)==TTT_X) return 'X';
if (Check(x,y)==TTT_O) return 'O';
return ' ';
}

void DrawBoard()
{
cout << "  a b c" << endl;
cout << "1 " << GetChar(0,0) << "|" << GetChar(1,0) << "|" << GetChar(2,0) << endl;
cout << "  -+-+-" << endl;
cout << "2 " << GetChar(0,1) << "|" << GetChar(1,1) << "|" << GetChar(2,1) << endl;
cout << "  -+-+-" << endl;
cout << "3 " << GetChar(0,2) << "|" << GetChar(1,2) << "|" << GetChar(2,2) << endl;
}
};

int _tmain(int argc, _TCHAR* argv[])
{
classTicTacToe myTTT;
string inputBuffer;

int whosTurn = 0;
int exitPressed = 0;

while (!exitPressed)
{
myTTT.DrawBoard();
if (whosTurn==0)
cout << "X's turn" << endl;
else
cout << "O's turn" << endl;

cout << endl << "Enter a move or x to exit.  Ex a1<enter>" << endl;
std::getline(std::cin, inputBuffer);
cout << endl << endl;


if (inputBuffer=="x")
exitPressed=1;
else
{
// Check for a valid move
if (inputBuffer.length()!=2)
cout << "Please enter a 2 character board location and press enter.  Ex b3<enter>" << endl;
else
{
char iB1=inputBuffer[0];
char iB2=inputBuffer[1];
int x=-1;
int y=-1;
if (iB1=='a') x=0;
if (iB1=='b') x=1;
if (iB1=='c') x=2;
if (iB2=='1') y=0;
if (iB2=='2') y=1;
if (iB2=='3') y=2;
if (x<0 || y<0)
cout << "Invalid move. Bad coordinates." << endl;
else
{
// Check if the square is free
if (myTTT.Check(x,y)!=0)
cout << "Invalid move.  Square already occupied." << endl;
else
{
if (whosTurn==0)
{
whosTurn=1;
myTTT.PutX(x,y);
}
else
{
whosTurn=0;
myTTT.PutO(x,y);
}
}
}
}
}
// Check for a draw
if (myTTT.GameDrawn())
{
myTTT.DrawBoard();
cout << "Game drawn!  Press <enter> to finish." << endl;
std::getline(std::cin, inputBuffer);
exitPressed=1;
}

// Check for a win
if (int winner=myTTT.GameWon())
{
myTTT.DrawBoard();
if (winner==TTT_X)
cout << "X Wins the game!  Press <enter> to finish." << endl;
else
cout << "O Wins the game!  Press <enter> to finish." << endl;
std::getline(std::cin, inputBuffer);
exitPressed=1;
}
}
return 0;
}


[stormbringer]

a few quick comments:

1) take your class methods out of the class declaration. It may not make a big change here, but in the long run, your methods may become complex and render your class unreadable

2) avoid endless "ifs", use switch()/case instead

3) avoid return codes like 0 and -1, use symbols instead (like "true" and "false" or "TRUE" and "FALSE"

4) avoid returning "int" types if you are in C++, use "bool" instead. If you are in C, the define yourself a "bool" type (even if behind the scenes it is an "int" anyway". this will render your code much more readable

5) although your code is quite clear here, I would have added more comments. Especially if you are a beginner. C/C++ syntax can become unreadable very quickly if you take advantage of the laguage syntax capabilities. Help yourself and add more comments

[Jim]

Maybe you could create the cell as a class?  In real life I wouldn't do that, but it might be more interesting as a learning thing.  Right now you only have one class.

Jim

[hellfire]

Since you're coming from a completely case-insensitive compiler, it's a good time to think about your coding guidelines.
There are usually no good or bad ones, but some are more pleasant to read.
For example I wouldn't want to use "class" as a prefix for all my classes.
Personally I don't care if some object is a struct, typedef or class, so I don't use prefixes here.
On the other hand it makes sense when member-variables are recognizable as members by their name (avoids some bugs that are difficult to find).
There are many things that could be structurally improved, like dividing "board", "logic" and "input" (see mvc-pattern) - but then again it's just tictactoe...

[Allosentient]

One of the best things you can do to make your programs better is to split large functions into smaller ones, and name them logically.  May not automatically make them better but it will make it easier for you to make them better.  Pretty much the design principle of "small functions are better".

For example (pseudocode):

Instead of this:

Code: [Select]

for(int i = 0; i < count; i++)
{
     -50 Lines of Code-
}

Do this:

Code: [Select]

for(int i = 0; i < count; i++)
{
      ProcessCode();
}

ProcessCode()
{
      -Your 50 lines of code-
}


[Voltage]

Thanks for the tips guys.  There's some good advice there.

I'll try something a little more scene related for my next attempt.  Maybe a plasma.  Just to get used to setting up a window and pushing pixels in c++.

[benny!]

Thanks for the tips guys.  There's some good advice there.

I'll try something a little more scene related for my next attempt.  Maybe a plasma.  Just to get used to setting up a window and pushing pixels in c++.

For your next attempt it might be a good to think about a common
framework you create for doing graphics stuff. So a framework you
can use as a start for every graphic effect you want to write (plasma,
starfield, wobbler etc.)

Have a look at the brilliant source of Hellfire's wire programm. Think
this is pretty well structured and designed.

[Voltage]

Good idea about a framework Benny.

.... regarding that source....  I have both versions downloaded and will definately reference them while I'm learning this stuff.

[LittleWhite]

Hello,

As it was already write :
Write your code outside the declarations.
Often we create always two files for our class. One .cpp with the code of the function. One .h ( or .hpp ) for the declaration of the class and the declaration of the functions.
We used to create these two files each time that we create a class.

It was already write , but it is better to remind it , write a lot of comments. It's not only for the beginners. It because in two months , if you look your code, you can have some difficulties for understand it. Don't hesitate to write an short explanation of the function ... just before the function , and if possible to clarify the difficult part.

For the TicTacToe, you know that you can have value ( for the position ) under 0 , so you can use the unsigned int. It's not an optimisation ... but I think it can be better , and maybe avoid some test like 'if under 0' ( but it can be dangerous too sometimes.

Normally , the main must contain maximum 20 lines of code... Just for intialise and call other functions...

Ok , maybe my comments are more for the big program , and not really when you start to code. But one day you will find this useful ( I hope ).

And , I good advice is too set Visual Studio to show you all warning. It in the option of Visual , but I forget where. ( Option of the project ).
And don't let the warning when you compile the code. Try as possible to correct the warning , sometimes it avoids some bugs.

Hope this help