Simulate round-robin tournament drawFast draw wxPython OnPaintRuby program to simulate a game of Narcotic SolitaireSimulate object.__getattribute__ in PythonPython list dictionary items round robin mixingDraw a directory treeUEFA Champions League Draw SimulatorCoding exercise to simulate animal populationTournament Management modelAxelrod TournamentIteratively simulate a parasite population
Repelling Blast: Must targets always be pushed back?
Noun clause (singular all the time?)
Why does processed meat contain preservatives, while canned fish needs not?
Examples of subgroups where it's nontrivial to show closure under multiplication?
Who is the Umpire in this picture?
Combinable filters
What route did the Hindenburg take when traveling from Germany to the U.S.?
Why was the Spitfire's elliptical wing almost uncopied by other aircraft of World War 2?
Do I have to worry about players making “bad” choices on level up?
Is there really no use for MD5 anymore?
What do the phrase "Reeyan's seacrest" and the word "fraggle" mean in a sketch?
How do I reattach a shelf to the wall when it ripped out of the wall?
Why is it that the natural deduction method can't test for invalidity?
A Note on N!
Is there a way to get a compiler for the original B programming language?
What does the "ep" capability mean?
Phrase for the opposite of "foolproof"
What is the most expensive material in the world that could be used to create Pun-Pun's lute?
Meaning of Bloch representation
Mac Pro install disk keeps ejecting itself
Do I have an "anti-research" personality?
How to make a pipeline wait for end-of-file or stop after an error?
Realistic Necromancy?
Exchange,swap or switch
Simulate round-robin tournament draw
Fast draw wxPython OnPaintRuby program to simulate a game of Narcotic SolitaireSimulate object.__getattribute__ in PythonPython list dictionary items round robin mixingDraw a directory treeUEFA Champions League Draw SimulatorCoding exercise to simulate animal populationTournament Management modelAxelrod TournamentIteratively simulate a parasite population
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty,.everyoneloves__bot-mid-leaderboard:empty margin-bottom:0;
$begingroup$
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.
python simulation
New contributor
$endgroup$
add a comment |
$begingroup$
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.
python simulation
New contributor
$endgroup$
$begingroup$
I think your code is missing an entry point (if __name__ == "__main__":
) because it shouldn't run in it's current format. Can you fix? Verify by copying then pasting into a new file and running that.
$endgroup$
– C. Harley
Apr 23 at 15:18
$begingroup$
@C.Harley This seems to be a lib, as such, I don't see the necessity for an entry point. You can callsimulate_draw
with a list of names of your liking if you want to test it.
$endgroup$
– Mathias Ettinger
Apr 23 at 16:40
$begingroup$
@Mathias Ettinger - aren't we here to help improve fellow developers? Without any tests or data nor a clear entry point even if it was library, this wouldn't fly within my team.
$endgroup$
– C. Harley
Apr 24 at 5:41
$begingroup$
@C.Harley Well then write an answer pointing to the lack of tests, no need to require an entry point for the question when it's not required.
$endgroup$
– Mathias Ettinger
Apr 24 at 6:02
add a comment |
$begingroup$
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.
python simulation
New contributor
$endgroup$
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.
python simulation
python simulation
New contributor
New contributor
edited Apr 23 at 15:23
Eldar
New contributor
asked Apr 23 at 15:13
EldarEldar
384
384
New contributor
New contributor
$begingroup$
I think your code is missing an entry point (if __name__ == "__main__":
) because it shouldn't run in it's current format. Can you fix? Verify by copying then pasting into a new file and running that.
$endgroup$
– C. Harley
Apr 23 at 15:18
$begingroup$
@C.Harley This seems to be a lib, as such, I don't see the necessity for an entry point. You can callsimulate_draw
with a list of names of your liking if you want to test it.
$endgroup$
– Mathias Ettinger
Apr 23 at 16:40
$begingroup$
@Mathias Ettinger - aren't we here to help improve fellow developers? Without any tests or data nor a clear entry point even if it was library, this wouldn't fly within my team.
$endgroup$
– C. Harley
Apr 24 at 5:41
$begingroup$
@C.Harley Well then write an answer pointing to the lack of tests, no need to require an entry point for the question when it's not required.
$endgroup$
– Mathias Ettinger
Apr 24 at 6:02
add a comment |
$begingroup$
I think your code is missing an entry point (if __name__ == "__main__":
) because it shouldn't run in it's current format. Can you fix? Verify by copying then pasting into a new file and running that.
$endgroup$
– C. Harley
Apr 23 at 15:18
$begingroup$
@C.Harley This seems to be a lib, as such, I don't see the necessity for an entry point. You can callsimulate_draw
with a list of names of your liking if you want to test it.
$endgroup$
– Mathias Ettinger
Apr 23 at 16:40
$begingroup$
@Mathias Ettinger - aren't we here to help improve fellow developers? Without any tests or data nor a clear entry point even if it was library, this wouldn't fly within my team.
$endgroup$
– C. Harley
Apr 24 at 5:41
$begingroup$
@C.Harley Well then write an answer pointing to the lack of tests, no need to require an entry point for the question when it's not required.
$endgroup$
– Mathias Ettinger
Apr 24 at 6:02
$begingroup$
I think your code is missing an entry point (
if __name__ == "__main__":
) because it shouldn't run in it's current format. Can you fix? Verify by copying then pasting into a new file and running that.$endgroup$
– C. Harley
Apr 23 at 15:18
$begingroup$
I think your code is missing an entry point (
if __name__ == "__main__":
) because it shouldn't run in it's current format. Can you fix? Verify by copying then pasting into a new file and running that.$endgroup$
– C. Harley
Apr 23 at 15:18
$begingroup$
@C.Harley This seems to be a lib, as such, I don't see the necessity for an entry point. You can call
simulate_draw
with a list of names of your liking if you want to test it.$endgroup$
– Mathias Ettinger
Apr 23 at 16:40
$begingroup$
@C.Harley This seems to be a lib, as such, I don't see the necessity for an entry point. You can call
simulate_draw
with a list of names of your liking if you want to test it.$endgroup$
– Mathias Ettinger
Apr 23 at 16:40
$begingroup$
@Mathias Ettinger - aren't we here to help improve fellow developers? Without any tests or data nor a clear entry point even if it was library, this wouldn't fly within my team.
$endgroup$
– C. Harley
Apr 24 at 5:41
$begingroup$
@Mathias Ettinger - aren't we here to help improve fellow developers? Without any tests or data nor a clear entry point even if it was library, this wouldn't fly within my team.
$endgroup$
– C. Harley
Apr 24 at 5:41
$begingroup$
@C.Harley Well then write an answer pointing to the lack of tests, no need to require an entry point for the question when it's not required.
$endgroup$
– Mathias Ettinger
Apr 24 at 6:02
$begingroup$
@C.Harley Well then write an answer pointing to the lack of tests, no need to require an entry point for the question when it's not required.
$endgroup$
– Mathias Ettinger
Apr 24 at 6:02
add a comment |
1 Answer
1
active
oldest
votes
$begingroup$
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.
$endgroup$
add a comment |
Your Answer
StackExchange.ifUsing("editor", function ()
StackExchange.using("externalEditor", function ()
StackExchange.using("snippets", function ()
StackExchange.snippets.init();
);
);
, "code-snippets");
StackExchange.ready(function()
var channelOptions =
tags: "".split(" "),
id: "196"
;
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function()
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled)
StackExchange.using("snippets", function()
createEditor();
);
else
createEditor();
);
function createEditor()
StackExchange.prepareEditor(
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader:
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
,
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
);
);
Eldar is a new contributor. Be nice, and check out our Code of Conduct.
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f217969%2fsimulate-round-robin-tournament-draw%23new-answer', 'question_page');
);
Post as a guest
Required, but never shown
1 Answer
1
active
oldest
votes
1 Answer
1
active
oldest
votes
active
oldest
votes
active
oldest
votes
$begingroup$
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.
$endgroup$
add a comment |
$begingroup$
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.
$endgroup$
add a comment |
$begingroup$
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.
$endgroup$
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.
edited Apr 23 at 20:23
answered Apr 23 at 17:03
JosayJosay
26.3k14087
26.3k14087
add a comment |
add a comment |
Eldar is a new contributor. Be nice, and check out our Code of Conduct.
Eldar is a new contributor. Be nice, and check out our Code of Conduct.
Eldar is a new contributor. Be nice, and check out our Code of Conduct.
Eldar is a new contributor. Be nice, and check out our Code of Conduct.
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f217969%2fsimulate-round-robin-tournament-draw%23new-answer', 'question_page');
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
$begingroup$
I think your code is missing an entry point (
if __name__ == "__main__":
) because it shouldn't run in it's current format. Can you fix? Verify by copying then pasting into a new file and running that.$endgroup$
– C. Harley
Apr 23 at 15:18
$begingroup$
@C.Harley This seems to be a lib, as such, I don't see the necessity for an entry point. You can call
simulate_draw
with a list of names of your liking if you want to test it.$endgroup$
– Mathias Ettinger
Apr 23 at 16:40
$begingroup$
@Mathias Ettinger - aren't we here to help improve fellow developers? Without any tests or data nor a clear entry point even if it was library, this wouldn't fly within my team.
$endgroup$
– C. Harley
Apr 24 at 5:41
$begingroup$
@C.Harley Well then write an answer pointing to the lack of tests, no need to require an entry point for the question when it's not required.
$endgroup$
– Mathias Ettinger
Apr 24 at 6:02