5
\$\begingroup\$

I'm not proficient at Python. My question is twofold: whether this selection sort algorithm implementation for number lists could be considered clean code (in the sense of being modular and maintainable), and how should I go about making it more Pythonic if necessary.

def selection_sort(the_list):
    for sorted_sublist_end_position in range(0, len(the_list) - 1):
        smallest_item_position = find_smallest_item_position(the_list, sorted_sublist_end_position)
        swap(the_list, sorted_sublist_end_position, smallest_item_position)
    return the_list

def find_smallest_item_position(the_list, position_before_unsorted_sublist):
    current_smallest_item_position = position_before_unsorted_sublist
    sublist_start_position = position_before_unsorted_sublist + 1
    for unsorted_sublist_position in range(sublist_start_position, len(the_list)):
        if the_list[unsorted_sublist_position] < the_list[current_smallest_item_position]:
            current_smallest_item_position = unsorted_sublist_position
    return current_smallest_item_position

def swap(a_list, i, j):
    aux = a_list[i]
    a_list[i] = a_list[j]
    a_list[j] = aux

print(selection_sort([2, 5, 4]))
print(selection_sort([5, 4, 2]))
print(selection_sort([]))

Potential issues I can think of:

  • Variable names are very long (that would come from my Java background).

  • I guess I could pass a single sublist to the second function, as the sublist start position is implicit, instead of two arguments. Would that make the code more Pythonic?

  • Clean Code says functions should do one thing / be responsible by one thing. So I expect a function for instance to perform a single for. The second function performs both a for and a nested if, however if I extract a sub-function out of it the sub-function name will be just a rephrasing of what the sub-function does, which is not good. Should I go about doing that or is the function good enough as it is?

\$\endgroup\$
2
  • \$\begingroup\$ (While Python does not stand in the way of literal programming, I don't think this is it.) \$\endgroup\$
    – greybeard
    Jan 7 at 16:17
  • \$\begingroup\$ I didn't mean to implement it as literal programming, in the sense that code would be read like sentences, just tried to give out meaningful names which help to understand what the algorithm is doing. But maybe I went a bit overboard. I've made a couple of improvements in the naming of variables. \$\endgroup\$
    – Piovezan
    Jan 7 at 19:20

1 Answer 1

9
\$\begingroup\$

whether this selection sort... could be considered clean code (in the sense of being modular and maintainable)

Sorting (especially using a trivial algorithm like selection sort) is a simple thing to implement, so assuming you do it right, you will not need to maintain it in the future, like ever.

As for modularity I cannot give any definite answer since there isn't much to work with. There're only 3 functions, and it is unknown how or why you might be using them in the future. It is very well possible that you will not really need those helper functions at all.

Potential issues I can think of: Variable names are very long

That is true. Your code is very hard to read because the variable names are way too expressive. For example:

  • The loop variables could be replaced with i without losing clarity since it's obvious that you're iterating over array indices
  • current_smallest_item_position explains its purpose in too much detail and could be replaced current_min (it's understood that this is an index from the function itself)
  • sublist_start_position doesn't explain its meaning well and could be replaced with search_from

Here's an example of how I'd rewrite your code changing the variable naming only:

def selection_sort(lst):
    for i in range(len(lst) - 1):
        j = find_smallest_item_position(lst, i)
        swap(lst, i, j)
    return lst

def find_smallest_item_position(lst, search_after):
    current_min = search_after
    search_from = search_after + 1
    for i in range(search_from, len(lst)):
        if lst[i] < lst[current_min]:
            current_min = i
    return current_min

(Notice how I replaced range(0, len(lst) - 1) with range(len(lst) - 1). That is because a starting index of 0 is implied when range receives a single argument.)

Looks much more clear now, doesn't it? The find_smallest_item_position function could be improved further by getting rid of current_min = search_after thing and inlining the search_from calculation:

def find_smallest_item_position(lst, current_min):
    for i in range(current_min + 1, len(lst)):
        if lst[i] < lst[current_min]:
            current_min = i
    return current_min

I guess I could pass a single sublist to the second function, as the sublist start position is implicit, instead of two arguments. Would that make the code more Pythonic?

Are you thinking of doing this instead?

def selection_sort(lst):
    for i in range(len(lst) - 1):
        j = find_smallest_item_position(lst, i)       # old version
        j = find_smallest_item_position(lst[i:]) + i  # new version
        swap(lst, i, j)
    return lst

That would be a terrible idea. Passing a searched-list and a starting-search-index is a normal thing to do. This version of the code, on the other hand, is not better in terms of readability but is worse in terms of performance since you have to make a (partial) copy of the list on every loop iteration.

Clean Code says functions should do one thing / be responsible by one thing

While that is true in general, I think you're too strict about this. If you start moving every single statement into a separate function, it'll only be detrimental for your code. IMO, you should only split big functions into smaller functions when you see that it becomes too complicated to reason about or when you realize that some logic could genuinely be reused in multiple places.

Your swap function is 100% a poor application of this principle. Notice how I did not include it my rewrite of your code - that is because nobody does that in Python. Instead, you can simply write:

lst[i], lst[j] = lst[j], lst[i]

No function needed at all.

Assuming find_smallest_item_position will not be reused elsewhere, I'd say there's no reason to make it a separate function too. And in case you think that your code would look ugly, with enough Python profficiency you could express it in a much more concise manner:

def find_smallest_item_position(lst, current_min):
    return min(range(current_min, len(lst)), key=lambda x: lst[x])

Which leads to a very clean selection_sort implementation:

def selection_sort(lst):
    for i in range(len(lst) - 1):
        j = min(range(i, len(lst)), key=lambda x: lst[x])
        lst[i], lst[j] = lst[j], lst[i]
    return lst
\$\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.