Skip to main content
added 143 characters in body
Source Link
Samwise
  • 4k
  • 8
  • 14
  1. I think it makes the play logic clearer to group the two "valid" conditions into a single if predicate. Another way to approach this would be to implement both rule_one and rule_two to return True when the list is empty so that you don't need to special-case it in the caller.

    I think it makes the play logic clearer to group the two "valid" conditions into a single if predicate. Another way to approach this would be to implement both rule_one and rule_two to return True when the list is empty so that you don't need to special-case it in the caller.

  2. Implicit "truthy" checks can lead to subtle bugs, so I usually prefer explicit boolean conditions, e.g. is not None or len(...) > 0.

  1. I think it makes the play logic clearer to group the two "valid" conditions into a single if predicate. Another way to approach this would be to implement both rule_one and rule_two to return True when the list is empty so that you don't need to special-case it in the caller.
  1. I think it makes the play logic clearer to group the two "valid" conditions into a single if predicate. Another way to approach this would be to implement both rule_one and rule_two to return True when the list is empty so that you don't need to special-case it in the caller.

  2. Implicit "truthy" checks can lead to subtle bugs, so I usually prefer explicit boolean conditions, e.g. is not None or len(...) > 0.

Source Link
Samwise
  • 4k
  • 8
  • 14

I'm going to make all the easy improvements I can spot and call them out as I go; some of what I'm about to say will probably duplicate excellent points already raised by @AlexV.

  1. Running the game when you instantiate the object is an unusual interface. I'd rename that main method something like run and have the __main__ function call that after creating the game object.

  2. Docstrings are conventionally marked with tripled double-quotes, not single-quotes. They also typically go inside the function they document rather than before it, and they should document the external behavior, not the internal implementation (use comments to explain internals where necessary).

  3. Avoid unnecessary state; your currentWord is only ever used in the context of a single turn, so there's no need to track it in your class.

  4. Variable and method name should be snake_case, not camelCase.

  5. Adding type annotations helps validate code correctness.

  6. Instead of:

if condition:
   return True
return False

do:

return condition

(assuming condition is already a bool value; if you use type annotations, mypy will enforce this for you)

  1. I think it makes the play logic clearer to group the two "valid" conditions into a single if predicate. Another way to approach this would be to implement both rule_one and rule_two to return True when the list is empty so that you don't need to special-case it in the caller.

Here's the code I wound up with:

#Exercise source = https://edabit.com/challenge/dLnZLi8FjaK6qKcvv
from typing import List

class Shiritori:
    """
    Class Shiritori
        Controls the action and state of the game
    """
    def __init__(self):
        self.played_words: List[str] = []    # words used so far

    def rule_one(self, word: str) -> bool:
        """Determines if the word's first letter matches the last word's letter."""
        return word[0] == self.played_words[-1][-1]

    def rule_two(self, word: str) -> bool:
        """Determines if the word has not already been played"""
        return word not in self.played_words

    def add_word(self, word: str) -> None:
        """Adds the current word to the list of played words"""
        self.played_words.append(word)

    def play(self, word: str) -> None:
        """
        Play a word, validating it against the rules.  
        If any rules are broken, end the game and restart.
        """
        word = word.lower().strip()

        # If no words have been played yet, this play is automatically valid.
        # Otherwise, it must satisfy rules one and two.
        if (len(self.played_words) == 0
                or self.rule_one(word) and self.rule_two(word)):
            self.add_word(word)
        else:
            self.game_over()

    def game_over(self) -> None:
        """prints the game over message and resets the game"""
        print("You have lost. Game is being restarted.")
        self.played_words = []

    def run(self) -> None:
        """
        The main loop of the game.
        Keeps accepting words from the user and playing them in the game.
        Exit the loop by entering an empty string.
        """
        print("Enter a word or enter '' to exit the game")
        while True:
            word = input("Please enter your word>")
            self.play(word)
            if word == '':
                break
        print("Thank you for playing.")

if __name__ == '__main__':
    game = Shiritori()
    game.run()