random team distributer

Question

Any idea how to shorten and improve the program? it's going to take a list included of 22 people and split them into two teams and return the names and teams

import random
# list of teams
teams = ['A', 'B']
# list of players which is inculded of 22 people
players = ['a', 'b', 'c', 'd', 'e',
           'f', 'g', 'h', 'i', 'j',
           'k', 'l', 'm', 'n', 'o',
           'p', 'q', 'r', 's', 't',
           'u', 'v']

# takes the list of names


class Human:
    def __init__(self, name):
        self.name = name

# distribure names and people to teams randomly and it inherits names from Human class


class Player(Human):
    def get_team(self):
        teams = dict()
        random.shuffle(self.name)
        teams['A'] = self.name[0::2]
        teams['B'] = self.name[1::2]
        return teams


my_team = Player(players)
my_team = my_team.get_team()
for team, name in my_team.items():
    print(team, name)

Answer 1

The concept is good, but there is room for improvement. Here are some problems I see:

Misleading names

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 (Humans, names, 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. Since 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 Player object.

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 Human's 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.

Your 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.

Hardcoded assumptions

# 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]
        return teams

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',
           'u', 'v')

class TeamAssigner:
    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)]
        return players_on_teams

my_teams = TeamAssigner.assign_players_to_teams(TEAMS, PLAYERS)
for team, name in my_teams.items():
    print(team, name)

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...

Answer 2

Honestly, your code is pretty compact already. There are really only 2 areas that I think can be shortened.

1. The get_team method of Player.

    def get_team(self):
        random.shuffle(self.name)
        return {'A':self.name[0::2], 'B':self.name[1::2]}

If you want to improve readability, you can set the dictionary to team and then return team.

2. The last couple of lines.

my_team = Player(players).get_team()
for team, name in my_team.items():
    print(team, name)

Answer 3

This problem doesn't really need a class to solve it. Jack Diederich's Stop Writing Classes is very relevant here, and like the examples shown in the talk, Human and Player probably shouldn't be classes in the first place.

All we need is a function that takes in a list of players, and returns the teams:

import random

def get_teams(players):
    randomly_ordered_players = random.sample(players, len(players))
    return {
        "A": randomly_ordered_players[0::2],
        "B": randomly_ordered_players[1::2],
    }

Note that we're using random.sample here which returns a new randomly-ordered list instead of shuffling the list in place. This is cleaner because we don't mutate the input list.

---

We can make the function more flexible by also taking in a list of team names, so we can divide players up into any number of teams. This is @MJ713's solution using a dict comprehension:

import random

def get_teams(teams, players):
    randomly_ordered_players = random.sample(players, len(players))
    number_of_teams = len(teams)
    return {
        teams[i]: randomly_ordered_players[i::number_of_teams]
        for i in range(number_of_teams)
    }

Example:

for team_name, players in get_teams(
    ["A", "B"], ["a", "b", "c", "d", "e", "f", "g", "h"]
).items():
    print(team_name, players)

A ['e', 'f', 'a', 'g']
B ['h', 'b', 'd', 'c']

for team_name, players in get_teams(
    ["A", "B", "C", "D"], ["a", "b", "c", "d", "e", "f", "g", "h"]
).items():
    print(team_name, players)

A ['d', 'b']
B ['e', 'g']
C ['h', 'c']
D ['f', 'a']

Answer 4

This could be a good application for grouper() from Itertools Recipies:

from itertools import zip_longest
from random import shuffle

players = ['a', 'b', 'c', 'd', 'e',
       'f', 'g', 'h', 'i', 'j',
       'k', 'l', 'm', 'n', 'o',
       'p', 'q', 'r', 's', 't',
       'u', 'v']


def grouper(iterable, n, fillvalue=None):
    "Collect data into fixed-length chunks or blocks"
    # grouper('ABCDEFG', 3, 'x') --> ABC DEF Gxx"
    args = [iter(iterable)] * n
    return zip_longest(*args, fillvalue=fillvalue)

shuffle(players)

team_a, team_b = grouper(players, 11, '')