Discount Pokemon Battles

Question

I have decided to create a discount Pokemon battle simulator using a module called guizero. It's basically a wrapper for Tkinter that is easier to learn. Recently, I started to learn OOP and thought that this project would be a good practice for my "new-found skills". Someone in my computing group looked at my code and told me it was terrible so I thought I'd get someone else's opinion.

Edit

I just want to point out that I've been trained to comment everything I code to a standard the a beginner could understand it.

This is the code (you will have to download guizero to make it work):

# Import guizero for the GUI and random
import guizero as gui
import random

# Create an empty list for the Animals
actives = []

# Create an immutable list (tuple) for default animals
DEFAULT_ATTRS = (('Cow',     100,  10,  15,   4),
                 ('Chicken',  40,  50,  40,   5),
                 ('Duck',     45,  35,  70,   2))

# Create a function that checks if all values in a list are equal
def all_equal(iterable):
    for i in range(len(iterable)):
        if iterable[i-1] != iterable[i]:
            return False
    return True

# Create an Animal class for the animals
class Animal:

    # Create an immutable value so that another values can't change the string
    # Make a string that can be formated and evaluated
    POWERSUM = 'int((({1}+{2})*{3})/{0})'

    def __init__(self,name=None,strength=None,speed=None,skill=None,age=None):

        assign = all_equal([name,strength,speed,skill,age]) and name == None
        self.assigned = not assign
        
        if assign:
            return None

        self.optimum = 50
        self.name = name.title()
        attr = [strength,speed,skill]

        while sum(attr) > random.randint(180,220):
            # If the sum is greater than 220 (or less)
            # Change the max and the min values
            attr[attr.index(max(attr))] -= random.randint(1,9)
            attr[attr.index(min(attr))] += random.randint(1,5)

        self.strength, self.speed, self.skill = attr
        self.fitness = 100
        self.attr = attr[:]

        self.active = True

        # Create a list with the values [number of battles, battles lost]
        self.battles = [0,0]

        self.age = age
        self.power = 0

    def __repr__(self):
        # Create the display string
        # Name. Stats: Strength, Spped, Skill and Age
        attr = self.attr[:]+[self.age]
        return '{}. Statistics: {},{},{} and {}'.format(self.name,*attr)

    def returnPower(self):
        # Get the power. The optimum age is 50
        # Effectively create a parabola effect on the age
        if self.age > 50:
            age = self.optimum / (101-self.age)
        else:
            age = self.optimum / self.age
        self.power = eval(self.POWERSUM.format(age,*self.attr))
        return self.power

# Add the three default values
for attr in DEFAULT_ATTRS:
    actives.append(Animal(*attr))

