Jump to content
  • Advertisement
Sign in to follow this  
Fixxer

Any advice for me on my programming style?

This topic is 4868 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Since im pretty new, Id like to know how my style is, what I might be doing wrong or right. I just wrote this on my own. Not the header, just the main source.
// File: convert.h 
 #include <iostream> 
 #include <sstream> 
 #include <string> 
 #include <stdexcept> 
  
 class BadConversion : public std::runtime_error { 
 public: 
   BadConversion(const std::string& s) 
     : std::runtime_error(s) 
     { } 
 }; 
  
 inline std::string stringify(double x) 
 { 
   std::ostringstream o; 
   if (!(o << x)) 
     throw BadConversion("stringify(double)"); 
   return o.str(); 
 } 
 
/* 
TIC TAC TOE 
MADE BY NATHAN KRYGIER 
-------------------------- 
*/ 

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

using namespace std; 


//========================================================= 

class PlayingArea 
{ 

public: 

   PlayingArea(); 
   ~PlayingArea(); 
   int ClearBoard(); //this will clear the board and start a new game 
   void SetSquare(int SquareTaken, string Symbol); //this will place an X or and O in the appropriate space 
   bool CheckSquare(string Square); //checks to make sure square has not been taken 
   bool CheckForWinner(string& String); // checks for a winner 

private: 

   string BoardVector[4][4]; //this is the tic tac toe board 

}; 

//======================================================== 

class Player 
{ 

public: 
   Player(); 
   Player(string Symbol); 
   ~Player(); 
   int TakeSquare(int Square); //this will return which square the user wishes to take. 
   string Symbol(); 
   int Wins; 
   int Losses; 
   int Ties; 
   int SquareTaken; 
private: 
   string PlayerSymbol; 

}; 


//========================================================== 

int PlayingArea::ClearBoard() 
{ 
   double z= 1; 

   for (int x=0; x < 3; x++){ 

      int y=0; 
       
      BoardVector[x][y] = stringify(z); 
      z++; 
      y++; 
      BoardVector[x][y] = stringify(z); 
      z++; 
      y++; 
      BoardVector[x][y] = stringify(z); 
      z++; 
      y++; 
   } 

   z=1; 

   for (int r=0; r < 3; r++){ 

      int y=0; 
       
      cout<<"|_"<<BoardVector[r][y]<<"_|_"; 
      z++; 
      y++; 
      cout<<BoardVector[r][y]<<"_|_"; 
      z++; 
      y++; 
      cout<<BoardVector[r][y]<<"_|\n"; 
      z++; 
      y++; 
   } 

   return(0); 

} 

//=========================================================== 

PlayingArea::PlayingArea() 
{ 

} 

//=========================================================== 

PlayingArea::~PlayingArea() 
{ 

} 

//=========================================================== 

Player::Player(string Symbol) 
{ 

   PlayerSymbol = Symbol; 

} 

//=========================================================== 

Player::~Player() 
{ 

} 

//=========================================================== 

string Player::Symbol() 
{ 

   return (PlayerSymbol); 

} 

//=========================================================== 

void PlayingArea::SetSquare(int SquareTaken,string Symbol) 
{ 

   switch (SquareTaken) 
   { 

   case 1: BoardVector [0][0] = Symbol; 
      break; 
   case 2: BoardVector [0][1] = Symbol; 
      break; 
   case 3: BoardVector [0][2] = Symbol; 
      break; 
   case 4: BoardVector [1][0] = Symbol; 
      break; 
   case 5: BoardVector [1][1] = Symbol; 
      break; 
   case 6: BoardVector [1][2] = Symbol; 
      break; 
   case 7: BoardVector [2][0] = Symbol; 
      break; 
   case 8: BoardVector [2][1] = Symbol; 
      break; 
   case 9: BoardVector [2][2] = Symbol; 

   } 

      int z=1; 

   for (int r=0; r < 3; r++){ 

      int y=0; 
       
      cout<<"|_"<<BoardVector[r][y]<<"_|_"; 
      z++; 
      y++; 
      cout<<BoardVector[r][y]<<"_|_"; 
      z++; 
      y++; 
      cout<<BoardVector[r][y]<<"_|\n"; 
      z++; 
      y++; 
   } 

} 

//=========================================================== 

bool PlayingArea::CheckSquare(string Square) 
{ 
   int z=1; 
   bool w = false; 

   for (int r=0; r < 3; r++){ 

      int y=0; 
       
      if (Square == BoardVector[r][y]) 
      { 
         w = true; 
      } 
      z++; 
      y++; 
      if (Square == BoardVector[r][y]) 
      { 
         w = true; 
      } 
      z++; 
      y++; 
      if (Square == BoardVector[r][y]) 
      { 
         w = true; 
      } 
      z++; 
      y++; 
   } 

   return (w); 

} 

