The concept is good, but there is room for improvement. Here are some problems I see:
Clear names are very important in coding, because they make it easy or hard for other people (including your future self) to understand what you have written. Some of the names you have used do not match what the code actually does, which is confusing.
- Based on the name of the
Human class, I'd expect an instance of this class to represent one human, but that is not how it is used (see below).
- Based on the name of the
Player class, and the way it extends
Human, I'd expect an instance of this class to represent one player; if so, then there should be 22 instances of the class, or one object for each name in the
players list. Instead, it seems to represent all the players at once, which its name does not imply.
- I would expect the
name attribute of
Human to store a single name, but
Player stuffs an entire list of player names into that attribute.
At minimum, these names should be made plural (
Players, etc.) to reflect their contents. It would be even better to give
Player a name that describes what it actually does, or what it is responsible for within the program.
Unnecessary hierarchy, part 1: only one subclass
You mentioned in a comment that you are trying to practice your OOP skills, so "[you] want to force [yourself] to use classes even if it sounds silly". Practice is good, but part of good OOP is understanding why and how to use classes and objects effectively...and that includes knowing when not to use them, or when not to use certain parts of them.
For example: why do you need both a
Human class and a
Player subclass? In the program that you have now, the only humans are players; therefore there is no advantage to having two classes. You could do everything in the
Player class and drop the
Human class altogether, or vice versa. On the other hand, if you already have plans to expand your program to include non-player humans (e.g. coaches, referees), then a
Human superclass might make sense. The structure of the code should fit your use case.
Unnecessary hierarchy, part 2: superclass does nothing
A subclass is supposed to have a "type of" or "kind of" relationship to its superclass, e.g. a
Square is a kind of
Rectangle, or a
Dog is a kind of
Animal. The problem is that your code does not gain much from defining
Player as a kind of
Human has no defined behaviors and barely any data, inheriting from it doesn't provide many benefits. It would be simpler and more flexible to use composition instead of inheritance, by creating or importing a
Human object inside the
Unnecessary use of internal state / object attributes
One of the nice things about objects is that they can create and maintain an internal state--a little chunk of data that it hides away from the rest of the program. (Keeping separate things separate is called encapsulation or separation of concerns, and it makes programs much easier to think about and easier to debug.) In Python, internal state is stored in object attributes, like
self.name. But you have to know when to use internal state and when not to use it. Keeping multiple copies of the same data in different places, or multiple references to the same data object, can cause confusion and lead to bugs.
get_team() method takes a list of players, splits the players between two known teams, and then returns a
dict() with that information. This method is called only once in your entire program. So, what is the advantage of having the
Player object store the list of player names inside itself? In the current code, there is no advantage. You could easily change
get_team() to a class method (which can be called without instantiating an object) that takes the player names as a parameter.
# list of teams
teams = ['A', 'B']
Since you defined this
teams list at the top of the code, I would assume that editing that list would change the program output. But when I look closer, I can see that this list is not even used. Your
get_team() method is explicitly written to split the players into two teams named 'A' and 'B', regardless of what is in the list above.
teams['A'] = self.name[0::2]
teams['B'] = self.name[1::2]
This is not very flexible, especially if you want to import the team names from outside the program (e.g. from user input, or from a config file). A better method would take a list of teams as an input, count the number of teams, and then split the players accordingly.
Here's how I might write it:
TEAMS = ('A', 'B') # Replaced mutable list with immutable tuple.
# Names of constants should be in all-caps.
PLAYERS = ('a', 'b', 'c', 'd', 'e',
'f', 'g', 'h', 'i', 'j',
'k', 'l', 'm', 'n', 'o',
'p', 'q', 'r', 's', 't',
def assign_players_to_teams(teams, players):
players_on_teams = dict()
randomized_players = random.sample(players, len(players)) # changed to .sample because .shuffle doesn't work on tuples / immutable sequences
for i, team in enumerate(teams):
players_on_teams[team] = randomized_players[i::len(teams)]
my_teams = TeamAssigner.assign_players_to_teams(TEAMS, PLAYERS)
for team, name in my_teams.items():
Of course, this is only what I would write if I was not planning to expand the program in the future. If you have plans to make the program bigger and make it do more things, then some of these decisions might be the wrong ones. For instance, if you wanted the ability to move players from one team to another after the initial split, then you would need a class that did keep an internal state of who is on which team...