6
\$\begingroup\$

This is a very simple anagram game that plays only with names of colors.

I tried to focus on code design. I also wanted it to support further updates. But, my init seems smelly and you will notice it, especially in the main loop.

I was wondering about my naming and commenting/docstrings. I need strong criticism here, without any mercy.

This is the module I've created with the only class:

class AnagramGame:

    def __init__(self, words=None, rword=None, inp=None,
                 anagram=None, is_corr_word=False):
        self._words = None
        self._rword = rword # Stands for random word
        self._anagram = anagram
        self._inp = inp # User input
        self._is_corr_word = is_corr_word # Stands for correct word bool


    def get_words(self):
        return self._words

    def set_words(self, filename):
        in_file = open(filename, 'r')
        self._words = [in_line.rstrip('\n')
                       for in_line in in_file.readlines()]
        in_file.close()

    def del_words(self):
        del self._words

    words = property(get_words, set_words, del_words)


    def get_rword(self):
        return self._rword

    def set_rword(self, sequence):
        import random
        self._rword = random.choice(sequence)

    def del_rword(self):
        del self._rword

    rword = property(get_rword, set_rword, del_rword)


    def get_anagram(self):
        return self._anagram

    def set_anagram(self, sequence):
        import random
        self._anagram = ''.join(random.sample(sequence, len(sequence)))

    def del_anagram(self):
        del self._anagram

    anagram = property(get_anagram, set_anagram, del_anagram)


    def get_is_corr_word(self):
        return self._is_corr_word

    def set_is_corr_word(self, boolean):
        self._is_corr_word = boolean

    def del_is_corr_word(self):
        del self._is_corr_word

    is_corr_word = property(get_is_corr_word, set_is_corr_word,
                            del_is_corr_word)


    def ask_input(self, prompt=''):
        self._inp = input(prompt)

    def check_input(self, prompt_corr, prompt_incorr):
        if self._inp == self.rword:
            print(prompt_corr, end='')
            self._is_corr_word = True
            return self._is_corr_word
        else:
            print(prompt_incorr, end='')
            self._is_corr_word = False
            return self._is_corr_word

And this is the main program:

def main():
    import games

    print("Anagram Mini Game", "\n")

    while True:
        Angrm = games.AnagramGame() # Reset all attributes
        Angrm.words = 'colors.txt' # Load words
        Angrm.rword = Angrm.words # Extract a word
        Angrm.anagram = Angrm.rword # Create anagram of the word

        print("Try build an anagram of this shuffled color name: ", Angrm.anagram)

        Angrm.ask_input("Give it a try - ")
        Angrm.check_input("GG beast", "Wrong - ") # Not indent after impact prompts
        while not Angrm.is_corr_word:
            Angrm.ask_input()
            Angrm.check_input("GG beast", "Wrong - ")

        print("\n\n") # Indent for a new game


if __name__ == '__main__':
    main()
\$\endgroup\$
  • 4
    \$\begingroup\$ Instead of shortened variable names ("rword"), you would be wise to use full names to improve clarity and do away with unneeded comments. So, use variable names such as "randomWord", "userInput", "is_correct_word". \$\endgroup\$ – Juha Untinen Aug 29 '13 at 7:31
  • 4
    \$\begingroup\$ And in the case of Python, underscores seem to be the recommendation instead of camelCase: tis_but_a_scratch instead of blackKnight \$\endgroup\$ – Juha Untinen Aug 30 '13 at 7:22
4
\$\begingroup\$

In your __init__() function, you have defined words as a parameter but you are assigning None to _words

class AnagramGame:

def __init__(self, words=None, rword=None, inp=None,
             anagram=None, is_corr_word=False):
    self._words = None
    # ...

You may either remove this parameter or assign the value to _words


Instead of using an infinite loop while True:, you should have created a bool object. Something like this:

checker = False
while not checker:
    # ...

I see the purpose of while True: is to make the user play the game endlessly. It would have been great to have an exit criteria too. Else, you could have skipped the infinite loop.


You don't have to re-initialize the object and reload the file on each loop run. You may initialize them before the loop, load the files and let the game run on its own.

To reset the values, you may create another function that would only reset the relevant data-members and continue the game further.


As you have already mentioned, please have proper comments and doc-strings in the code. It is always better to add them during development. Else mostly they get ignored!

\$\endgroup\$
  • \$\begingroup\$ I feel its odd to have one single bool for the loop/loops outside of the module. Is it odd? Cant really get it how to call variable attributes. I also thought to add some docstrings but what to write on these simple methods, same with commenting. Should i use property() or the decorator? \$\endgroup\$ – dragons Aug 30 '13 at 8:48
  • \$\begingroup\$ I don't think anything will sit outside the module. The bool variable be declared in main() and will help you to control the loop exit properly. Docstrings/comments are for general practice. Use properties in this case. You may use decorators where you want something to be accomplished over multiple functions like @static_method or something similar. \$\endgroup\$ – Vivek Jain Aug 30 '13 at 11:32
3
\$\begingroup\$

Your long anagram class might be useful for learning about classes, getters and setters, but it isn't actually doing very much. You shouldn't usually be having to access an object's properties that much from outside the object; it should be dealing with the complicated stuff internally. There are a few different options. One would be for Anagram only to deal with making up anagrams and checking them, so that your main game loop would look something like this:

 while True:
      game = games.Anagram(file='colors.txt');
      for anagram in game.anagrams:
          print('Try build an anagram of this shuffled color name: ' + anagram.shuffled)
          attempt = input('Give it a try -')
          if attempt == anagram.original:
              print('GG beast', end='')
          else:
              print('Wrong - ', end='')

and another would be for all of the game logic to be included in Anagram, so that, for instance, only the following would be left in the main game loop:

game = games.Anagram(file='colors.txt',
    intro='Try build an anagram of this shuffled color name:',
    prompt='Give it a try -',
    right='GG beast',
    wrong='Wrong - ')
for turn in game.turns:
    turn.play()        

What you currently have is messy because it is a mixture of the two. Either way, there's not much call for all that getting and setting.

Also, import statements should usually be at the top of the file.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.