class BattleWindow(gui.App):

    # Create a class that creates a GUI,
    # avoiding the need for global variables; they're all attributes

    def __init__(self,*animals):

        super().__init__(title='Animals Battles',layout='grid')

        # Create the function so that if the window is closed,
        # it automatically opens the menu window
        self.on_close(self.cancel)

        texts = [[],[]]

        for i,person in enumerate(['Animal Selected','Opponent']):
            texts[i].append(person)
            for cate in ['Strength','Skill','Speed','Age','Fitness','Power']:
                texts[i].append(cate)

        buttons = ((self.power,      'Power' ,[0,0]),
                   (self.opponent,'Opponent' ,[1,0]),
                   (self.battle,    'Battle' ,[2,0]),
                   (self.firstaid,'First aid',[3,0]))

        for func,text,grid_xy in buttons:
            self.aidbtn = gui.PushButton(self,func,text=text,grid=grid_xy)

        self.animals = list(animals)

        # Create 2 'empty' animals that can't do anything
        # just in case the user tries to do something
        self.chosen = Animal()
        self.opponent = Animal()
        
        self.displays = [[],[]]
        self.options = ['None']
        
        for animal in animals:
            self.options.append(animal.name)
            
        # Create a Combo to choose the animal
        self.combo = gui.Combo(self,self.options,command=self.disp,grid=[0,2])

        for i,text in enumerate(texts):
            for x,tx in enumerate(text):
                
                pos = [[x],[x]]
                
                if i%2 == 0:
                    pos[0].append(1)
                    pos[1].append(2)
                else:
                    pos[0].append(3)
                    pos[1].append(4)

                gui.Text(self,text=tx+': ',grid=pos[0],align='left')
                if tx != 'Animal Selected':
                    self.displays[i].append(gui.Text(self,grid=pos[1]))

        # Display the GUI so that everything shows up
        self.display()
        
    def battle(self):
        
        fitness = 'fitness'
        if not (hasattr(self.chosen,fitness) or hasattr(self.opponent,fitness)):
            gui.warn('No opponent!','You need an opponent!')
            return
        
        # Decrease the fitnesses of the animals by 75% of the value
        self.opponent.fitness *= 0.75
        self.chosen.fitness *= 0.75

        # Add 1 to the number of battles
        self.chosen.battles[0] += 1
        self.opponent.battles[0] += 1

        # If power has not yet been calculated,
        # return so that the battle never happens
        if self.displays[0][-1].get() == 'N/A':
            return
    
        if self.opponent.power > self.chosen.power:
            winner = self.opponent
            self.chosen.fitness *= 0.75
            self.chosen.battles[1] += 1
        else:
            winner = self.chosen
            self.opponent.fitness *= 0.75
            self.chosen.battles[1] += 1
        gui.info('The winner is...','The Winner is ... {}'.format(winner.name))
        
        # Set the fitness display to the fitness to 2d.p.
        self.displays[0][-2].set(round(self.chosen.fitness,2))
        self.displays[1][-2].set(round(self.opponent.fitness,2))

        # Check if either fitness is less than 1 as
        # 0 can never be reached
        if self.opponent.fitness < 1 or self.chosen.fitness < 1:

            if self.opponent.fitness < 1:
                self.opponent.active = False
                name = self.chosen.name
                popname = self.opponent.name
                x = 1
                    
            if self.chosen.fitness < 1:
                self.chosen.active = False
                name = 'None'
                popname = self.chosen.name
                x = 0

            # Clear the displays if the fitnesses are less than 1
            for disp in self.displays[x]:
                disp.clear()

            # Remove the name from the dropdown options
            # then destroy the combo and create a new one
            # The new combo is then set to either the current
            # animal or None if the user animal faints
            self.options.remove(popname)
            self.combo.destroy()
            self.combo = gui.Combo(self,self.options,grid=[0,2])
            self.combo.set(name)

            # Get rid of the Animal object from the animals so that
            # the random opponent can't be one of the fainted ones
            actives.pop([i.name for i in actives].index(popname))

    def cancel(self):
        # Go back to the menu system
        self.destroy()
        Menu()

    def disp(self,_):
        # If the combo is None, set the displays to N/A
        if self.combo.get() == 'None':
            for disp in self.displays[0]:
                disp.set('N/A')

        self.chosen = self.animals[self.options.index(self.combo.get())-1]

        # Create a copy of the attr attribute of self.chosen.attr
        # Next add the age and the fitness to the list
        attrs = self.chosen.attr[:]
        attrs.append(self.chosen.age)
        attrs.append(self.chosen.fitness)

        # Next change the displays to the
        # appropriate values
        for i in range(len(attrs)):
            self.displays[0][i].set(attrs[i])
        # Finally set the 'Power' display to N/A
        self.displays[0][-1].set('N/A')

    def firstaid(self):
        # Create a function that allows self.chosen to get more fitness
        if self.chosen.battles[0] == 0:
            return
        # Check if the battle win percentage is high enough to get first aid
        if 100 * (self.chosen.battles[1] / self.chosen.battles[0]) > 60:
            if self.chosen.fitness > 50:
                amount = 100 - self.chosen.fitness
            else:
                amount = 50
            self.chosen.fitness += amount
            self.displays[0][-2].set(round(self.chosen.fitness,2))
            # Make the button disabled so that it can't be pressed again
            self.aidbtn.config(state=gui.DISABLED)
        else:
            gui.warn('Too many losses','You haven\'t won enough battles!')

    def opponent(self):
        # Randomly choose an enemy. While that enemy
        # is the same as the 'chosen', choose again
        value = random.choice(actives)
        while value == self.chosen:
            value = random.choice(actives)
        self.opponent = value

        # Create a copy of the opponent attrs
        # Then add the age, fitness and name
        attrs = self.opponent.attr[:]
        attrs.append(self.opponent.age)
        attrs.append(self.opponent.fitness)
        attrs.insert(0,self.opponent.name)

        # Add the displays for the opponent
        for i in range(len(attrs)):
            self.displays[1][i].set(attrs[i])
        self.displays[1][-1].set('N/A')

    def power(self):
        # Set the text to the power. Doesn't need
        # the value to be assigned; happens in the returnPower() function
        if self.chosen.assigned:
            self.displays[0][-1].set(self.chosen.returnPower())
        if self.opponent.assigned:
            self.displays[1][-1].set(self.opponent.returnPower())

