I'm just looking for constructive criticism of my Ruby implementation of a bubble sort algorithm.
class BubbleSorter < Object def bubble_sort(list) swaps = 0 # parse the list until it is sorted until @sorted == true # run the comparison of adjacent elements for i in 0...(list.length - 1) # if the first is greater than the second, swap them with parallel assignment if list[i] > list[i+1] list[i], list[i+1] = list[i+1], list[i] # increase the number of swaps performed in this run by 1 swaps += 1 end # compare the next 2 elements i += 1 end # uncomment the following line to see each iteration: # p list # If any swaps took place during the last run, the list is not yet sorted if swaps > 0 @sorted = false # no swaps? Everything is in order else @sorted = true end # reset swap count for each run swaps = 0 end endend2 Answers2
Although you should comment your work, some of your comments seem unecessary. Let's look at some lines to see why:
# parse the list until it is sorteduntil @sorted == trueThe comment seems redundant. Your line of codeuntil @sorted == true reads clear enough for me to understand that you want to keep doing something until the list is sorted.
# if the first is greater than the second, swap them with parallel assignmentif list[i] > list[i+1] list[i], list[i+1] = list[i+1], list[i]This code is also pretty self explanatory. If you did have a comment you should be explainingwhy you do this. However, bubblesort is pretty well known so you probably don't even need a comment on this.
# increase the number of swaps performed in this run by 1swaps += 1Againwhy? I can read the code, it is obvious you're incrementing the number ofswaps. I would omit.
# compare the next 2 elementsi += 1Based of theif statement I would know that this the purpose ofi. I would omit it,but it does explain why you incrementi, so there is some grey zone here.
You write some good comments toward the end, let's analyze why they are good:
# If any swaps took place during the last run, the list is not yet sortedif swaps > 0 @sorted = false# no swaps? Everything is in orderelse @sorted = trueIt explainswhy we need to check ifswaps > 0 and how the sorting algorithm depends on this.
No need for@sorted variable
You can remove the@sorted variable and return directly if no swaps were performed, change the loop towhile true and addreturn if swaps == 0 instead of theif swaps ... @sorted =... statements.
No need for a class
You also do not need a class for this, you can either write a top-level function or put it into a module, a class is used to hold data and functions to operate on it, a class that holds a single static function is unreasonable.
Rubystep method
You should use the.step(2) method as it is more explicit than manual incrementing and modyfing a loop variable inside the body is unusual and breaks the expectation that we already know how many times afor loop should run.

