Sfdwhf

I decided to implement the round robin algorithm in Python. My code takes a list of teams as input and prints the schedule.

This is my first try to write something by my own after taking some online courses, so I am absolutely sure that this code must be significantly improved.

Here it is:

import random



def simulate_draw(teams):

    if len(teams) % 2 == 0:

        simulate_even_draw(teams)

    else:

        simulate_odd_draw(teams)



def simulate_even_draw(teams):

    dic = {}

    for i in range(len(teams)):

        dic[i] = teams[i]



    games = []

    arr1 = [i+1 for i in range(int(len(teams)/2))]

    arr2 = [i+1 for i in range(int(len(teams)/2), len(teams))][::-1]



    for i in range(len(teams)-1):

        arr1.insert(1, arr2[0])

        arr2.append(arr1[-1])

        arr2.remove(arr2[0])

        arr1.remove(arr1[-1])

        zipped = list(zip(arr1, arr2))

        games.append(zipped)

        zipped = [] 



    for game in games:

        for gm in list(game):

            r = random.sample(gm, len(gm))

            print(dic[r[0]-1] + ' plays ' + dic[r[1]-1])



def simulate_odd_draw(teams):

    dic = {}

    for i in range(len(teams)):

        dic[i] = teams[i]

    dic[i+1] = ''

    games = []

    arr1 = [i+1 for i in range(int((len(teams)+1)/2))]

    arr2 = [i+1 for i in range(int((len(teams)+1)/2), len(teams)+1)][::-1]

    for i in range(len(teams)):

        arr1.insert(1, arr2[0])

        arr2.append(arr1[-1])

        arr2.remove(arr2[0])

        arr1.remove(arr1[-1])

        zipped = list(zip(arr1, arr2))

        games.append(zipped)

        zipped = [] 

    for game in games:

        for gm in list(game):

            r = random.sample(gm, len(gm))

            if len(teams)+1 not in r:

                print(dic[r[0]-1] + ' plays ' + dic[r[1]-1])

I think that big blocks of code that largely repeat themselves inside 2 functions may be united in one function, but not sure how to implement it.

Making the code testable and tested

The first step to improve your code is to try to make it testable. By doing so, you usually have to deal with Separation of Concerns: in your case, you have to split the logic doing the output from the logic computing games. The easiest way to do so it to rewrite slightly the simulate_XXX functions to return values instead of writing them.

