6
\$\begingroup\$

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
    def addPlayer(self, name, number, position):
        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()

this.addPlayer('Messi', 10, 'forward')
this.addPlayer('Neymar', 11, 'forward')
this.addPlayer('Song', 9, 'midfield')
this.addPlayer('Valdes', 1, 'goalie')
this.addPlayer('Pique', 25, 'defense')
this.addPlayer('Pedro', 8, 'forward')

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
\$\endgroup\$
2
  • \$\begingroup\$ 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. \$\endgroup\$ – Alex L Mar 16 '14 at 22:16
  • \$\begingroup\$ True, at this point it is useless. I think I had intentions of using it later on. Thanks. \$\endgroup\$ – Kyle.Atkin5 Mar 17 '14 at 1:17
6
\$\begingroup\$

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': []
        }

    def add_player(self, name, number, position):
        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')
bcn.add_player('Messi', 10, 'forward')
bcn.add_player('Neymar', 11, 'forward')
bcn.add_player('Song', 9, 'midfield')
bcn.add_player('Valdes', 1, 'goalie')
bcn.add_player('Pique', 25, 'defense')
bcn.add_player('Pedro', 8, 'forward')
bcn.add_player('Josay', 10, 'invalid')

# 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
\$\endgroup\$
2
  • \$\begingroup\$ 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. \$\endgroup\$ – Kyle.Atkin5 Mar 18 '14 at 4:28
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – DSM Mar 18 '14 at 19:22
4
\$\begingroup\$

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.

\$\endgroup\$

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.