Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

##Style

Style

##Documentation

Documentation

##DRY

DRY

##"Magic numbers"

"Magic numbers"

##Naming

Naming

##Functionality

Functionality

##Style

##Documentation

##DRY

##"Magic numbers"

##Naming

##Functionality

Style

Documentation

DRY

"Magic numbers"

Naming

Functionality

Source Link
jonrsharpe
  • 14k
  • 2
  • 36
  • 62

Short answer: not terribly well! It's a good start (the class is a sensible idea and if __name__ == '__main__': is a good way to run the code), but you have made a couple of mistakes. Here are a few pointers.


##Style

Python has a style guide, which you should read and follow. A few highlights:

  • Naming: classes should be CamelCase and function/method/attribute names should be lowercase_with_underscores.
  • Whitespace: you should put spaces around operators, e.g. self.board = [...] and after commas (e.g. self.board = [[0, 0, 0], ...]).
  • Semicolons: just don't!

##Documentation

It's good to have a README, although it's not clear what format it's in, but much better is to include docstrings for modules, classes, methods and functions, so that the script has documentation built right into it. This can also be used to generate formatted documentation (see e.g. Sphinx) and by your IDE to provide tooltips and even e.g. type hinting.


##DRY

Or "don't repeat yourself". For example, note that setX and setO are almost exactly the same; you could refactor this to a single method which is called by the other methods as appropriate.

You could also significantly simplify main by using loops to reduce repetition, e.g.:

def main():
 print("Hello")
 g = xo()
 g.showBoard()
 now, nxt = g.setX, g.setO
 for x, y in [(2, 2), (1, 1), ...]:
 print(now(x, y))
 g.showBoard()
 print(g.won)
 now, nxt = nxt, now

##"Magic numbers"

Like 1 and 2 (for X and O) and -1 and -2 (not clear) are best avoided - they don't provide any useful information to the reader and make refactoring very difficult. Instead, use named "constants", e.g.:

PLAYER_O = 1
PLAYER_X = 2

Then the code becomes clearer, e.g. self.win(PLAYER_X) tells you what's actually happening.


##Naming

As well as the styling of the names, it's worth thinking about their meaning, too. It's best to avoid contractions (e.g. self.symbols is clearer than self.sym, and I've no idea what e.g. modResX really means). Better names means more readable code, which is a key aim of Python.


##Functionality

  • It's conventional for a method to either change state and return None, or to return something but not change state - win returns something and changes state (although the return value is ignored anyway).
  • Rather than showBoard, I would be inclined to implement a __str__ method that returns the "friendly" representation of the board; then you can use print(g) rather than g.showBoard(). If you do retain the current functionality, note that again there's no need for a return value that's never used.
  • 0 is the default first argument to range, so you can write simply e.g. for i in range(3):, but note that it's neater to iterate over the board itself (i.e. for row in self.board:, etc.).
lang-py

AltStyle によって変換されたページ (->オリジナル) /