# Create the default window that creates
# a menu system
class Menu(gui.App):

    def __init__(self):

        super().__init__(title='Menu System',layout='grid',height=300)

        gui.Text(self,text='Please choose an option',grid=[0,0])

        # Create a 2d tuple containing the infos
        options = (('Add new animal',self.addNew,[1,0]),
                   ('Battle!!!',self.battle,[2,0]),
                   ('Delete animal',self.delete,[3,0]))

        # Create a list containing the names of the
        # animals for leter
        self.names = [i.name for i in actives]

        # Create the buttons for the options
        for text,func,grid_xy in options:
            gui.PushButton(self,func,text=text,grid=grid_xy)

        # Display all the widgets from the GUI
        self.display()

    def clear(self):
        # Clear the texts used
        for text in self.text:
            text.destroy()
        # Clear the entries used
        for ent in self.entries:
            ent.destroy()
        # Clear and delete the 2 buttons
        self.btn.destroy()
        self.cancel.destroy()
        del self.btn,self.cancel

    def addAnimal(self):
        # Create a list of the gotten values
        self.got = []
        for i in range(len(self.entries)):
            self.got.append(self.entries[i].get())
        if self.got[0] == '':
            gui.error('Name','Please provide a name')
            return
        # Add the animal to the actives values
        actives.append(Animal(*self.got))
        gui.info('Animal Added','{} added!'.format(self.got[0]))
        # Clear the widgets
        self.clear()
        
    def addNew(self):
        # Create a tuple containg the Text widget information
        texts = (('strength',[1,2],[1,1]),
                 ('speed',   [2,2],[2,1]),
                 ('skill',   [3,2],[3,1]),
                 ('age',     [4,2],[4,1]))
        entries = []
        text = []
        
        text.append(gui.Text(self,text='Enter animal name: ',grid=[0,1]))
        entries.append(gui.TextBox(self,grid=[0,2],width=25))
        # Create the Text widgets and the Slider widgets
        for t,sc,tc in texts:
            text.append(gui.Text(self,text='Enter animal '+t+': ',grid=tc))
            entries.append(gui.Slider(self,start=1,end=100,grid=sc))

        # Create copies of the entries and text lists
        self.entries = entries[:]
        self.text = text[:]
        # Create the 2 buttons for submitting and cancelling
        self.btn = gui.PushButton(self,self.addAnimal,text='Submit',grid=[6,1])
        self.cancel = gui.PushButton(self,self.clear,text='Cancel',grid=[6,2])

    def battle(self):
        # Destroy menu window and open BattleWindow
        self.destroy()
        BattleWindow(*actives)

    def deleteOne(self):
        # If the combo for deletion does not equal None
        # pop the name from actives and give a info window
        if self.todelete.get() != 'None':
            index = self.names.index(self.todelete.get())
            delete = actives.pop(index).name
            gui.info('Deleted','{} has been deleted!'.format(delete))
            self.names.pop(index)
        # Destroy the button and Combo
        self.todelete.destroy()
        self.bn.destroy()

    def delete(self):
        # Create a combo for the animal and a 'delete' button
        self.todelete = gui.Combo(self,['None']+self.names,grid=[0,1])
        self.bn = gui.PushButton(self,self.deleteOne,text='Delete',grid=[1,1])

# Initialize the menu window to start  
Menu()

Answer 1

# Import guizero for the GUI and random
import guizero as gui
import random

This, like many of your comments, is exactly the kind of comment you should never write. It just says what the code does. We can see what the code does. Use comments, where absolutely necessary, to explain why it does it. You've added that

