Skip to main content
added 123 characters in body
Source Link
Eric Lippert
  • 14.9k
  • 4
  • 38
  • 57

Other answers have noted that there is a great deal of unnecessary repetition in the button handling logic. There is also a great deal of repetition in the winning-condition-checking logic. Any time you copy-paste code like that, consider whether the code should go in a method which is then just called. There should be a "disable all buttons" method that you call, rather than repeating that logic a dozen times.

The winning condition logic appears to be incorrect. It seems to say that you cannot win on the last move, but only tie.

There is no variable keeping track of whether the game is won by x, won by o, tied, or still in progress. It's not necessary to have such a variable as it is redundant to the other game state, but sometimes it is easier to write the code when you can have one place to ask "is the game still in progress?"

The best advice I can give you though aside from eliminating all that redundancy is to separate your display logic from your game logic. You are using the appearance of some buttons as the storage for the game state, which means that you cannot change the appearance of the game without rewriting all of the logic. (Imagine for instance that you wanted to change the game so that instead of X and O it was blue and red buttons, or you wanted to have graphics on the buttons instead of text.)

The fancy term for this is "model view controller pattern", but you don't need to understand the formalism in order to use it effectively. Make a class that represents the game state and the rules; that class decides whether a move is legal, keeps track of the turns, and decides who won. The form class is responsible for taking in the moves and informing the game class.

Once you have that then many scenarios become easier. You can test the game logic independently of the UI, for example.

Other answers have noted that there is a great deal of unnecessary repetition in the button handling logic. There is also a great deal of repetition in the winning-condition-checking logic. Any time you copy-paste code like that, consider whether the code should go in a method which is then just called. There should be a "disable all buttons" method that you call, rather than repeating that logic a dozen times.

The winning condition logic appears to be incorrect. It seems to say that you cannot win on the last move, but only tie.

The best advice I can give you though aside from eliminating all that redundancy is to separate your display logic from your game logic. You are using the appearance of some buttons as the storage for the game state, which means that you cannot change the appearance of the game without rewriting all of the logic. (Imagine for instance that you wanted to change the game so that instead of X and O it was blue and red buttons, or you wanted to have graphics on the buttons instead of text.)

The fancy term for this is "model view controller pattern", but you don't need to understand the formalism in order to use it effectively. Make a class that represents the game state and the rules; that class decides whether a move is legal, keeps track of the turns, and decides who won. The form class is responsible for taking in the moves and informing the game class.

Other answers have noted that there is a great deal of unnecessary repetition in the button handling logic. There is also a great deal of repetition in the winning-condition-checking logic. Any time you copy-paste code like that, consider whether the code should go in a method which is then just called. There should be a "disable all buttons" method that you call, rather than repeating that logic a dozen times.

The winning condition logic appears to be incorrect. It seems to say that you cannot win on the last move, but only tie.

There is no variable keeping track of whether the game is won by x, won by o, tied, or still in progress. It's not necessary to have such a variable as it is redundant to the other game state, but sometimes it is easier to write the code when you can have one place to ask "is the game still in progress?"

The best advice I can give you though aside from eliminating all that redundancy is to separate your display logic from your game logic. You are using the appearance of some buttons as the storage for the game state, which means that you cannot change the appearance of the game without rewriting all of the logic. (Imagine for instance that you wanted to change the game so that instead of X and O it was blue and red buttons, or you wanted to have graphics on the buttons instead of text.)

The fancy term for this is "model view controller pattern", but you don't need to understand the formalism in order to use it effectively. Make a class that represents the game state and the rules; that class decides whether a move is legal, keeps track of the turns, and decides who won. The form class is responsible for taking in the moves and informing the game class.

Once you have that then many scenarios become easier. You can test the game logic independently of the UI, for example.

Source Link
Eric Lippert
  • 14.9k
  • 4
  • 38
  • 57

Other answers have noted that there is a great deal of unnecessary repetition in the button handling logic. There is also a great deal of repetition in the winning-condition-checking logic. Any time you copy-paste code like that, consider whether the code should go in a method which is then just called. There should be a "disable all buttons" method that you call, rather than repeating that logic a dozen times.

The winning condition logic appears to be incorrect. It seems to say that you cannot win on the last move, but only tie.

The best advice I can give you though aside from eliminating all that redundancy is to separate your display logic from your game logic. You are using the appearance of some buttons as the storage for the game state, which means that you cannot change the appearance of the game without rewriting all of the logic. (Imagine for instance that you wanted to change the game so that instead of X and O it was blue and red buttons, or you wanted to have graphics on the buttons instead of text.)

The fancy term for this is "model view controller pattern", but you don't need to understand the formalism in order to use it effectively. Make a class that represents the game state and the rules; that class decides whether a move is legal, keeps track of the turns, and decides who won. The form class is responsible for taking in the moves and informing the game class.