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
