13
\$\begingroup\$

I'm currently working my way through a textbook to enhance my beginner Python skills (Goodrich, Data Structures and Algorithms in Python). The first, somewhat in-depth problem involving OOP was the following:

Write a Python program to simulate an ecosystem containing bears and fish. The ecosystem consists of a river, which is modeled as a list. Each element should be a Bear object, a Fish object, or None. Animals have attributes gender and strength. In each time step, each animal randomly either moves to an adjacent tile or stays where it is. If a fish encounters a bear, the fish gets eaten. If two animals of the same type encounter each other, a new instance of that animal type is generated if they are the same gender. Otherwise, they fight and the one with larger strength survives.

I would really appreciate some feedback on my code. I am not doing this as part of a class, so it's hard to tell whether I'm doing well and where I need improvement.

#!/usr/bin/env python3

"""
Simulation of river environment containing bears and fish.
"""

import random

class Animal:
    """Animals are the inhabitants of the river"""

    def __init__(self, gender = None, strength = None):
        """Initialize animal

        gender     gender of the animal (M, F) determines mating or fighting (default random)
        strength   stregth of animal, determines winner of fight (default random)
        """

        if not gender:
            self._gender = random.choice(['M','F'])
        else:
            self._gender = gender
        if not strength:
            self._strength = random.randint(0,9)
        else:
            self._strength = strength

    def get_gender(self):
       """Return the animals gender"""
        return self._gender

    def get_stren(self):
        """Return the animals strength"""
        return self._strength


class Bear(Animal):
    def __init__(self, gender = None, strength = None):
        super().__init__(gender, strength)

class Fish(Animal):
    def __init__(self, gender = None, strength = None):
        super().__init__(gender, strength)

class River:
    """A river is in array containing animals"""

    def __init__(self, length):
        """Initialize a river with a random assortment of bears, fish, and empty cells

        length     length of the river
        """

        self._length = length
        self._contents = []
        for i in range(self._length):
            rval = random.randint(1,3)
            if rval == 1:
                self._contents.append(Bear())
            elif rval == 2:
                self._contents.append(Fish())
            else:
                self._contents.append(None)

    def __len__(self):
        """Return the length of the river"""
        return self._length

    def __getitem__(self, k):
        """Return the contents of the kth cell in the river list"""
        return self._contents[k]

    def __setitem__(self, k, val):
        """Set the contents of the kth cell in the river list"""
        self._contents[k] = val

    def count_none(self):
        """Count the number of empty cells in the river list"""
        return self._contents.count(None)

    def add_random(self, animal):
        """Add animal to empty cell of river list after mating occurs"""
        if self.count_none() > 0:
            choices = [i for i, x in enumerate(self._contents) if x==None]
            index = random.choice(choices)
            self._contents[index] = animal

    def update_cell(self, i):
        """Update the cell based on rules defined above"""
        if self._contents[i] != None:
            move = random.randint(-1,1) #animal can either move forward, back, or stay in place
            if move != 0 and 0 <= i + move < self._length:
                if self._contents[i + move] == None:
                    self._contents[i + move] = self._contents[i]
                    self._contents[i] = None
                elif type(self._contents[i]) == type(self._contents[i+move]):
                    if self._contents[i].get_gender() != self._contents[i+move].get_gender():
                        #two animals of the same type and different gender mate
                        if type(self._contents[i]) == Bear:
                            self.add_random(Bear())
                        else:
                            self.add_random(Fish())
                    else: #two animals of the same type and gender fight
                        if self._contents[i].get_stren() > self._contents[i+move].get_stren():
                            self._contents[i+move] = self._contents[i]
                        self._contents[i] = None

                else:
                    #bear always kills fish if they encounter eachother
                    if type(self._contents[i]) == Bear: 
                        self._contents[i + move] = self._contents[i]
                    self._contents[i] = None

    def update_river(self):
        """Update each cell in the river"""
        for i in range(len(self._contents)):
            self.update_cell(i)

    def print_river(self):
        """Print the river contents in human readable form
           Each cell displays the animal type, strength, and gender between two pipes

           Example: male bear with strength 8    |B8M|
                    female fish with strength 0  |F0F|
        """
        s = '|'
        for x in self._contents:
            if x:
                if type(x) == Bear:
                    s += 'B'
                elif type(x) == Fish:
                    s += 'F'
                s += str(x.get_stren())
                s += x.get_gender()
            else:
                s += '   '
            s += '|'
        print(s)


