3
\$\begingroup\$

For practice I tried implementing a bubble sort algorithm in Ruby. I'm especially interested in creating clean DRY and KISS code and keeping things readable, but efficient. Any hints to improve things would be very much appreciated. The code looks like this:

class Sorter
  def initialize
  end

  def sort(stack)
    if stack.length == 0
      return stack
    else
      checkArguments(stack)
      if stack.length == 1
        return stack
      else
        newstack = []
        stackIsSorted = false
        while !stackIsSorted
          newstack = iterateThroughStack(stack)
          if  newstack == stack
            stackIsSorted = true
          else
            stack = newstack
          end
        end
        return newstack
      end
    end
  end

  def checkArguments(stack)
    stack.each do  
      |element|  
      if element.class != Float && element.class != Fixnum
        raise ArgumentError, "element detected in stack that is not an integer or double"
      end
    end
  end

  def iterateThroughStack(stack)
    newstack = []
    currentElement = stack[0]
    for i in (1...stack.length)
      if  currentElement < stack[i]
        newstack.push(currentElement)
        currentElement = stack[i]
      else
    newstack.push(stack[i])
      end
    end
    newstack.push(currentElement)
  end
end

Then, after reading about Test Driven Development, I started using this practice and since then, I think code makes more sense with unit tests. So below are the unit test I wrote:

require 'test/unit'
require 'lib/Sorter'

class Test_Sorter < Test::Unit::TestCase
  def setup
    @sorter = Sorter.new
  end

  def test_emptyStack
    stack = []
    assert_equal(stack, @sorter.sort(stack), "sorting empty stack failed")
  end

  def test_StackWithOneElement
    stack = [1]
    assert_equal(stack, @sorter.sort(stack), "sorting stack with one element: 1 failed")
    stack = ["a"]
    assert_raise (ArgumentError) { @sorter.sort(stack) }
  end

  def test_StackWithTwoElements
    stack = [2, 1]
    sorted_stack = [1, 2]
    assert_equal(sorted_stack, @sorter.sort(stack), "sorting stack with two elements: 1, 2 failed")
    stack = [2, "a"]
    assert_raise (ArgumentError) { @sorter.sort(stack) }
  end

  def test_StackWithThreeElements
    stack = [2, 3, 1]
    sorted_stack = [1, 2, 3]
    assert_equal(sorted_stack, @sorter.sort(stack), "sorting stack with three elements: 1, 2, 3 failed")
  end

  def test_StackWithFourElements
    stack = [4, 2, 3, 1]
    sorted_stack = [1, 2, 3, 4]
    assert_equal(sorted_stack, @sorter.sort(stack), "sorting stack with four elements: 1, 2, 3, 4 failed")
  end

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

There are few things I would do differently:

  1. Using class without instance variables doesn't make much sense, you can use module for that if the only thing you want is to limit scope. Of course it had been useful if you would have initialized class with some parameters like Sorter.new(Float, Fixnum) and saved them for later use,
  2. When you iterate through the stack you know for sure if you swapped items or not, so I would say it's a good idea to save this information inside iterateThroughStack and pass it back so you don't need to compare arrays in sort,
  3. Duck typing in Ruby means you normally don't check types, who cares what type of element is if it allows comparison, so I would remove checkArguments unless you have some special requirements,
  4. I would use each instead of for,
  5. Don't use return unless you want to return from the middle of a function (which is presumably bad thing to do).

So it all comes down to this:

class Sorter
  def initialize
  end

  def sort(stack)
    if stack.length > 1
      stackIsSorted = false
      while !stackIsSorted
        stackIsSorted, stack = iterateThroughStack(stack)
      end
    end
    stack
  end

  def iterateThroughStack(stack)
    stackIsSorted = true
    newstack = []
    currentElement, *tail = stack
    tail.each do |element|
      if  currentElement < element
        newstack.push(currentElement)
        currentElement = element
      else
        newstack.push(element)
        stackIsSorted = false
      end
    end
    [stackIsSorted, newstack.push(currentElement)]
  end
end
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks Victor! I never heard of the *tail syntax, but found it here. Your code would require a test to be changed: sort(["a"]) will now return ["a"] in stead of raising an error. Isn't that an issue? \$\endgroup\$ – bartaelterman May 24 '12 at 7:33
  • \$\begingroup\$ It depends on your requirements, strings can be sorted too. \$\endgroup\$ – Victor Moroz May 24 '12 at 13:42
1
\$\begingroup\$

Just a note about the tests: I'd create a test method for every assert call. For example,

  def test_StackWithTwoElements
    stack = [2, 1]
    sorted_stack = [1, 2]
    assert_equal(sorted_stack, @sorter.sort(stack), "sorting stack with two elements: 1, 2 failed")
    stack = [2, "a"]
    assert_raise (ArgumentError) { @sorter.sort(stack) }
  end

would be

  def test_StackWithTwoElements
    stack = [2, 1]
    sorted_stack = [1, 2]
    assert_equal(sorted_stack, @sorter.sort(stack), "sorting stack with two elements: 1, 2 failed")
  end


  def test_StackWithInvalidElement
    stack = [2, "a"]
    assert_raise (ArgumentError) { @sorter.sort(stack) }
  end

Too many asserts in one test is a bad smell. It's Assertion Roulette and you lost Defect Localization. If the first assert_equal throws an exception you don't know anything about the results of the other assert calls which could be important because they could help debugging and defect localization.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ That's a nice suggestion. Thanks for sharing the links too. Interesting stuff. \$\endgroup\$ – bartaelterman May 21 '12 at 13:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.