I've been trained to comment everything I code to a standard the a beginner could understand it

but I don't think commenting everything helps in that respect, compared to well-written code with sensible variable names and good docstrings that will appear in the help.

Also you should group imports, standard library first, per the style guide.

import random

import guizero as gui

---

# Create an immutable list (tuple) for default animals
DEFAULT_ATTRS = (('Cow',     100,  10,  15,   4),
                 ('Chicken',  40,  50,  40,   5),
                 ('Duck',     45,  35,  70,   2))

A minor thing, but a tuple is not just an immutable list; see e.g. What's the difference between lists and tuples?

Also, I wouldn't align values like that; use a single space after each comma, otherwise if you add another item with a longer name or value you have to realign everything.

---

# Create a function that checks if all values in a list are equal
def all_equal(iterable):

This comment is redundant again and, worse, inconsistent with the code. Evidently you've realised that this functionality works fine with non-list iterables and renamed the argument, but you haven't updated the comment. Also when you're describing modules, classes and functions you should do so with docstrings, not just comments:

def all_equal(iterable):
    """Whether all of the values in the iterable are equal."""

This makes them useful to IDEs, documentation generators, etc.

---

assign = all_equal([name,strength,speed,skill,age]) and name == None
self.assigned = not assign

if assign:
    return None

This is bad; sorry, no two ways about it. If this is intended as validation of the inputs, so at least one of the values must be provided, it should look something like:

if all(item is None for item in [name,strength,speed,skill,age]):
    raise ValueError('at least one of the inputs must be provided')

You could use any instead of all to mean "all of the inputs must be provided", but in that case why provide default parameter values at all? Note the comparison of None by identity (is) not equality (==); it's a singleton.

---

Your initialisation in general seems too long and complex. Specifically, I would extract this:

attr = [strength,speed,skill]

while sum(attr) > random.randint(180,220):
    # If the sum is greater than 220 (or less)
    # Change the max and the min values
    attr[attr.index(max(attr))] -= random.randint(1,9)
    attr[attr.index(min(attr))] += random.randint(1,5)

self.strength, self.speed, self.skill = attr

To be:

self.strength, self.speed, self.skill = self._adjust_attr_values(strength, speed, skill)

Again, this method would have a docstring explaining why this is necessary.

---

I wouldn't store self.attr; that duplicates existing information, and risks updating one but not the other. If it's really needed, it should be a calculated and ideally read-only property:

@property
def attr(self):
    return [self.strength, self.speed, self.skill]

---

# Create a list with the values [number of battles, battles lost]
self.battles = [0,0]

One problem with this is that the list doesn't actually keep that content with it. You will find yourself writing battles, lost = thing.battles, even when you only want one of them, and then you mix up the order one time and you've got a really tricky bug to track down. Why not have two attributes?

self.battles_won = 0
self.battles_lost = 0 

You could add another property for the total. Or create a new object entirely to just hold wins and losses with the total, but that's probably overkill.

In general also I would first assign all of the parameters, then do all of the initialisation from fixed values. This means that the reader can get the context of the parameters out of their head as early as possible.

---

def __repr__(self):
    # Create the display string
    # Name. Stats: Strength, Spped, Skill and Age
    attr = self.attr[:]+[self.age]
    return '{}. Statistics: {},{},{} and {}'.format(self.name,*attr)

Per the data model, __repr__ should:

...look like a valid Python expression that could be used to recreate an object with the same value (given an appropriate environment). If this is not possible, a string of the form <...some useful description...> should be returned.

Your method does neither, so should be called __str__.

---

def returnPower(self):
    # Get the power. The optimum age is 50
    # Effectively create a parabola effect on the age
    if self.age > 50:
        age = self.optimum / (101-self.age)
    else:
        age = self.optimum / self.age
    self.power = eval(self.POWERSUM.format(age,*self.attr))
    return self.power

Based on all of the above:

@property
def power(self):
    """Calculate the power. The optimum age is 50."""
    age = 50 / ((101 - self.age) if self.age > 50 else self.age)
    return int(((self.strength + self.speed) * self.skill) / age)

This is far less cryptic than your formulation; I'm not sure what risk you were trying to mitigate with the POWERSUM, and if all animals have the same optimum why make it an instance attribute?

