Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link
  1. This function returns a new list, like the built-in sorted. It does not update data in place, like the sort method on lists. So a name containing "sorted" would give a better hint at its behaviour.

  2. The docstring is not detailed enough to explain how to call the function. What arguments do I pass for group_key and sort_key? What happens if I pass True for group_reverse or sort_reverse?

  3. The function calls operator.itemgetter to make key functions. But this means that the function will only work when the keys are dictionary or sequence values. If you want to use object properties as keys, you're out of luck (that would need operator.attrgetter). And if you want to compute something more complicated by passing in your own function, that's no good either.

    I would recommend accepting any function for group_key and sort_key and letting the caller use operator.itemgetter if that's what they need. This makes the function behave more like the built-in sorted and similar functions that take key arguments.

I would recommend accepting any function for group_key and sort_key and letting the caller use operator.itemgetter if that's what they need. This makes the function behave more like the built-in sorted and similar functions that take key arguments.

  1. Extend a list using the extend method, which updates the list in-place. If you use augmented assignment +=, Python may have to create a new list and copy all the elements across.

    Extend a list using the extend method, which updates the list in-place. If you use augmented assignment +=, Python may have to create a new list and copy all the elements across.

  1. This function returns a new list, like the built-in sorted. It does not update data in place, like the sort method on lists. So a name containing "sorted" would give a better hint at its behaviour.

  2. The docstring is not detailed enough to explain how to call the function. What arguments do I pass for group_key and sort_key? What happens if I pass True for group_reverse or sort_reverse?

  3. The function calls operator.itemgetter to make key functions. But this means that the function will only work when the keys are dictionary or sequence values. If you want to use object properties as keys, you're out of luck (that would need operator.attrgetter). And if you want to compute something more complicated by passing in your own function, that's no good either.

I would recommend accepting any function for group_key and sort_key and letting the caller use operator.itemgetter if that's what they need. This makes the function behave more like the built-in sorted and similar functions that take key arguments.

  1. Extend a list using the extend method, which updates the list in-place. If you use augmented assignment +=, Python may have to create a new list and copy all the elements across.
  1. This function returns a new list, like the built-in sorted. It does not update data in place, like the sort method on lists. So a name containing "sorted" would give a better hint at its behaviour.

  2. The docstring is not detailed enough to explain how to call the function. What arguments do I pass for group_key and sort_key? What happens if I pass True for group_reverse or sort_reverse?

  3. The function calls operator.itemgetter to make key functions. But this means that the function will only work when the keys are dictionary or sequence values. If you want to use object properties as keys, you're out of luck (that would need operator.attrgetter). And if you want to compute something more complicated by passing in your own function, that's no good either.

    I would recommend accepting any function for group_key and sort_key and letting the caller use operator.itemgetter if that's what they need. This makes the function behave more like the built-in sorted and similar functions that take key arguments.

  4. Extend a list using the extend method, which updates the list in-place. If you use augmented assignment +=, Python may have to create a new list and copy all the elements across.

Source Link
Gareth Rees
  • 50.1k
  • 3
  • 130
  • 210

1. Quick review

  1. This function returns a new list, like the built-in sorted. It does not update data in place, like the sort method on lists. So a name containing "sorted" would give a better hint at its behaviour.

  2. The docstring is not detailed enough to explain how to call the function. What arguments do I pass for group_key and sort_key? What happens if I pass True for group_reverse or sort_reverse?

  3. The function calls operator.itemgetter to make key functions. But this means that the function will only work when the keys are dictionary or sequence values. If you want to use object properties as keys, you're out of luck (that would need operator.attrgetter). And if you want to compute something more complicated by passing in your own function, that's no good either.

I would recommend accepting any function for group_key and sort_key and letting the caller use operator.itemgetter if that's what they need. This makes the function behave more like the built-in sorted and similar functions that take key arguments.

  1. Extend a list using the extend method, which updates the list in-place. If you use augmented assignment +=, Python may have to create a new list and copy all the elements across.

2. Rewrite

The algorithm you've chosen is:

  1. Sort data by the group key.

  2. Iterate over the data, collecting the groups.

  3. Iterate over the groups, sorting each one.

Steps 2 and 3 can be combined into one iteration using itertools.groupby. The alternation between ascending and descending can be implemented using itertools.cycle. These two iterations can be combined using zip, like this:

from itertools import cycle, groupby
def sorted_alternately(data, group_key, sort_key,
 group_reverse=False, sort_reverse=False):
 """Sort data on group_key (in reverse order if group_reverse=True) but
 within each group of items, alternately sort in ascending and
 descending order on sort_key (in descending and ascending order in
 sort_reverse=True).
 >>> from itertools import product
 >>> from operator import itemgetter as get
 >>> sorted_alternately(product(range(3), repeat=2), get(0), get(1))
 [(0, 0), (0, 1), (0, 2), (1, 2), (1, 1), (1, 0), (2, 0), (2, 1), (2, 2)]
 """
 data = sorted(data, key=group_key, reverse=group_reverse)
 groups = groupby(data, key=group_key)
 reverses = cycle((sort_reverse, not sort_reverse))
 result = []
 for (_, group), reverse in zip(groups, reverses):
 result.extend(sorted(group, key=sort_key, reverse=reverse))
 return result
lang-py

AltStyle によって変換されたページ (->オリジナル) /