Sign in to follow this  
wicked357

Would like an opinion on my code.

Recommended Posts

wicked357    2424
I have just created a TicTacToe game in win32 Console, using keyboard macro and gotoxy function, which both rely on Windows OS, anyways that is besides the point since I am not looking to porting it to another platform. I am mainly asking here how my coding structure, and coding habits look, I want to get some constructive critisism of my work, it is about 800 lines of coding, but that could be shortened if I wasn't so anal on my structure. For example,
for(int i = 0; i < whatever; i++) { /*do something...*/ };




in my coding I like to do...
for(int i = 0; i < whatever; i++)
{
    //do something...
};




So to the point anyone have constructive critisism that may help me in the future please feel free to let me know. Below is a link to my executible file, source, and vs2008 project files. TicTacToe v1.0 [Edited by - wicked357 on February 13, 2010 4:53:07 AM]

Share this post


Link to post
Share on other sites
klee1    166
I agree with AverageJoeSSU. One of the main things that I have been learning at school is that when you code you should optimize for readability.

I prefer:
for(int i = 0; i < whatever; i++)
{
//do something...
}

almost infinitely to the other way of writing it that you have, just because since I have seen it that way so many times it makes it very easy for me to understand and interpret what is going on in the code.

There are some things in your code though that I think could be rewritten to improve reliability/readability:

You have:

OBJECT x1, x2, x3, x4, x5, x6, x7, x8, x9;
OBJECT o1, o2, o3, o4, o5, o6, o7, o8, o9;



and then later when you're drawing the board:

//Draw X on game markers location
if((place.one == 1) && (x1.drawn == true))
{
drawX(x1.x, x1.y);
}
if((place.two == 1) && (x2.drawn == true))
{
drawX(x2.x, x2.y);
}
if((place.three == 1) && (x3.drawn == true))
{
drawX(x3.x, x3.y);
}
if((place.four == 1) && (x4.drawn == true))
{
drawX(x4.x, x4.y);
}
if((place.five == 1) && (x5.drawn == true))
{
drawX(x5.x, x5.y);
}
if((place.six == 1) && (x6.drawn == true))
{
drawX(x6.x, x6.y);
}
if((place.seven == 1) && (x7.drawn == true))
{
drawX(x7.x, x7.y);
}
if((place.eight == 1) && (x8.drawn == true))
{
drawX(x8.x, x8.y);
}
if((place.nine == 1) && (x9.drawn == true))
{
drawX(x9.x, x9.y);
}



I think you could use arrays to clean up this code and reduce copy-paste mistakes later, which I think would also make your code readable.
for(int i = 0; i < 9 /*Should be a const int for maintainability*/; i++)
{
if(board[i] == 1) //1 for X (could be changed to an enum {X, O, NOTHING})
{
drawX(xPieces[i].x, xPieces[i].y);
}
else if(board[i] == 2) //2 for O
{
drawO(oPieces[i].x, oPieces[i].y);
}
}

This would be your entire code for drawing the x's and o's on the board instead of having the if chain you have.

If you use arrays like this through out your code you will be able to leverage loop structures to eliminate much of your duplicate code and make the code more readable.

Just a few thoughts.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this