1
\$\begingroup\$

I have a list as follows and I want to shuffle the elements that have the same frequency while keeping the elements with different frequencies in the correct positions. The code below works; however, it is not that nice. I was wondering if anyone had some suggestions e.g. using list comprehension.

pieces_needed = [('a',1),('b',1),('d',1),('c',2),('g',3),('f',5),('z',5)]
temp = []
new_pieces_needed = []
current_freq = 1
for i in pieces_needed:
   if i[1] == current_freq:
      temp.append(i)
   else:
      random.shuffle(temp)
      new_pieces_needed +=  temp
      temp = []
      temp.append(i)
      current_freq = i[1]

random.shuffle(temp)       
new_pieces_needed +=  temp

print(new_pieces_needed)

Example output:

[('d',1),('b',1),('a',1),('c',2),('g',3),('z',5),('f',5)]
\$\endgroup\$
2
\$\begingroup\$
  1. Both branches of the if statement have the line:

    temp.append(i)
    

    This duplication could be avoided.

  2. The variable temp could have a better name, for example group or current_group.

  3. i[1] is evaluated twice.

  4. The variable name i is normally used for an index, but in this case it is an element of the list, so it could have a better name.

  5. These lines appear twice:

    random.shuffle(temp)
    new_pieces_needed +=  temp
    

    This duplication could be avoided by postponing the shuffles until the groups have been found, like this:

    # Group pieces_needed according to their frequency.
    groups = []
    current_freq = object()
    for piece in pieces_needed:
        freq = piece[1]
        if freq != current_freq:
            groups.append([])
            current_freq = freq
        groups[-1].append(piece)
    
    # Randomly shuffle each group.
    for group in groups:
        random.shuffle(group)
    
    # Concatenate the shuffled groups.
    new_pieces_needed = [piece for group in groups for piece in group]
    
  6. There is something arbitrary about the use of "pieces" and "frequency" and piece[1]. The same general algorithm would work for any kind of data, not just pieces. So I think the code would be clearer if we generalized it like this, taking an arbitrary iterable and an optional key function:

    def shuffle_groups(iterable, key=None):
        """Group the iterable (using the key function if supplied), randomly
        shuffle each group, and return the concatenation of the shuffled
        groups.
    
        """
    

    and then we can shuffle the pieces by passing operator.itemgetter(1) for the key argument:

    from operator import itemgetter
    new_pieces_needed = shuffle_groups(pieces_needed, key=itemgetter(1))
    
  7. The grouping logic can be delegated to itertools.groupby and the concatenation to itertools.chain:

    from itertools import chain, groupby
    from random import shuffle
    
    def shuffle_groups(iterable, key=None):
        """Group the iterable (using the key function if supplied), randomly
        shuffle each group, and return the concatenation of the shuffled
        groups.
    
        """
        groups = [list(group) for _, group in groupby(iterable, key)]
        for group in groups:
            shuffle(group)
        return list(chain(*groups))
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.