---

# Create an empty list for the Animals
actives = []

# Create an immutable list (tuple) for default animals
DEFAULT_ATTRS = (('Cow',     100,  10,  15,   4),
                 ('Chicken',  40,  50,  40,   5),
                 ('Duck',     45,  35,  70,   2))

...

# Add the three default values
for attr in DEFAULT_ATTRS:
    actives.append(Animal(*attr))

This all seems a bit odd looking back. Why not define the class then just do:

actives = [
    Animal('Cow', 100, 10, 15, 4),
    Animal('Chicken', 40, 50, 40, 5),
    Animal('Duck', 45, 35, 70, 2),
]

Answer 2

Most of your comments aren't that great, but thank you for including them, as some helped when changing your code. Comments are good when your code is hard to understand or read, and so if you optimize something you'd want to include the comment. And so after doing a couple of changes, the only comment I'd keep would be the one in returnPower.

But your code is fairly good, on a line by line level your code is pretty good. However, your code as a whole is poorly structured. You want to off-load as much code as you can from your GUI classes to other classes. In this case, a Battle class would be a good addition. And you want a function that displays all the information about your battle, so that you can call that one function after each change to the battle.

Animal

• Each animal needs to have a name, strength, speed, skill and age, and so having these as optional arguments is counter intuitive, and can lead to incorrect object state. Instead have these as just normal needed arguments, and set self.chosen to None.

• Using random in the constructor is bad, as it doesn't allow developers to accurately save the animals state. Instead on each load of the game the animal may change. If you want to implement this, do it outside the class, such as when you create a new animal.

• Organize the constructor so that it's easier to read, moving all the assignments to the top, and all the default values to the bottom makes it much easier to read.

• You should want to make returnPower a property, and remove power, as renaming the function to power allows you to use it as you want.

• You should implement the fight mechanics here, you don't need any knowledge of the battle, to allow an animal to fight another one. This can decide the winner, and mutate the fitness for both animals.

Battle

• This should be an implementation of how animals battle each other, irrespective of if the program is using a GUI, or a console script.

• It only need to know what the possible animals are, and the current two are.

• When you implement each battle on this class, you should check that the class is in the correct state. Battle the animals, and then set the animals to None, or another animal, if they run out of fitness.

• When implementing firstaid, you only need to check that the class is in the correct state, and then call min, rather than a hard to read if/else.

• Finally change_opponent should just change the opponent, and ensure that the new opponent is not the same as the currently selected animal.

BattleWindow

• I personally would split up the creation of the different parts of the battle, to small functions. Prefixed with _build_. I would split them into the following groups: buttons, dropdown, stat_labels, display, and opponent_label.

• I'd heavily promote a display_battle function, this should update all the stats for both animals, and the opponents name. It doesn't matter if any of these have changed, if you update everything correctly, you don't have to implement this anywhere else.

• Finally; battle, firstaid, and opponent, all become much simpler, you call the Battle class' functions, and display any output.

---

And so I'd use the following classes:

class Animal:
    POWERSUM = 'int((({1}+{2})*{3})/{0})'
    def __init__(self, name, strength, speed, skill, age):
        self.name = name.title()
        self.strength = strength
        self.speed = speed
        self.skill = skill
        self.age = age

        self.optimum = 50
        self.fitness = 100
        self.battles = [0, 0]

    def __str__(self):
        return ('{} ({}, {}, {}, {})'.format(self.name, self.strength,
                                             self.speed, self.skill, self.age))

    @property
    def power(self):
        # Get the power. The optimum age is 50
        # Effectively create a parabola effect on the age
        if self.age > 50:
            age = self.optimum / (101-self.age)
        else:
            age = self.optimum / self.age
        return eval(self.POWERSUM
                    .format(age, self.strength, self.speed, self.skill))

    def fight(self, other):
        for animal in (self, other):
            animal.fitness *= 0.75
            animal.battles[0] += 1

        winner, looser = (
            (self, other)
            if self.power > other.power else
            (other, self)
        )
        winner.battles[1] += 1
        looser.fitness *= 0.75

        for animal in (self, other):
            if animal.fitness < 1:
                animal.active = False
        return winner, looser


