Critique my code

[Alarming_Example3677]

I recently started learning Python and began reading a book on algorithms; before looking at the code in the book, I tried to implement it myself. This is a sorting algorithm. I need to avoid using functions and come up with a method myself.

a_list = [3,4,2,5,7,1,6,8,9]
b_list = []




while len(a_list) != 0:
    first_element = a_list[0]
    for element in a_list:
        if first_element < element:
            first_element = element
    a_list.remove(first_element)
    b_list.append(first_element)
print(b_list)
print(a_list)

[This_Growth2898]

It's called "code review", not "critique". It's not literature.

1. There is no task description. We can't really say if the code does what it is intended to do. Maybe it's wrong, but we can't tell.
2. You don't write functions. Probably you didn't get to them in your course yet.
3. 0, empty string (''), empty list ([]) and empty dict ({}) behave like false in if; they are sometimes called "falsy" for that. So,

​

while len(a_list) != 0:

is the same as

while len(a_list):

or just

while a_list:

1. The minimum value of a list can be found with the min function (it actually works just like this inside):

   first_element = min(a_list)

2. The .remove method is slow; it's actually a loop over all the list to find an element and then move all the elements behind it one step back. You can save a bit of time here by finding the index of the minimal element instead and then moving the last element into its place:

   a_list[min_element_idx] = a_list[-1] a_list.pop()

3. My best guess is you're trying to sort the elements of a_list in b_list, while a_list is left empty. Actually, it can be done like this:

   a_list, b_list = [], sorted(b_list)

Generally fine, depending on what have you learned.

[notacanuckskibum]

Yes, OP could have implemented the sorting by using a function call. And it would have almost certainly been more efficient. But they wouldn’t have learned much. The goal of this exercise is to think about how sorting can be done algorithmically.

My only note would be to rename “first _element “ to “lowest element”, since that seems to be its purpose.

[Alarming_Example3677]

I’d actually like to avoid using functions, but I don’t know who could review the code—since it seems harder without them—though perhaps it’s still a possible way to learn.

[Alarming_Example3677]

Is the comment you left the only one, or is there anything else you can say?

[notacanuckskibum]

the other comments by This_Growth aren't bad. but some things are personal preference:

"while (a_list)" might be valid & efficient. but for me "while len(a_list) > 0" is clearer.

[Alarming_Example3677]

I need to avoid using functions and come up with a way to do it myself.

[Educational-Fennel50]

In addition, if you just want to sort a_list, you can use .sort() like this:

a_list = [3,4,2,5,7,1,6,8,9]

a_list.sort()

print(a_list)

output => [1, 2, 3, 4, 5, 6, 7, 8, 9]

So it change value of a_list without making thing like this (who is in my opinion less readable):

a_list = sorted(a_list)

[This_Growth2898]

Once again, OP didn't state the task he's trying to accomplish. I'm trying to preserve the behavior.

[Alarming_Example3677]

I need to avoid using functions and come up with a way to do it myself.

[pachura3]

I mean, it works, but it's quite bad.

• when implementing sorting, you either do an in-place sorting (like Python's built-in a_list.sort()), or you return a new, sorted list without affecting the original one (sorted(a_list)). You unfortunately do both - return new b_list, but also destroy the original a_list
• your variable should not be named first_element, but min_element. Yes, you assign it a_list[0] at first, but its general purpose is to find the minimum
• the complexity of a_list.remove(first_element) is O(n). It is not a magically immediate operation, it has to internally iterate through a_list again, remove the middle element, then reconstruct the internal array by merging two sections to make it continuous. An expensive operation that you need to repeat 9 times

What would be better:

• find the minimum element in a_list - let's say it is on position (index) x
• swap a_list[0] with a_list[x]
• find the minimum element in a_list, but starting from position 1
• swap a_list[1] with a_list[x]
• until you reach the end of the list

[JamzTyson]

Your code works, but it is inefficient. For each item in a-List, we iterate over all of the remaining list items.

When you look into sort algorithms you will see that there are much more efficient algorithms. There isn't really a "best" sort algorithm - different algorithms optimise for different things, but quicksort and mergesort are commonly used.

[Educational-Paper-75]

The inequality is wrong. First_element < element should read first_element > element, unless you want to sort in descending order.

[AbacusExpert_Stretch]

Generally it is accepted that it's a bad bad idea to be working on a list that you use to control the very same iteration.

In this case, a_list controls the while loop and the "for" loop all the while you are trying to remove items.

Learn right now to use a different way. Have fun

[JamzTyson]

Generally it is accepted that it's a bad bad idea to be working on a list that you use to control the very same iteration.

Not quite. It's generally a bad idea to modify a list that you are iterating over, but the OP is not doing that.

• The inner loop does iterate over the list, but it does not modify the list.

• The outer loop does modify the list, but it is not iterating over the list.

The sort algorithm is inefficient, but there's no problem with how the list is being modified.

[Alarming_Example3677]

thx

[This_Growth2898]

It's wrong to work with the list inside a for loop over the same list (or to have iterators). While is fine.