ryanbhayward / games-puzzles-algorithms

software for CMPUT 355 (initial 396): ugrad course on games, puzzles, algorithms
36 stars 153 forks source link

Generalize TTT resolving #48 #67

Closed dmorrill10 closed 8 years ago

imccarten1 commented 8 years ago

I didn't find any bugs, but there are a few style issues.

  1. I think the method and variables k and self._k could have clearer names.
  2. I don't see why GameState and Board need to be separate classes when many of their methods such as str and row and column are near duplicates of each other.
  3. A different default value of k such as min(num_rows, num_columns) might make more sense. Also it would make sense to add a check that k is at most max(num_rows, num_columns).
dmorrill10 commented 8 years ago

Thanks.

  1. Yeah, I agree, but I couldn't think of a better name at the time. Maybe num_spaces_to_win?
  2. I disagree. They are managing two different types of state. Board manages lower level changes to the board while GameState manages the higher level game dynamics. Also, I count 6 methods that are exact forwards but 8 that do some extra operations. I think that this separation is still conceptually useful and more readable than the alternative.
  3. Right, thank you, I forgot to make that change.
dmorrill10 commented 8 years ago

I will look at using inheritance to make GameState and Board more readable.

dmorrill10 commented 8 years ago

@imccarten1, please review these changes and merge them or suggest additional changes.