class Battle:
    def __init__(self, animals):
        self.animals = list(animals)
        self.chosen = None
        self.opponent = None

    def battle(self):
        if self.chosen == self.opponent:
            return False, "Animals can't fight themselves"
        if self.chosen is None or self.opponent is None:
            return False, "One or more animals have not been selected"
        ret = self.chosen.fight(self.opponent)
        if self.chosen.fitness < 1:
            self.animals.remove(self.chosen)
            self.chosen = None
        if self.opponent.fitness < 1:
            self.animals.remove(self.opponent)
            self.change_opponent()
        return True, ret

    def firstaid(self):
        if self.chosen is None:
            return False, 'Animal is None'
        if self.chosen.battles[0] == 0:
            return False, 'No battles'
        if 100 * (self.chosen.battles[1] / self.chosen.battles[0]) <= 60:
            return False, "You haven't won enough battles!"
        self.chosen.fitness = min(self.chosen.fitness + 50, 100)
        return True, None

    def change_opponent(self):
        choices = [a for a in self.animals if a != self.chosen]
        if choices:
            value = random.choice(choices)
        else:
            value = None
        self.opponent = value


class BattleWindow(gui.App):
    def __init__(self, *animals):
        super().__init__(title='Animals Battles', layout='grid')
        self._battle = Battle(animals)
        self.on_close(self._cancel)

        self._buttons = self._build_buttons()
        self._dropdown = self._build_dropdown()
        self._labels = self._build_stat_labels()
        self._display = self._build_display()
        self._opponent = self._build_opponent_label()

        # Default enemy, and update view
        self.opponent()
        self.display()

    def _cancel(self):
        # Go back to the menu system
        self.destroy()
        Menu()

    def _build_buttons(self):
        buttons = (
            (self.battle,   'Battle',    [0, 0]),
            (self.firstaid, 'First aid', [1, 0]),
            # (self.opponent, 'Opponent',  [2, 0])
        )
        return [
            gui.PushButton(self, func, text=text, grid=grid_xy)
            for func, text, grid_xy in buttons
        ]

    def _build_dropdown(self):
        options = ['None'] + [animal.name for animal in self._battle.animals]
        return gui.Combo(self, options, command=self._dropdown_change,
                         grid=[0, 2])

    def _build_stat_labels(self):
        cols = ['Strength', 'Skill', 'Speed', 'Age', 'Fitness', 'Power']
        labels = []
        for i, person in enumerate(['Animal Selected', 'Opponent']):
            i = i*2 + 1
            labels.append([
                gui.Text(self, text=txt+': ', grid=[j, i], align='left')
                for j, txt in enumerate([person] + cols)
            ])
        return labels

    def _build_display(self):
        display = []
        for i in range(2):
            i = i*2 + 2
            display.append([
                gui.Text(self, grid=[j, i])
                for j in range(1, 7)
            ])
        return display

    def _build_opponent_label(self):
        return gui.Text(self, grid=[0, 4])

    def _dropdown_change(self, _):
        chosen = self._dropdown.get()
        if chosen == 'None':
            chosen = None
        else:
            possible = (a for a in self._battle.animals if a.name == chosen)
            chosen = next(possible, None)

        self._battle.chosen = chosen
        self.display_battle()

    def display_battle(self):
        battle = self._battle
        self._opponent.set('None'
                           if battle.opponent is None else
                           battle.opponent.name)
        animals = [battle.chosen, battle.opponent]
        for disp, animal in zip(self._display, animals):
            if animal is None:
                for d in disp:
                    d.set('N/A')
            else:
                disp[0].set(animal.strength)
                disp[1].set(animal.skill)
                disp[2].set(animal.speed)
                disp[3].set(animal.age)
                disp[4].set(round(animal.fitness, 2))
                disp[5].set(animal.power)

    def battle(self):
        status, ret = self._battle.battle()
        if not status:
            gui.warn('Battle', ret)
        else:
            gui.info('Battle', 'The Winner is {}'.format(ret[0].name))
        self.display_battle()

    def firstaid(self):
        status, ret = self._battle.firstaid()
        if not status:
            gui.warn('Firstaid', ret)
        self.display_battle()

    def opponent(self):
        self._battle.change_opponent()
        self.display_battle()