if __name__ == "__main__":
    river = River(10)
    for i in range(10):
        print("Year ", i)
        river.print_river()
        river.update_river()
\$\endgroup\$
2
  • 4
    \$\begingroup\$ … a new instance of that animal type is generated if they are the same gender. Otherwise, they fight and the one with larger strength survives. — That's rather interesting biology! \$\endgroup\$ May 1 '15 at 7:19
  • \$\begingroup\$ Error in the description? In the code, a new instance is generated if they are different gender. \$\endgroup\$
    – janos
    May 1 '15 at 12:46
6
\$\begingroup\$

Don't compare against None

Don't compare against None with == or !=. Use is None and is not None.

Use @property instead of getters

Instead of getters like this:

def get_stren(self):
    """Return the animals strength"""
    return self._strength

It's more natural to use properties:

@property
def stren(self):
    return self._strength

When you use this, instead of animal.get_stren(), you will have to write animal.stren (without parentheses).

I also dropped the redundant comment.

Generalize when possible

The current code has many hard-wired elements for Bear and Fish, for example:

    for i in range(self._length):
        rval = random.randint(1,3)
        if rval == 1:
            self._contents.append(Bear())
        elif rval == 2:
            self._contents.append(Fish())
        else:
            self._contents.append(None)

If later you want to add one more animal type, you will have to edit the code in many places, and it can be quite troublesome. You can make the above example easier to extend like this:

    animal_types = (Bear, Fish)
    len_animal_types = len(animal_types)

    for _ in range(self._length):
        rval = random.randint(0, len_animal_types)
        if rval == len_animal_types:
            self._contents.append(None)
        else:
            self._contents.append(animal_types[rval]())

Another opportunity for generalization is when you print letter symbols depending on the animal type:

if type(x) == Bear:
    s += 'B'
elif type(x) == Fish:
    s += 'F'

It would be better to add a @property in Animal:

@property
def symbol(self):
    return self.__class__.__name__[0]

And then you could use simply x.symbol to get the initial letter of the class name of the Animal instance.

Other simplifications

Since strings are iterables, instead of the tedious:

        self._gender = random.choice(['M', 'F'])

You can write simpler:

        self._gender = random.choice('MF')

Follow PEP8

PEP8 is the official Python coding style guide. Please follow it.

For example, instead of:

def __init__(self, gender = None, strength = None):

Write like this:

def __init__(self, gender=None, strength=None):
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thanks so much for your comments! I learned a ton from them. I actually hadn't read about decorators yet, but now that I know about @property, it's clearly an infinitely better approach. I also didn't realize you could just throw brackets after the class type and have a new instance. Super useful. \$\endgroup\$ May 2 '15 at 0:53
6
\$\begingroup\$

