Python Shirtori Game Exercise

Question

I'm looking to improve my code writing in general by peer review. I've followed the following exercise: https://edabit.com/challenge/dLnZLi8FjaK6qKcvv.

The exercise is to create an English version Shiritori Game. You can find an explanation by following the link but I'm going to explain below anyway.

The game is a word matching games where the last letter of the last word said must match the first letter of the next word. A word cannot be used again once it has been played in that game. If a word doesn't match these rules then it is game over and the game is restarted.

Please find the code below:

#Exercise source = https://edabit.com/challenge/dLnZLi8FjaK6qKcvv
'''
 Class Shiritori
 Controls the action and state of the game
'''
class Shiritori:
 '''
 Set up an empty array to store the last words used.
 Stores the currently used word for judging
 Initiates the main game loop main()
 '''
 def __init__(self):
 self.playedWords = []
 self.currentWord = None
 self.main()
 
 '''
 Determines if the current word's first letter matches the last word's letter
 '''
 def ruleOne(self):
 lastWord = self.playedWords[-1]
 if lastWord[-1] == self.currentWord[0]:
 return True
 return False
 
 '''
 Determines if the current word has already been played
 '''
 def ruleTwo(self):
 if self.currentWord in self.playedWords:
 return False
 return True
 
 '''
 Adds the current word to the list of played words
 '''
 def addWord(self):
 self.playedWords.append(self.currentWord)
 
 '''
 Play a turn
 1. enters a word which is turned into lowercase and removes any additional spaces
 2. checks if it is the first word to be played, add the word if no words have been played
 3. checks the word against rule 1 and rule 2, adds the word if both rules are true
 4. if 2 or 3 have not been done then activate the game over message and restart the game
 '''
 def play(self, word):
 word = word.lower()
 word = word.strip()
 self.currentWord = word
 if not self.playedWords:
 self.addWord()
 elif(self.ruleOne() and self.ruleTwo()):
 self.addWord()
 else:
 self.gameOver()
 
 '''
 prints the game over message and restarts the game
 '''
 def gameOver(self):
 print("You have lost. Game is being restarted.")
 self.playedWords = []
 self.currentWord = None
 
 '''
 The main loop of the game. Keeps accespting words from users and playing them in the games.
 Exit the loop by entering in an empty string or just pressing enter.
 '''
 def main(self):
 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.")
 
game = Shiritori()

Edit 1

Code Explanation

Once run the code should run the main function of the class which will loop over until the user decided to quit by pressing enter (entering an empty string). Otherwise the program should accept a string of characters as words. Please note there is no parsing of the data except turning the word into lowercase and removing any additional spaces. The entered word will be checked against an array to see if the two rules match up, if the rules don't match then the game goes into the 'game over' function which resets the game automatically.

The game runs within the console.

Unorthodox choices

1. array[-1] and string[-1] last letter and last string retrieval

   I originally used len(string/array)-1 to retrieve the last letter/string. However this seems more visually appealing but it is questionable whether it is readable

2. Main inside or outside of class

   I chose inside however maybe it should be outside of the class as it makes sense to have the main loop there and keep it separate from the object however it is the main controls so I am unsure.

3. play function if,elif,else

   the if and elif statement do the same thing, there should be a more efficient way of doing it without separating the action into the function and having both statements

4. if word == ''

   Not sure if unorthodox but there should be a better comparison

5. word.lower(), word.strip(), self.currentWord = word

   There might be a better way of combining this?

• user211881: Could you add a short explanation for how you code works, and why you made any unorthodox choices if you did?
• Meerfall the dewott: @K00lman I added them in, please let me know if I should change anything else
• user211881: No it all looks good now.

Answer 1

Unorthodox choices

Before having a deeper look at the code, let's look at what the "Unorthodox choices" listed in the question

1. Using array[-1] is perfectly valid Python and will very likely be considered much more readable and Pythonic than using len(...)-1.

2. main function

   Having the main function inside the class and call it directly when you create the object is IMHO much more unorthodox. I personally would discourage it, though it's probably not unheard of. What is usually used in Python is a separate main() function with the game loop as well as the infamous if __name__ == "main":, also known as top-level script environment in technical terms. All code inside the body of this if is only run when you execute the file as a script, instead e.g. importing something into another file.

3. play function, if statement

   It's easy combine both conditions:

if not self.playedWords or (self.ruleOne() and self.ruleTwo()): 
 self.addWord()
else:
 self.gameOver()

It would also be possible to invert the condition:

if self.playedWords and not (self.ruleOne() or self.ruleTwo()): 
 self.gameOver()
else:
 self.addWord()

4. if word == ''

   Python encourages you to be explicit in your code. Just for the sake of the argument, if not word: would have a similar effect.

5. word.lower(), word.strip(), self.currentWord = word

   It's possible to chain those calls: self.currentWord = word.lower().strip(). Quick note here: .strip() only removes trailing and leading whitespace. If the user entered multiple words, the whitespace in between them will not be affected.

Style

I highly recommend having a look at the official Style Guide for Python Code aka PEP 8 (again). Some key takeaways of that read should be:

• prefer to use lowercase_with_underscores for function and variable names.
• UpperCase is "reserved" for class names (no issue in the code here)
• docstrings should be defined inside the function, i.e. after def what_ever_name():, not before. Otherwise Python's help(...) and also most IDEs will not pick it up correctly. There is more about docstrings in PEP 257.

Fortunately, there is a wide variety of tools that can help with keeping a consistent style, even when projects grow larger. This list on Code Review Meta provides you with a good starting point to get going.

Code

As I already said, I would not recommend starting the game immediately in __init__. even if you keep the main game loop inside the class, the user should have to trigger the game explicitly.

The member variable self.currentWord is not strictly needed. By allowing a word as input for ruleOne, ruleTwo, and addWord, it would be easier to see what's going on.

With a little bit of rewriting, a set could be used. A set does not allow duplicate members and has faster membership tests compared to a list (constant time vs. linear time). However, sets don't preserve the order of elements, i.e. the class has to store the last word in a member variable.

Having a last_word instead of current_word/currentWord would also be closer to what one might expect when looking at the game rules.

Answer 2

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)

7. 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.

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

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()