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  endend
200_success's user avatar
200_success
146k22 gold badges191 silver badges481 bronze badges
askedAug 21, 2016 at 21:41
\$\endgroup\$

2 Answers2

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

The 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 += 1

Againwhy? I can read the code, it is obvious you're incrementing the number ofswaps. I would omit.

# compare the next 2 elementsi += 1

Based 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 = true

It explainswhy we need to check ifswaps > 0 and how the sorting algorithm depends on this.

answeredAug 21, 2016 at 22:54
Dair's user avatar
\$\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 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.

answeredAug 22, 2016 at 11:13
Caridorc's user avatar
\$\endgroup\$

You mustlog in to answer this question.