First off, kudos; that looks really good. You've clearly put some effort in and it's nice to see docstrings, general style guide compliance (with a few exceptions I've commented on below) and a built-in demo run.


Animal.__init__ could be much neater. Consider the following:

def __init__(self, gender=None, strength=None):
    if gender is None:
        gender = random.choice('MF')
    self._gender = gender
    if strength is None:
        strength = random.randint(0, 9)
    self._strength = strength

Note that:

  • I've altered some of the whitespace around e.g. = and ,, per the style guide.
  • I'm explicitly testing for None, rather than truthiness - if you try to create an Animal with zero strength, you will see why this is generally a good idea!
  • By replacing the argument rather than assigning directly to the attribute, I've cut each case from four lines to three, and as each attribute is only assigned in once place it makes renaming etc. easier.

As a further improvement, you could consider moving the choices for the attributes out of __init__ and into class attributes, e.g.:

GENDERS = 'MF'
STRENGTHS = range(0, 10)

 def __init__(self, gender=None, strength=None):
    if gender is None:
        gender = random.choice(self.GENDERS)
    self._gender = gender
    if strength is None:
        strength = random.choice(self.STRENGTHS)
    self._strength = strength   

This makes it really easy to have different Animal subclasses with different ranges for gender and strength.

Also, there's no point implementing __init__ on the subclasses if all it does is pass the same parameters to super.


Explicit get_ and set_ methods aren't very Pythonic. Instead, we tend to use properties (see e.g. Python @property versus getters and setters on SO). In your case, this would look like e.g.:

@property
def gender(self):
    return self._gender

This looks a lot neater in use, as you don't need to call the property; you access it as a regular attribute:

animal.gender

rather than:

animal.get_gender()

In the River, storing self._length as a separate attribute seems a bit odd - this is just len(self._contents)! You could also neaten up the logic for filling the river, again using class attributes, e.g.:

CELLS = [Bear, Fish, lambda: None]

def __init__(self, length):
    self._contents = [random.choice(self.CELLS)() for _ in range(length)]

Here, note:

  • The use of _ by convention to indicate a value that won't actually be used for anything.
  • I have switched to a list comprehension, a more Pythonic way of building a list.
  • Because we are calling each object retrieved from self.CELLS, it is necessary to make the third option a callable that returns None, rather than None itself; I've used a lambda expression to implement this.

Your description of print_river "Print the river contents in human readable form", makes me think that it should be implemented as __str__, instead. Note that this is easiest if you move the Animal logic up into those classes, e.g.:

class Bear:

    def __str__(self):
        return "B{0.strength}{0.gender}".format(self)


class Fish:

    def __str__(self):
        return "F{0.strength}{0.gender}".format(self)

Note that use of str.format, which is much neater than explicit + concatenation. Also, there seems to be a bit of repetition here; perhaps you could:

think of a way to refactor Animal and its subclasses such that you only need to define __str__ on the base class?

River.__str__ is now very simple:

class River:

    def __str__(self):
        return '|'.join(['   ' if cell is None else str(cell)
                         for cell in self._contents])

str.join will insert the pipes between each cell (although not at the start and end). Note that this has separated the logic for displaying a single Animal from the logic for displaying a whole River, which is good OOP practice.


Finally, there is a lot of logic packed into the update_ methods. I would approach this as follows:

def update(self):  # we know it's a river, that's the class!
    """Update the river, according to the following rules:

    ...

    """
    for index, cell in enumerate(self._contents):
        if cell is not None:  # don't bother updating None cells
            self._update_cell(cell, index)  # leading underscore means "private"

def _update_cell(self, cell, index):
    """Update a single non-empty cell."""
    move = random.randint(-1, 1)
    new_index = index + move
    if new_index in range(len(self._contents)):
        target_cell = self._contents[new_index]
        if target_cell is None:
            self._move(index, new_index)
        else:
            if (isinstance(cell, type(target_cell)) and 
                cell.gender != target_cell.gender):
                self._spawn(cell)
            elif isinstance(cell, type(target_cell)):
                self._fight(index, new_index)
            else:
                self._eat(index, new_index)

def _eat(self, index1, index2):
    """The Fish always gets eaten."""
    ...

def _fight(self, index1, index2):
    """The weaker Animal gets killed."""
    ...

def _move(self, old_index, new_index):
    """Move from old_index to new_index."""
    ...

def _spawn(self, cell):
    """Spawn a new instance of the class in a free cell."""
    ...

Note that rather than type(self._contents[i]) == Bear you should use isinstance(self._contents[i], Bear).

\$\endgroup\$
1
  • 1
    \$\begingroup\$ This is incredibly helpful, and I learned a lot. Thank you! str.format isn't something I'd used before, and it's definitely a lot cleaner. Same with @property. Between you and the above commenter, my code is not infinitely cleaner. \$\endgroup\$ May 2 '15 at 1:05
3
\$\begingroup\$

You have a problem with the logic in your code.

If the animal moves to the next cell, it gets to move again in the same step.

\$\endgroup\$
2
  • 3
    \$\begingroup\$ This does provide an answer to the question. The code contains a bug. \$\endgroup\$
    – Pimgd
    May 1 '15 at 10:03
  • \$\begingroup\$ Thanks! I noticed this as I was finishing up my approach. After cleaning things up with the recommendations above, I will look for a way to tackle this bug. \$\endgroup\$ May 1 '15 at 22:10

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.