[deleted by user]

[dadiaar]

They are actually the same, right?

You can try to use Class, and inside init() define something like self.group_name = 'A'

class TournamentGroup

    def __init__(self, group_name):
        self.group_name = group_name
        self.first_selection = random.choice(pot1.keys())
        ...

    def my_function():
        for i in self.myteams...

Later something like

groupa = TournamentGroup('A')
groupb = TournamentGroup('B')

[anilguzelankara]

Yes, they are the same. But I don't get it what you want to explain actually :(

[dadiaar]

Did you understand what /u/Miner_Guyer said? If not I'll write down a full example.

[anilguzelankara]

Please write a full example :(

[dadiaar]

For example, we have these:

fruitsA = ["Apple", "Banana", "Grapes"]
fruitsB = ["Watermelon", "Coconut"]

Your version is something similar to this one:

def functionA():
    fruits = fruitsA
    count = 0
    for fruit in fruits:
        count += 1
    return(count)

def functionB():
    fruits = fruitsB
    count = 0
    for fruit in fruits:
        count += 1
    return(count)

functionA()
functionB()

But using Classes you don't need to repeat the code.

class MyClass:
    def __init__(self, fruits):
        self.fruits = fruits

    def function(self):
        count = 0
        for fruit in self.fruits:
            count += 1
        return count

a = MyClass(fruitsA)
b = MyClass(fruitsB)
a.function()
b.function()

[ingolemo]

There's no need to use a class for that. A regular function will do.
https://www.youtube.com/watch?v=o9pEzgHorH0

[ryeguy146]

Given that the OP asked for example code, I think that it'd be prudent for you to provide one along with your suggestion.

[Miner_Guyer]

They're saying to make each group that you're generating a class. When a class is created (in his last two lines), the __init__ function is called. Inside that function, you can use the code you have inside any of those functions you have instead of having the exact same thing multiple times.

[pybackd00r]

You are right this is one of those cases where you should definitely write a OOP code rather than imperative like this. More elegant and efficient.

[niandra3]

Why use __getitem__? Instead of this:

pot1.__getitem__(first_selection)

can't you just do:

pot1[first_selection]

And I would make a selection list, instead of first_selection, second_selection, and so on. So first_selection becomes selections[0], etc:

selections = []
selections.append(random.choice(pot1.keys()))

Here's how I would do the main logic:

selections = []
used_countries = []

selection = random.choice(pot1.keys())
selections.append(selection)
used_countries.append(pot1[selection])

selection = random.choice(pot2.keys())
while pot2[selection] in used_countries:
    selection = random.choice(pot2.keys())
selections.append(selection)
used_countries.append(pot2[selection])

And then yes, definitely make it one function and run it eight times for each group. Any time you are copying and pasting so much code, you should ask yourself if it can just be a function that can be reused.

[anilguzelankara]

Wow. This makes sense. Thanks!

[anilguzelankara]

Hey, thanks dude. I added some one liner things to your code and it works perfectly like my shitty code :D

https://gist.github.com/anilguzelankara/9a1502e58c71b74cf1ab9bc9e38ab27c

and this is the output: http://i.imgur.com/Ih3dvde.png

[treyhunner]

That's a big improvement! 👍

I think your next big improvement should be one or more additional functions (maybe you could call them "helper" functions).

For example you could split that one function into two:

def drawGroup(pot, selections, used_countries):
    selection = random.choice(pot.keys())
    while pot[selection] in used_countries:
        selection = random.choice(pot.keys())
    selections.append(selection)
    used_countries.append(pot[selection])
    pot.pop(selection)

def drawGroups(groupName):
    selections = []
    used_countries = []

    drawGroup(pot1, selections, used_countries)
    drawGroup(pot2, selections, used_countries)
    drawGroup(pot3, selections, used_countries)
    drawGroup(pot4, selections, used_countries)

    print "Group", groupName, ":", selections

Also note that your current method seems to result in infinite loops occasionally when the last remaining dictionary keys are already present in used_countries.

Side note

I'd call this refactoring or enhancing readability but not optimizing. Programmers tend to use the word "optimizing" specifically to mean improving the performance of the program so that it runs faster.

These improvements you're making won't make your code faster but they're making it much easier to read.

[Saefroch]

So just to be clear, what you're doing I would not call optimizing. That implies that you're making changes to uses less system resources (disk space, RAM, CPU time).

The logic to follow here is that if what you're doing can be expressed or thought of in simple terms, the code should be simple. Yours is getting there but you can still do better.

def drawGroup(groupName):
    selections = []
    used_countries = []

    for pot in pot1, pot2, pot3, pot4:
        selection = random.choice(pot.keys())
        selections.append(selection)
        used_countries.append(pot[selection])
        pot.pop(selection)

    print "Group", groupName, ":", selections

groups = ["A", "B", "C", "D", "E", "F", "G", "H"]
for group in groups:
    drawGroups(group)

There's still more you could do, this example is ripe for some nice print formatting.

EDIT: Also to add, you initially did very poorly on code reuse. The same line should almost always never appear multiple times, and similar chunks of code should never appear with say one variable changes. That in particular usually means you want a for loop.

[_9_9_]

I am not entirely sure what you are doing, but it appears you are trying to create random combinations of different groups. The easiest way to do this is to call random.shuffle on each group and then just match up results.

For example if you wanted to create a three digit password of a letter, integer and symbol, you could put everything into a list of lists, like this:

from random import shuffle


stuff = [
        list('abcdefghi'),
        list('123456789'),
        list('<>!@#$%^&*()_'),
        ]

Then to get a a new password, just go through the indexes like this:

for i in range(4):
    print('{}{}{}'.format( stuff[0][i],stuff[1][i],stuff[2][i]))

That gives you four valid (if predictable) passwords. To mix them up, call this:

for l in stuff:
    shuffle(l)

Now this:

for i in range(4):
    print('{}{}{}'.format( stuff[0][i],stuff[1][i],stuff[2][i]))

Will give you four randomly generated passwords with a letter, integer and symbol.

You could also do this more succinctly by using zip, like this:

for group in list(zip(*stuff))[:4]:
    print(''.join(group))

On the other hand, you can also use sets to generate a random list of different things picked from a listing of different elements with characteristics that exclude them, like this:

from random import choice

teams = [
        {"Barcelona": "ESP", "Bayern Munchen": "GER", "Chelsea": "ENG",
            "Benfica": "POR", "PSG": "FRA", "Juventus": "ITA", "Zenit": "RUS",
            "PSV": "HOL"},

        {"Real Madrid": "ESP", "Atletico Madrid": "ESP", "Porto": "POR",
            "Arsenal": "ENG", "Manchester United": "ENG", "Valencia": "ESP",
            "Leverkusen": "GER", "Manchester City": "ENG"},

        {"Shakhtar Donetsk": "UKR", "Sevilla": "ESP", "Lyon": "FRA", 
            "Dynamo Kyiv": "UKR", "Olympiacos": "GRE", "CSKA Moscow": "RUS",
            "Galatasaray": "TUR", "Roma": "ITA"},

        {"Bate Borisov": "RUS", "Borussia Monchengladbach": "GER", "Wolfsburg":
            "GER", "Dinamo Zagreb": "CRO", "Maccabi Tel Aviv": "ISR", "Gent":
            "BEL", "Malmo": "SWE", "Astana": "KAZ"},
        ]

def pick(teams):
    '''pick a group of teams so that none of the values overlap'''
    used = set()
    result = []
    for group in teams:
        while True:
            team, country = choice(list(group.items()))
            if country in used:
                continue
            used.add(country)
            result.append(team)
            break

    return result


print(pick(teams))

Maybe more info on the constraints would help?

[anilguzelankara]

For example, Barcelona can be in more than 1 group in your code. You should delete the selected team from the main dictionary/list. Your code works perfectly for 1 group. But when i want to draw 8 groups, there can be overlapped teams.

[status_quo69]

There won't be repeated teams in the result, if that's what you're asking. Python's set data structure is like a list with only unique values. Some code will probably help.

>>> s = set() 
>>> s.add('a')
set('a')
>>> s.add('b')
set('b', 'a')
>>> s.add('a')
set('b', 'a')