# Modeling a soccer team with substitutions

I'm new to Python and want to make sure I'm not developing bad habits. If you could please review this code below and give me any tips, practice ideas or critiques you might have. Any ideas on which aspects of the language I should focus on early would also be appreciated.

#!usr/env/python
#Modeling a soccer team with substitutions

#Players stored in dictionary {Name:Numbers}
players = {}
# Player positions stored in a list
forward = []
midfield = []
defense = []
goalie = []

class Team(object):
def __init__(self, name):
self.name = name

class Players(object):
def __init__(self):
pass
#creates a player list with names, numbers, and preferred positions
self.name = name
self.number = number
self.position = position
players[name] = number

if position == 'forward':
forward.append(name)
return forward
elif position == 'midfield':
midfield.append(name)
return midfield
elif position == 'defense':
defense.append(name)
return defense
elif position == 'goalie':
goalie.append(name)
return goalie
else:
print 'Only forward, midfield, defense, goalie accepted'

print "Added %s to the team!" % name

#Should list the players currently on the field. Only 3 player sample data
players_on_field = []
class OnField(object):
def __init__(self):
pass
def sub_in(self, sub_name):
self.sub_name = sub_name
if len(players_on_field) == 3:
print "Field is full, sub_out first!"
#testing some options
#x = raw_input('Which player would you like to sub out?')
#OnField.sub_out(x)
else:
players_on_field.append(sub_name)
print "%s entering the game" % sub_name
return

def sub_out(self, sub_name):
for i in range(3):
if players_on_field[i] == sub_name:
del players_on_field[i]
print "%s has been removed from the game" % sub_name
return
#testing some options
#x = raw_input('Who would you like to sub in?')
#OnField.sub_in(x)
else:
print "That player is not on the field"
return

def list_players_on_field(self):
print players_on_field

#Testing
Barcelona = Team('Barcelona')
this = Players()
game = OnField()

print players

print 'Forward Players'
print forward
print 'Midfield Players'
print midfield
print 'Defense Players'
print defense
print 'Goalies'
print goalie

#Subsitution testing
print "Players currently on the field"
print players_on_field
game.sub_in('Messi')
game.sub_in('Pedro')
game.sub_in('Song')
game.list_players_on_field()
game.sub_out('Pedro')
print players_on_field
game.sub_in('Neymar')
print players_on_field

• Your Team class is useless, no? It has no use (a player is not assigned to a team), so I don't think you need it. – Alex L Mar 16 '14 at 22:16
• True, at this point it is useless. I think I had intentions of using it later on. Thanks. – Kyle.Atkin5 Mar 17 '14 at 1:17

You naming convention does not follow PEP 8. Try to follow it as much as you can as it helps having same consistent guidelines all over Python code.

I do not quite understand how the on field thingy is supposed to work so I'll skip on that part.

Try to avoid global variables. Try to think how things would be affected if you were trying to handle multiple teams. Most probably things would go wrong.

I guess your different lists should be part of the team class. Also, there is not much point for having a Player class as a player on its own is nothing but a name. As far as I understand soccer, the number is for a given player in a given team. Thus, the team should be some kind of containers of pairs name/number.

Also, it seems like your are doing the same things for different teams of players that could be handled the same way. Instead of write code, it's easier to use smart data structures and make them work for you. In your case, it seems like a dictionnary from position to list of (player name + player number) is good idea. Default composition is to have empty lists and to fill them later on.

Here's what my code looks like :

class Team(object):
def __init__(self, name):
self.name = name
self.composition = {
'forward' : [],
'midfield': [],
'defense': [],
'goalie': []
}

if position not in self.composition:
print 'Only %s accepted' % ', '.join(self.composition.keys())
else:
self.composition[position].append((name,number))
print 'Added %s to the team!' % name

bcn = Team('Barcelona')

# It could be a good idea to create a method to do this :-)
for pos, players in bcn.composition.iteritems():
print pos
for p in players:
print p

• not in .keys() is a much better way to handle that check. I wasn't aware of it before. Thank you! On_Field was supposed to be kind of a snapshot of the available players if the coach were looking at options to sub players in / out. – Kyle.Atkin5 Mar 18 '14 at 4:28
• It's more idiomatic to write position not in self.composition rather than position not in self.composition.keys(). Moreover, in Python 2, .keys() generates a separate list that needs to be inefficiently searched. – DSM Mar 18 '14 at 19:22

The idea of a class in python is to keep track of a state. Your OnField class does nothing else than appending and removing players from players_on_field(which is outside the class). Therefor, sub_in and sud_out could be defined outside the class.

players_on_field = []
class OnField(object):
def __init__(self):
pass
def sub_in(self, sub_name):
self.sub_name = sub_name
if len(players_on_field) == 3:
print "Field is full, sub_out first!"
#testing some options
#x = raw_input('Which player would you like to sub out?')
#OnField.sub_out(x)
else:
players_on_field.append(sub_name)
print "%s entering the game" % sub_name
return

def sub_out(self, sub_name):
for i in range(3):
if players_on_field[i] == sub_name:
del players_on_field[i]
print "%s has been removed from the game" % sub_name
return
else:
print "That player is not on the field"
return

def list_players_on_field(self):
print players_on_field


Could become

players_on_field = []

def sub_in(sub_name):
if len(players_on_field) == 3:
print "Field is full, sub_out first!"
#testing some options
#x = raw_input('Which player would you like to sub out?')
#OnField.sub_out(x)
else:
players_on_field.append(sub_name)
print "%s entering the game" % sub_name
return

def sub_out(sub_name):
for i in range(3):
if players_on_field[i] == sub_name:
del players_on_field[i]
print "%s has been removed from the game" % sub_name
return
#testing some options
#x = raw_input('Who would you like to sub in?')
#OnField.sub_in(x)
else:
print "That player is not on the field"
return


I would avoid to print errors. You should raise errors. Ex: raise PlayerNotOnFieldError()

You are also using some 'magic values'. Ex: if len(players_on_field) == 3:. I would suggest that you define a constant MAX_NUMBER_OF_PLAYER_ON_THE_FIELD = 3 so that the if statement alone is verbose. The same apply to the player positions. This way you could re-use the constants and only maintain then in one place.