Once it it done, you can easily write tests for the function computing the games (in order to make this easier to implement, I've extracted out the randomising part as well).

At this stage, we have something like:

import random



def simulate_draw(teams):

    """Return the list of games."""

    if len(teams) % 2 == 0:

        return simulate_even_draw(teams)

    else:

        return simulate_odd_draw(teams)



def simulate_even_draw(teams):

    """Return the list of games."""

    matches = []

    dic = {}

    for i in range(len(teams)):

        dic[i] = teams[i]



    games = []

    arr1 = [i+1 for i in range(int(len(teams)/2))]

    arr2 = [i+1 for i in range(int(len(teams)/2), len(teams))][::-1]



    for i in range(len(teams)-1):

        arr1.insert(1, arr2[0])

        arr2.append(arr1[-1])

        arr2.remove(arr2[0])

        arr1.remove(arr1[-1])

        zipped = list(zip(arr1, arr2))

        games.append(zipped)

        zipped = [] 



    for game in games:

        for gm in list(game):

            r = gm # remove randomness for now - random.sample(gm, len(gm))

            a, b = dic[r[0]-1], dic[r[1]-1]

            matches.append((a, b))

            # print(a + ' plays ' + b)

    return matches



def simulate_odd_draw(teams):

    """Return the list of games."""

    matches = []

    dic = {}

    for i in range(len(teams)):

        dic[i] = teams[i]

    dic[i+1] = ''

    games = []

    arr1 = [i+1 for i in range(int((len(teams)+1)/2))]

    arr2 = [i+1 for i in range(int((len(teams)+1)/2), len(teams)+1)][::-1]

    for i in range(len(teams)):

        arr1.insert(1, arr2[0])

        arr2.append(arr1[-1])

        arr2.remove(arr2[0])

        arr1.remove(arr1[-1])

        zipped = list(zip(arr1, arr2))

        games.append(zipped)

        zipped = [] 

    for game in games:

        for gm in list(game):

            r = gm # remove randomness for now - random.sample(gm, len(gm))

            if len(teams)+1 not in r:

                a, b = dic[r[0]-1], dic[r[1]-1]

                matches.append((a, b))

                # print(a + ' plays ' + b)

    return matches





def displays_simulated_draws(teams):

    """Print the list of games."""

    for gm in simulate_draw(teams):

        a, b = random.sample(gm, len(gm))

        print(a + ' plays ' + b)





def test_simulate_draw():

    """Small tests for simulate_draw."""

    # TODO: Use a proper testing framework

    TESTS = [

        ([], []),

        (['A'], []),

        (['A', 'B', 'C', 'D'], [('A', 'C'), ('D', 'B'), ('A', 'B'), ('C', 'D'), ('A', 'D'), ('B', 'C')]),

        (['A', 'B', 'C', 'D', 'E'], [('A', 'E'), ('B', 'C'), ('A', 'D'), ('E', 'C'), ('A', 'C'), ('D', 'B'), ('A', 'B'), ('D', 'E'), ('B', 'E'), ('C', 'D')]),

    ]

    for teams, expected_out in TESTS:

        # print(teams)

        ret = simulate_draw(teams)

        assert ret == expected_out



if __name__ == '__main__':

    test_simulate_draw()

    displays_simulated_draws(['A', 'B', 'C', 'D'])

Now we can start improving the code in a safer way.

Remove what's not required

dic[i+1] = '' is not required, we can remove it.

Also, resetting zipped to the empty string is not required, we can remove it. Maybe we could get rid of zipped altogether.

Finally, we call for gm in list(game) when game is already a list. We can remove the call to list.

Loop like a native

I highly recommend Ned Batchelder's talk "Loop like a native" about iterators. One of the most simple take away is that whenever you're doing range(len(iterable)), you can probably do things in a better way: more concise, clearer and more efficient.

In your case, we could have:

for i in range(len(teams)):

    dic[i] = teams[i]

replaced by

for i, team in enumerate(teams):

    dic[i] = team

And we could do:

for _ in teams:

instead of

for i in range(len(teams))

(Unfortunately, this can hardly be adapted to the "even" situation)

Note: "_" is a usual variable names for values one does not plan to use.

Dict comprehension

The dictionnary initiation you perform via dict[index] = value in a loop could be done using the Dictionnary Comprehension syntactic sugar.

Instead of:

dic = {}

for i, team in enumerate(teams):

    dic[i] = team

we you can write:

dic = {i: team for i, team in enumerate(teams)}

Now it is much more obvious, it also corresponds to:

dic = dict(enumerate(teams))

Finally, we can ask ourselves how we use this dictionnary: the answer is "to get the team at a given index". Do we really need a dictionnay for this ? I do not think so. We can get rid of the dic variable and use teams directly.

At this stage, we have:

import random



def simulate_draw(teams):

    """Return the list of games."""

    if len(teams) % 2 == 0:

        return simulate_even_draw(teams)

    else:

        return simulate_odd_draw(teams)



def simulate_even_draw(teams):

    """Return the list of games."""

    matches = []

    games = []

    half_len = int(len(teams)/2)

    arr1 = [i+1 for i in range(half_len)]

    arr2 = [i+1 for i in range(half_len, len(teams))][::-1]

    for i in range(len(teams)-1):

        arr1.insert(1, arr2[0])

        arr2.append(arr1[-1])

        arr2.remove(arr2[0])

        arr1.remove(arr1[-1])

        games.append(list(zip(arr1, arr2)))

    for game in games:

        for gm in game:

            r = gm # remove randomness for now - random.sample(gm, len(gm))

            a, b = teams[r[0]-1], teams[r[1]-1]

            matches.append((a, b))

            # print(a + ' plays ' + b)

    return matches



def simulate_odd_draw(teams):

    """Return the list of games."""

    matches = []

    games = []

    half_len = int((len(teams)+1)/2)

    arr1 = [i+1 for i in range(half_len)]

    arr2 = [i+1 for i in range(half_len, len(teams)+1)][::-1]

    for i in range(len(teams)):

        arr1.insert(1, arr2[0])

        arr2.append(arr1[-1])

        arr2.remove(arr2[0])

        arr1.remove(arr1[-1])

        games.append(list(zip(arr1, arr2)))

    for game in games:

        for gm in game:

            r = gm # remove randomness for now - random.sample(gm, len(gm))

            if len(teams)+1 not in r:

                a, b = teams[r[0]-1], teams[r[1]-1]

                matches.append((a, b))

                # print(a + ' plays ' + b)

    return matches





def displays_simulated_draws(teams):

    """Print the list of games."""

    for gm in simulate_draw(teams):

        a, b = random.sample(gm, len(gm))

        print(a + ' plays ' + b)





def test_simulate_draw():

    """Small tests for simulate_draw."""

    # TODO: Use a proper testing framework

    TESTS = [

        ([], []),

        (['A'], []),

        (['A', 'B', 'C', 'D'], [('A', 'C'), ('D', 'B'), ('A', 'B'), ('C', 'D'), ('A', 'D'), ('B', 'C')]),

        (['A', 'B', 'C', 'D', 'E'], [('A', 'E'), ('B', 'C'), ('A', 'D'), ('E', 'C'), ('A', 'C'), ('D', 'B'), ('A', 'B'), ('D', 'E'), ('B', 'E'), ('C', 'D')]),

    ]

    for teams, expected_out in TESTS:

        # print(teams)

        ret = simulate_draw(teams)

        assert ret == expected_out



if __name__ == '__main__':

    test_simulate_draw()

    displays_simulated_draws(['A', 'B', 'C', 'D'])

The right tool for the task

The part:

    arr2.remove(arr2[0])

    arr1.remove(arr1[-1])

could/should probably be written with pop:

    arr2.pop(0)

    arr1.pop()

And now, these line can be merged with arrXX.append(arrYYY[ZZ]):

for i in range(len(teams)-1):

    arr1.insert(1, arr2.pop(0))

    arr2.append(arr1.pop())

    games.append(list(zip(arr1, arr2)))

Removing useless steps

A loop is used to fill an array. Another one is used to iterate over the array. We could try to use a single loop to do everything (disclaimer: this is not always a good idea as far as readability goes).

This removes the need for a few calls to list.

At this stage, we have:

def simulate_even_draw(teams):

    """Return the list of games."""

    matches = []

    half_len = int(len(teams)/2)

    arr1 = [i+1 for i in range(half_len)]

    arr2 = [i+1 for i in range(half_len, len(teams))][::-1]

    for i in range(len(teams)-1):

        arr1.insert(1, arr2.pop(0))

        arr2.append(arr1.pop())

        for gm in zip(arr1, arr2):

            matches.append((teams[gm[0]-1], teams[gm[1]-1]))

    return matches



def simulate_odd_draw(teams):

    """Return the list of games."""

    matches = []

    half_len = int((len(teams)+1)/2)

    arr1 = [i+1 for i in range(half_len)]

    arr2 = [i+1 for i in range(half_len, len(teams)+1)][::-1]

    for i in range(len(teams)):

        arr1.insert(1, arr2.pop(0))

        arr2.append(arr1.pop())

        for gm in zip(arr1, arr2):

            if len(teams)+1 not in gm:

                matches.append((teams[gm[0]-1], teams[gm[1]-1]))

    return matches

Better indices

You generate a list of indices using i+1 and then use val - 1 when you use them. You can make your life easier twice.

Iterable unpacking

Instead of using indices to get elements from an iterable with a know number of elements, you can use iterable unpacking.

You'd get

def simulate_even_draw(teams):

    """Return the list of games."""

    half_len = int(len(teams)/2)

    arr1 = [i for i in range(half_len)]

    arr2 = [i for i in range(half_len, len(teams))][::-1]

    matches = []

    for i in range(len(teams)-1):

        arr1.insert(1, arr2.pop(0))

        arr2.append(arr1.pop())

        for a, b in zip(arr1, arr2):

            matches.append((teams[a], teams[b]))

    return matches



def simulate_odd_draw(teams):

    """Return the list of games."""

    half_len = int((len(teams)+1)/2)

    arr1 = [i for i in range(half_len)]

    arr2 = [i for i in range(half_len, len(teams)+1)][::-1]

    matches = []

    for i in range(len(teams)):

        arr1.insert(1, arr2.pop(0))

        arr2.append(arr1.pop())

        for a, b in zip(arr1, arr2):

            if len(teams) not in (a, b):

                matches.append((teams[a], teams[b]))

    return matches

True divisions

Instead of using "/" and convert the float result to int, you can use "//" which is an integer division.

Other way to compute indices

We could write something like:

indices = list(range(len(teams)))

half_len = len(indices)//2

arr1 = indices[:half_len]

arr2 = indices[:half_len-1:-1]

and

indices = list(range(len(teams)+1))

half_len = len(indices)//2

arr1 = indices[:half_len]

arr2 = indices[:half_len-1:-1]

Altough, if we don't care about order, we could use the more direct:

arr1 = indices[:half_len]

arr2 = indices[half_len:]

Remove the duplicated logic

Don't repeat yourself is a principle of software development that you could easily apply here. Indeed, we have 2 functions that look very similar.

This is trickier than expected and I have to go. I may continue another day.

Batteries included

The Python standard library contains many useful things. Among them, we have the very interesting module itertools which itself contains combinations which is what you want.