//=========================================================== 

bool PlayingArea::CheckForWinner(string& GameWinner) 
{ 


   bool InternalLoop; 
   int z= 1; 

   for (int x=0; x < 3; x++){ 

      int y=0; 
       
       
      if (BoardVector[x][y] == stringify(z)){ 
         InternalLoop = true; 
         GameWinner = "No one Wins! It is a tie!"; 
      } 
      z++; 
      y++; 
      if (BoardVector[x][y] == stringify(z)){ 
         InternalLoop = true; 
         GameWinner = "No one Wins! It is a tie!"; 
      } 
      z++; 
      y++; 
      if (BoardVector[x][y] == stringify(z)){ 
         InternalLoop = true; 
         GameWinner = "No one Wins! It is a tie!"; 
      } 
      z++; 
      y++; 
   } 

   if ((BoardVector[0][0] == BoardVector[0][1])&&(BoardVector[0][1] == BoardVector [0][2])){ 
       
      InternalLoop=false; 
    
      if (BoardVector[0][0] == "X"){ 
         GameWinner = "Player One Wins!"; 
      } 
      else if (BoardVector[0][0] == "O"){ 
         GameWinner = "Player Two Wins!"; 
      } 
   } 
   else if ((BoardVector[0][0] == BoardVector[1][0])&&(BoardVector[1][0] == BoardVector [2][0])){ 
       
      InternalLoop=false;    
    
      if (BoardVector[0][0] == "X"){ 
         GameWinner = "Player One Wins!"; 
      } 
      else if (BoardVector[0][0] == "O"){ 
         GameWinner = "Player Two Wins!"; 
      } 
   } 
   else if ((BoardVector[0][0] == BoardVector[1][1])&&(BoardVector[1][1] == BoardVector [2][2])){ 
       
      InternalLoop=false;    

      if (BoardVector[0][0] == "X"){ 
         GameWinner = "Player One Wins!"; 
      } 
      else if (BoardVector[0][0] == "O"){ 
         GameWinner = "Player Two Wins!"; 
      } 
   } 
   else if ((BoardVector[0][1] == BoardVector[1][1])&&(BoardVector[1][1] == BoardVector [2][1])){ 
       
      InternalLoop=false; 
       
      if (BoardVector[0][1] == "X"){ 
         GameWinner = "Player One Wins!"; 
      } 
      else if (BoardVector[0][1] == "O"){ 
         GameWinner = "Player Two Wins!"; 
      } 
   } 
   else if ((BoardVector[0][2] == BoardVector[1][2])&&(BoardVector[1][2] == BoardVector [2][2])){ 
       
      InternalLoop=false; 
       
      if (BoardVector[0][2] == "X"){ 
         GameWinner = "Player One Wins!"; 
      } 
      else if (BoardVector[0][2] == "O"){ 
         GameWinner = "Player Two Wins!"; 
      } 
   } 
   else if ((BoardVector[0][2] == BoardVector[1][1])&&(BoardVector[1][1] == BoardVector [2][0])){ 
       
      InternalLoop=false; 
       
      if (BoardVector[0][2] == "X"){ 
         GameWinner = "Player One Wins!"; 
      } 
      else if (BoardVector[0][2] == "O"){ 
         GameWinner = "Player Two Wins!"; 
      } 
   } 
   else if ((BoardVector[1][0] == BoardVector[1][1])&&(BoardVector[1][1] == BoardVector [1][2])){ 
       
      InternalLoop=false;    

      if (BoardVector[1][0] == "X"){ 
         GameWinner = "Player One Wins!"; 
      } 
      else if (BoardVector[1][0] == "O"){ 
         GameWinner = "Player Two Wins!"; 
      } 
   } 
   else if ((BoardVector[2][0] == BoardVector[2][1])&&(BoardVector[2][1] == BoardVector [2][2])){ 
       
      InternalLoop=false; 
       
      if (BoardVector[2][0] == "X"){ 
         GameWinner = "Player One Wins!"; 
      } 
      else if (BoardVector[2][0] == "O"){ 
         GameWinner = "Player Two Wins!"; 
      } 
   } 


   return InternalLoop; 
} 

//=========================================================== 


