1
\$\begingroup\$

I was trying to solve the final puzzle at the end of the classes chapter (Chapter 8) in "Python Programming for the Absolute Beginner" which is stated as:

Create a Critter Farm program by instantiating several Critter objects and keeping track of them through a list. Mimic the Critter Caretaker program, but instead of requiring the user to look after a single critter, require them to care for an entire farm. Each menu choice should allow the user to perform some action for all the critters (feed all the critters, play with all of the critters, or listen to all of the critters). To make the program interesting, give each critter random starting hunger and boredom levels.

Here is my solution:

from random import randrange

class Critter(object):
    def __init__(self, name):
        self.__name = name
        self.__hunger = randrange(7)
        self.__boredom = randrange(7)

    def __str__(self):
        return "Hi, I'm " + self.__name + " and I feel " + self.mood + "."

    def talk(self):
        print(self)
        self.__passTime()

    def eat(self):
        self.__hunger -= 3
        if self.__hunger < 0:
            self.__hunger = 0
        self.__passTime()

    def play(self):
        self.__boredom -= 3
        if self.__boredom < 0:
            self.__boredom = 0
        self.__passTime()

    def __passTime(self):
        self.__hunger += 1
        self.__boredom += 1

    @property
    def mood(self):
        happiness = self.__hunger + self.__boredom
        if happiness < 5:
            return "happy"
        elif happiness < 10:
            return "ok"
        else:
            return "sad"

class CritterFarm(object):
    def __init__(self):
        self.__critters = []

    def talk(self):
        for critter in self.__critters:
            critter.talk()

    def add(self, name):
        self.__critters.append(Critter(name))

    def eat(self):
        for critter in self.__critters:
            critter.eat()

    def play(self):
        for critter in self.__critters:
            critter.play()

def main():
    farm = CritterFarm()
    option = ""
    while option != "0":
        choice = input("""
        0 - Quit
        1 - Add a Critter to your Farm
        2 - Listen to your Critters
        3 - Feed your Critters
        4 - Play with your Critters

        Choice: """)

        if choice == "0":
            print("Exiting...")
        elif choice == "1":
            name = input("Enter your new Critter's name: ")
            farm.add(name)
        elif choice == "2":
            farm.talk()
        elif choice == "3":
            farm.eat()
        elif choice == "4":
            farm.play()

if __name__ == "__main__":
    main()

Can anyone offer a code review? Thanks.

\$\endgroup\$
4
\$\begingroup\$

Why are you using __double_underscore_prefixes for your member variable names?

Double-underscores get name-mangled by Python for the purpose of 'class-local references'. They're for when your class's implementation relies on something that you don't want subclasses to customise. This is a very rare situation, and typically happens quite late in a library's lifetime.

What double-underscore names are not for is access control. This is one of the biggest pieces of mis-information given to Python beginners. Python doesn't have a concept of 'public' and 'private', and you shouldn't try to hack one together out of a language feature designed for something else. All your member variables should just be public by default; if you need to change an implementation detail, you can maintain your contract using properties.

If you really insist on declaring something as private, the conventional way to do that in Python is using a _single_leading_underscore. It's still not actually private, it's just a clear signal to your consumers that they shouldn't mess around with that stuff because it might change.


I also don't really like the way you're using __str__. __str__ is designed to return a meaningful description of the object, generally for the purposes of debugging (or when you wish to emulate string-like behaviour). You're using __str__ as part of the behaviour of your class (returning a formatted string in plain English).

I'd put the body of __str__ inside the talk method, and either delete __str__ altogether, or use it for something more meaningful to a person working with the code - something like this:

def __str__(self):
    return "Critter named {} with hunger {} and boredom {}".format(self.__name, self.__hunger, self.__boredom)

def talk(self):
    print("Hi, I'm " + self.__name + " and I feel " + self.mood + ".")
    self.__passTime()
\$\endgroup\$
2
\$\begingroup\$

Not much to say:

Variable naming

I find that happiness is not very well chosen; sadness would probably be a better name.

Conciseness

self.__hunger = max(self.__hunger - 3, 0) is a bit shorter. Same applies to boredom.

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