3
\$\begingroup\$

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 end end
\$\endgroup\$

    2 Answers 2

    Reset to default
    2
    \$\begingroup\$

    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 sorted until @sorted == true

    The comment seems redundant. Your line of code until @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 assignment if 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 explaining why 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 1 swaps += 1

    Again why? I can read the code, it is obvious you're incrementing the number of swaps. I would omit.

    # compare the next 2 elements i += 1

    Based of the if statement I would know that this the purpose of i. I would omit it, but it does explain why you increment i, 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 sorted if swaps > 0 @sorted = false # no swaps? Everything is in order else @sorted = true

    It explains why we need to check if swaps > 0 and how the sorting algorithm depends on this.

    \$\endgroup\$
      1
      \$\begingroup\$

      No need for @sorted variable

      You can remove the @sorted variable and return directly if no swaps were performed, change the loop to while true and add return if swaps == 0 instead of the if 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.

      Ruby step 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 a for loop should run.

      \$\endgroup\$