int main() 
{ 

   PlayingArea Board; 
   Player PlayerA("X"); 
   Player PlayerB("O"); 

   string GameWinner; 
   bool SYNC = true; 
   bool InternalLoop = Board.CheckForWinner(GameWinner); 
   bool Valid = true; 
   int Square; 


   do{    

      Board.ClearBoard(); 
      do{ 
         InternalLoop = Board.CheckForWinner(GameWinner); 
         if (InternalLoop == true){ 

            do{ 

               cout<<"\nPlayer One, which sqaure would you like to take?\nSqaure:"; 
               cin>>Square; 
               system("cls"); 
               Valid = Board.CheckSquare(stringify(Square)); 

            } while (Valid == false); 

         Board.SetSquare(Square,PlayerA.Symbol()); 
      } 

         InternalLoop = Board.CheckForWinner(GameWinner); 

         if (InternalLoop == true){ 
         do{ 

            cout<<"\nPlayer Two, which sqaure would you like to take?\nSqaure:"; 
            cin>>Square; 
            system("cls"); 
            Valid = Board.CheckSquare(stringify(Square)); 

         } while (Valid == false); 

         Board.SetSquare(Square,PlayerB.Symbol()); 
         } 

      } while (InternalLoop == true); 

      cout<<GameWinner<<"\n"; 

      cin.get(); 
      cin.get(); 
       
      SYNC=false; 
   }while(SYNC == true); 

return(0); 
}

Share this post


Link to post
Share on other sites
Advertisement
Yes, it's you style so don't ask people to judge it! You develop a style, not learn it. To each his own.

Share this post


Link to post
Share on other sites
Also if you start working on teams, some of them might require you to adapt their style, so do not get too attached to yours.

Share this post


Link to post
Share on other sites
Quote:
Original post by STLDude
Also if you start working on teams, some of them might require you to adapt their style, so do not get too attached to yours.


I agree... Personally I try to keep my style flexible, and often revise it every few months. This is somewhat crazy to keep changing my style, but that way I get used to different styles and also learn what works and what doesn't. (just my 2 cents)

As for your style, it looks pretty "standard" to me, which is good. Some coders have really weird styles which take some getting used to.

I do have a couple of comments though, though this is based on my personal preferences:

1. Enforce const correctness
2. Use ++x instead of x++ (more efficient, although optimized in release)
3. Personally I don't like "hanging" braces:


// Like this:

class Whatever
{
};

// But not this:

class Whatever {
};


4. Maybe I'm missing something, but why is SYNC in caps, and all other variables are LikeThis?

5. I'd use long instead of int... Not sure, but I think sizeof(int) is not guaranteed on all platforms.

6. No commenting. Maybe it's okay for simple code like this, but if all your code lacks comments, that's probably a bad thing :)

Share this post


Link to post
Share on other sites
In terms of design, have a read of The Bowling Game. You have quite a bit of repeated logic which could be extracted, condensed and cleaned up and, ultimately, made a lot clearer.

I use CapitalLetters for functions/classes and lower_case or camelCase for variables.

Share this post


Link to post
Share on other sites
Why is the board stored in an array of size [4][4] rather than [3][3]?

Why are you stringifying doubles for display rather than ints? Why stringify at all? Just store an array of single characters instead; you can use the characters '1' through '9' and 'X' and 'O' just fine.

Why are your inner loops "unrolled" when you iterate over the grid? (There is obviously a lot of copy-pasting going on here too, as evidenced by some loops containing "z++"s where z never actually gets used in the function. Copy and paste is a bad thing.)

When checking the board winner, you set up the draw condition if *any* square is empty, and then override that later. Presumably this works ok since when a draw happens, the draw condition will be set up from a previous check. But it's completely not the logic that's intended here; a draw should occur if *no* square is empty. Anyway, setting a global string and also returning a boolean value is an awful way to handle this. Return a single value indicating the status (four possibilities: X won, O won, draw, or game still on), and have the calling code interpret the return value appropriately.

And, as mentioned, *lots* of repeated logic. (Again, Copy and paste is a bad thing. Copy and paste is a bad thing. Copy and paste is a bad thing. See? Note the "bug" in my English which results from mindlessly copying and pasting ;) ) Also, excess complication in the logic. I won't recommend any gotos here (heh), but internal loop exits (i.e. "break" or "return") are ok - don't jump through hoops to avoid them.

Share this post


Link to post
Share on other sites
My advice is dont capitalize every variable like you are

some examples - SYNC / InternalLoop

all capitals indicates a CONST value

naming variables with uppercase is bad style because class names are usually uppercase.

use lowercase for first word, and each subsequent word uppercase, or an _ in between each word works.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!