8
\$\begingroup\$

I'm currently learning programming and Python from Python Programming for the Absolute Beginner, Third Edition. I've been doing the challenges set out at the end of each chapter but I'm never sure if I'm doing what the author wanted or if I'm doing them the best way or worst way.

The task is to:

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 care for a single critter, require them to care for an entire farm. Each menu choice should allow the user to perform some action for all of the critters (feed all of 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.

I've been learning for about 3 weeks now, and this was part of the first OOP chapter. Some feedback and any way I could better my code would be appreciated.

import random

class Critter(object):
    """a virtual pet"""
    def __init__(self, name, hunger = '', fun = ''):
        print("A new critter called", name, "has been created!")
        self.name = name
        self.hunger = random.randrange(0, 20)
        self.fun = random.randrange(0, 20)

    def __str__(self):
        """to see there hunger and fun levels"""
        return self.name, self.hunger, self.fun

    def health(self):
        """check the health of a critter"""
        health = self.fun + self.hunger
        if health < 5:
            print(self.name, "is very happy and healthy!")
        if 5 <= health <= 15:
            print(self.name, "is doing good!")
        if 16 <= health <= 25:
            print(self.name, "is okay right now!")
        if 26 <= health <= 35:
            print(self.name, "needs some attention, maybe try feeding "\
                  "or playing with them.")

    def talking_one(self):
        """what the critter will say when spoken too, random choice of 6"""
        self.__pass_time()
        what_to_say = random.randrange(0, 5)
        if what_to_say == 0:
            print("I like the colour red.")
        elif what_to_say == 1:
            print("I am very sleepy today.")
        elif what_to_say == 2:
            print("Today is today, i think.")
        elif what_to_say == 3:
            print("I eat my shoe.")
        elif what_to_say == 4:
            print("Give me your money.")
        elif what_to_say == 5:
            print("I can't think of anything else to say, leave me alone.")

    def talking_all(self):
        """when you talk to all the critters"""
        self.__pass_time()

    def feeding_one(self):
        """feeding only one"""
        self.__pass_time()
        self.hunger -= 4
        if self.hunger < 0:
            self.hunger = 0
        print(self.name, "says: 'Yummy, that was a very nice snack, thanks!'")

    def feedingall(self):
        """feeding all the critters at the same time"""
        self.__pass_time()
        self.hunger -= 4
        if self.hunger < 0:
            self.hunger = 0

    def play_one(self):
        """play with one of the critters"""
        self.__pass_time()
        self.fun -= 4
        if self.fun < 0:
            self.fun = 0
        if self.fun > 20:
            self.fun == 20
        print(self.name, "says: 'That was great fun, thanks for playing with me.'")

    def playall(self):
        """play with all the critters"""
        self.__pass_time()
        self.fun -= 4
        if self.fun < 0:
            self.fun = 0
        if self.fun > 20:
            self.fun == 20

    def __pass_time(self):
        """simulate the passing of time, to make them more hungry and bored"""
        self.hunger += 1
        self.fun +=1
        if self.hunger > 20:
            self.hunger == 20
        if self.fun > 20:
            self.hunger == 20

#creating the critters, and storing them in the critlist list
dave = Critter('Dave')
sarah = Critter('Sarah')
clair = Critter('Clair')
ashie = Critter('Ashie')
daen = Critter('Daen')
critlist = []
critlist.append(dave.__str__())
critlist.append(sarah.__str__())
critlist.append(clair.__str__())
critlist.append(ashie.__str__())
critlist.append(daen.__str__())

def main():
    print("""
\t\tWelcome to your critter farm!
\t\tSelect one of the options below to interact with them!

0 - Exit
1 - Play with a critter
2 - Play with all the critters
3 - Feed a critter
4 - Feed all the critters
5 - Talk with a critter
6 - Talk with all the critters
7 - View the health of a critter
""")

    answer = ''
    while answer !='0':
        answer = input("\nSelect your option: ")
# exit the programm
        if answer == '0':
            print("Logging out. Good-bye!")
# play with a critter
        elif answer == '1':
            who2 = input("Enter the name of the critter you want to play with "\
                        "Dave, Sarah, Clair, Ashie or Daen?: ")          
            who = who2.capitalize()
            if who == 'Dave':
                dave.play_one()
            elif who == 'Sarah':
                sarah.play_one()
            elif who == 'Clair':
                clair.play_one()
            elif who == 'Ashie':
                ashie.play_one()
            elif who == 'Daen':
                daen.play_one()
# play with all critter
        if answer == '2':
            dave.playall()
            sarah.playall()
            clair.playall()
            ashie.playall()
            daen.playall()
            print("All the critters say: 'Hurrah for our wonderful owner playing"\
                  "such a fun group game with us all!!'")
# feeding a critter
        if answer == '3':
            who2 = input("Enter the name of the critter you want to play with"\
                        "Dave, Sarah, Clair, Ashie or Daen?: ")          
            who = who2.capitalize()
            if who == 'Dave':
                dave.play_one()
            elif who == 'Sarah':
                sarah.play_one()
            elif who == 'Clair':
                clair.play_one()
            elif who == 'Ashie':
                ashie.play_one()
            elif who == 'Daen':
                daen.play_one()
# feeding all
        if answer == '4':
            dave.feedingall()
            sarah.feedingall()
            clair.feedingall()
            ashie.feedingall()
            daen.feedingall()
            print("Wow thanks we are all so grateful to you.")

# talking with a critter
        if answer == '5':
            who2 = input("Enter the name of the critter you want to talk with "\
                        "Dave, Sarah, Clair, Ashie or Daen?: ")          
            who = who2.capitalize()
            if who == 'Dave':
                dave.talking_one()
            elif who == 'Sarah':
                sarah.talking_one()
            elif who == 'Clair':
                clair.talking_one()
            elif who == 'Ashie':
                ashie.talking_one()
            elif who == 'Daen':
                daen.talking_one()
# talking with all
        if answer == '6':
            dave.talking_all()
            sarah.talking_all()
            clair.talking_all()
            ashie.talking_all()
            daen.talking_all()
            what_they_say = random.randrange(0, 5)
            if what_they_say == 0:
                print("We are an angry mob.")
            if what_they_say == 1:
                print("Three cheers for our owner!!!")
            if what_they_say == 2:
                print("We are a happy mob.")
            if what_they_say == 3:
                print("We all agree with you.")
            if what_they_say == 4:
                print("Yes master, we will do as you command.")
            if what_they_say == 5:
                print("Begin operation take over the world.")
# view there health
        if answer == '7':
            who2 = input("Enter the name of the critter you want to check on "\
                        "Dave, Sarah, Clair, Ashie or Daen?: ")          
            who = who2.capitalize()
            if who == 'Dave':
                dave.health()
            elif who == 'Sarah':
                sarah.health()
            elif who == 'Clair':
                clair.health()
            elif who == 'Ashie':
                ashie.health()
            elif who == 'Daen':
                daen.health()
# hidden from user, will show the list that there all contained in to
# view there hunger + fun levels
        if answer == 'moreinfo':
            updatelist = []
            updatelist.append(dave.__str__())
            updatelist.append(sarah.__str__())
            updatelist.append(clair.__str__())
            updatelist.append(ashie.__str__())
            updatelist.append(daen.__str__())
            print(updatelist)

main()
\$\endgroup\$
4
\$\begingroup\$

That's very neat and tidy code, and it is obvious what it is doing. A pleasure to read. And, since the critters are so cute I was smiling all the way through! Good work.

I think you've kind of missed the main point of the exercise though, which is "Create a Critter Farm program by instantiating several Critter objects and keeping track of them through a list." Let's walk through it. Instead of this...

#creating the critters, and storing them in the critlist list
dave = Critter('Dave')
sarah = Critter('Sarah')
clair = Critter('Clair')
ashie = Critter('Ashie')
daen = Critter('Daen')
critlist = []
critlist.append(dave.__str__())
critlist.append(sarah.__str__())
critlist.append(clair.__str__())
critlist.append(ashie.__str__())
critlist.append(daen.__str__())

... you should be doing something like this ...

critlist = [Critter('Dave)',
            Critter('Sarah'),
            Critter('Clair'),
            Critter('Ashie'),
            Critter('Daen')]

... and the reason you should be doing that is it makes a lot of other things much easier. For example, this ...

# play with all critter
if answer == '2':
    dave.playall()
    sarah.playall()
    clair.playall()
    ashie.playall()
    daen.playall()
    print("All the critters say: 'Hurrah for our wonderful owner playing"\
          "such a fun group game with us all!!'")

... becomes something like this ...

# play with all critter
if answer == '2':
    for critter in critlist:
        critter.playall()
    print("All the critters say: 'Hurrah for our wonderful owner playing"\
      "such a fun group game with us all!!'")

Which means you don't have to change the code if you add another critter, or rename one, or delete one. Also it is shorter. And the same goes for playing with them, and talking to them.

(It's a little bit more tricky to do the same sort of thing when selecting the name of a Critter you want to interact with, but since you seem to be doing so well I think I'll leave it to you to work out how. Drop a reply here if you want some help with that.)

I suspect also that you don't actually need separate methods in Critter for, for example, play_one() and play_all() as the only difference is whether the critter responds to you. You could just add a parameter to the call like this ...

critter.play(response_required=False)

and default it to True in the arguments list like this:

    def play(self,response_required=True):
    """play with one of the critters"""
    self.__pass_time()
    self.fun -= 4
    if self.fun < 0:
        self.fun = 0
    if self.fun > 20:
        self.fun == 20
    if response_required:
        print(self.name, "says: 'That was great fun, thanks for playing with me.'")

There's a small bug in the talking_one() method, as random.randrange(0,5) gives you a number from 0 to 4, and 5 will never happen. I'd suggest using random.choice instead like this:

def talking_one(self):
    """what the critter will say when spoken too, random choice of 6"""
    self.__pass_time()
    print(random.choice(["I like the colour red.",
                         "I am very sleepy today.",
                         "Today is today, i think.",
                         "I eat my shoe.",
                         "Give me your money.",
                         "I can't think of anything else to say, leave me alone."]))

and do the same sort of thing when talking with all of them.

If you do all of that, your code will be much more concise, and much less dependent on repeating the critter names for everything.

One last point. I found myself getting a bit confused over the variable 'fun'. In my head a bigger number for fun suggests there should be more fun, but yours seems to be the other way round. I'd suggest either renaming 'fun' to something like 'boredom' or changing it so bigger numbers = more fun. I know that's a bit tricky given the way it interacts with 'hunger', but a bit more thought about how to represent this would take away what was the only irritant in reading the code.

You're making excellent progress here. Keep up the good work.

\$\endgroup\$
1
\$\begingroup\$

thanks for sharing your code!

I wanted to talk about one pattern that repeats itself quite a bit in your code and how you could improve it.

The long if/elif cases where you check for a name, and then based on the name, you call a method on the relevant object.

if who == 'Dave':
      dave.play_one()
elif who == 'Sarah':
      sarah.play_one()
elif who == 'Clair':
      clair.play_one()
elif who == 'Ashie':
     ashie.play_one()
elif who == 'Daen':
     daen.play_one()

The problem arises here because you're saving the names of the critters in a list, and not the actual objects themselves.

This pattern repeats itself everywhere you need to map a string to a Critter instance.

Now think about what happens if I want to add a new Critter, what do I need to do?

I need to go up to the top of the code and make it, then I need to go to every if/else block that uses it and update it! It' a lot of work to just do a small thing.

Let's try keeping all the Critters in a dictionary.

critters = {
    "Dave" : Critter('Dave'),
    "Sarah": Critter('Sarah'),
    "Clair" : Critter('Clair'),
    "Ashie" : Critter('Ashie'),
    "Daen" : Critter('Daen')
}

now lets re-write this same if-else but using the dict instead.

...
who = who2.capitalize()
critter = critters.get(who)
critter.play_one()
...

Similarly this

who = who2.capitalize()
if who == 'Dave':
    dave.health()
elif who == 'Sarah':
    sarah.health()
elif who == 'Clair':
    clair.health()
elif who == 'Ashie':
    ashie.health()
elif who == 'Daen':
    daen.health()

could be replaced with

who = who2.capitalize()
critter = critters.get(who)
critter.health()

Now, the code that uses the dict no longer needs to be changed when I add a new Critter. I can simply add a new Critter to the dictionary and nothing else needs to change!

You could replace every if/else block that checks a name and calls a corresponding method on an object this way. It's fewer lines of code, and it's also more maintainable.

If you think about how you're currently doing it, you have the object, then you take the name from it. Later, you get the name and get the object back. It's a lot of extra overhead that you don't need to do at all!

Hopefully this was useful for you.

\$\endgroup\$
1
\$\begingroup\$

Sorry, I am very pedantic and that code is excellent but there is a spelling mistake in the __str__ method docstring where you say there when you mean their

\$\endgroup\$
3
  • \$\begingroup\$ (Welcome to CR!) Making the function name stand out is a good idea; enclosing it in "backticks" `` as a code snippet may be more conventional than making it bold. (there is a spelling mistake in the str method docstring moreover, it doesn't stick to the letter of PEP 257: prescribes the function or method's effect […] ("Do this", "Return that") - you can check the question's docstrings for helpfulness, consistency, compliance …) \$\endgroup\$ – greybeard Dec 2 '17 at 16:42
  • \$\begingroup\$ Sorry, I didn't mean to make it bold I just did str surrounded by double underscore and it made it bold for me \$\endgroup\$ – 13ros27 Dec 4 '17 at 15:55
  • 1
    \$\begingroup\$ As you likely noticed, I updated the formatting so that __str__ won't get converted to str- for more information about markdown (which is used for formatting posts here), refer to stackoverflow.com/editing-help#code \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 4 '17 at 16:25

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.