# Discount Pokemon Battles

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()

• You made some rookie mistakes, but that's ok. I'll point them out in a review if they aren't pointed out already in 9-10 hours. No time for an in-depth review now :-)
– Mast
Mar 21, 2017 at 8:07
• "Terrible"s a pretty harsh overstatement. Have a look around StackOverflow. This is beautiful compared to some code I've seen recently. Mar 21, 2017 at 15:11

## 2 Answers

# 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),
]

• think you got the assign = all equal(... wrong, i think this is a check to see if values have been set or not. If name is None AND all fields are equal (including name) ==> all Fields are None Mar 21, 2017 at 14:18
• @Lex oh, I see, thanks. I'll edit in the morning; I guess all(thing is None for ...) would be better then. Mar 21, 2017 at 14:42

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()