Reviewing old code: The Ruby way

Some time ago I made a racing videogame, written in Ruby using Gosu. Since I came from C++,SDL videogame programming, I didn't have my brain set into "Ruby mode" and most of my code is written like C++ code but using Ruby syntax. Now I will review some of this and hopefully rewrite them in the best Ruby way.

I will start with a bunch of methods used for generating random names for the enemies. This is the original code:

 # Random Name Generator
 # based in the code of Joshua Smith

class RandomName

    def getRandomName()

        retName = ""
        length = rand(1)+5;
        # CVCCVC or VCCVCV
        if (rand(1) < 1) then

            retName += getRandomConsonant()
            retName = retName.upcase
            retName += getRandomVowel()
            retName += getRandomConsonant()
            retName += getRandomConsonant()
            if (length >= 5) then
                retName += getRandomVowel()
            end
            if (length >= 6) then
                retName += getRandomConsonant()
            end
        else
            retName += getRandomVowel()
            retName = retName.upcase
            retName += getRandomConsonant()
            retName += getRandomConsonant()
            retName += getRandomVowel()
            if (length >= 5) then
                retName += getRandomConsonant()
            end
            if (length >= 6) then
                retName += getRandomVowel()
            end
        end
        return retName
    end

    def getRandomVowel()
        randNum = rand(3)+1
        case (randNum)
        when 0
            return 'a'
        when 1
            return 'e'
        when 2
            return 'i'
        when 3
            return 'o'
        when 4
            return 'u'
        end
    end

    def  getRandomConsonant()
        randLetter = (rand(25)+97).chr
        while (isCharVowel(randLetter)) do
            randLetter = (rand(25)+97).chr
        end
        return randLetter
    end

    def isCharVowel(letter)
        if (letter == 'a' or letter == 'e' or letter == 'i' or letter == 'o' or letter == 'u') then
            return true
        else
            return false
        end
    end
end  

Ug! Thats horribly convoluted! Let's start rewriting the smallest methods first. Starting with isCharVowel.

    def isCharVowel(letter)
        if (letter == 'a' or letter == 'e' or letter == 'i' or letter == 'o' or letter == 'u') then
            return true
        else
            return false
        end
    end
end  

Looking at this Ruby Style Guide, we can read:

Use two spaces per indentation level (aka soft tabs). No hard tabs.  

For fixing that, I will have to modify mi system's .vimrc, as we can read in this blog post about vim's tabbing the options that I have to change are:

set tabstop=2  
set shiftwidth=2  

Now I can reident the whole source code file using this set of shortcuts:

- Shift+V enter into select mode
- Shift+G Go to the end of the file
- Shift+= Autoident the selected block of code

And now the proper identation is set.

  def isCharVowel(letter)
    if (letter == 'a' or letter == 'e' or letter == 'i' or letter == 'o' or letter == 'u') then
      return true
    else
      return false
    end 
  end

The method's name is also not following the Ruby naming conventions, for method names we can read the following:

Method names should start with a lowercase letter, and may be followed by digits, underscores, and letters, e.g. paint, close_the_door  

I also remember reading in the book Agile Web Development with Rails 4 that is a good practice to end method names that return true or false like in ths case with the '?' character. So a proper name for this method would be is_vowel? or just vowel?

  def vowel?(letter)
    if (letter == 'a' or letter == 'e' or letter == 'i' or letter == 'o' or letter == 'u') then
      return true
    else
      return false
    end 
  end

Now I see, that the code has the following structure:

if condition  
    return value1
else  
    return value2
end  

That's the perfect candidate for being replaced by a ternary operator, like this:

return condition ? value1 : value2  

The method will look like this now:

  def vowel?(letter)
    return (letter == 'a' or letter == 'e' or letter == 'i' or letter == 'o' or letter == 'u') ? true : false
  end

The or chain looks horrible, and is easy replaceable using an include operator:

  def vowel?(letter)
    return ['a','e','i','o','u'].include? letter
  end

And as Ruby always returns the last evaluated expression, I can even get rid of the return word:

  def vowel?(letter)
    ['a','e','i','o','u'].include? letter
  end

The method looks ok for now, as we should avoid single line method defintions.

Now moving up, there is the previous method:

def getRandomConsonant()  
  randLetter = (rand(25)+97).chr
    while (isCharVowel(randLetter)) do
      randLetter = (rand(25)+97).chr
    end
  return randLetter
end  

Ugh! This method makes use of the one that I previously rewrote, as it's name says what id does is getting a random consonant by using a loop. That's ugly and completely against the Ruby way of doing things.

If we rethink the whole method, it has just two steps:

- Define the set of possible letters
- Pick a random one

The Ruby way of defining a set of numbers or letters like this is by using ranges. So if I want to express "I wan't all the letters from a to z" I will just write a..z, in this particular case there is a small subset that has to be excluded, so defining the set needed for this would be:

[*'b'..'z']-['e','i','o','u']

The * is the splat operator, and then the difference is used to substract the set of letters that I don't need. There's the set of possible letters, now for picking a random element, Ruby's Array class provides a method called sample. So the whole getRandomConsontant method can be rewritten like this:

def random_consonant  
    ([*'b'..'z']-['e','i','o','u']).sample
end  

Just in one line! Now the vowel? method can be deleted. The same way, the getRandomVowel can be rewritten into this:

def random_vowel  
    ['a','e','i','o','u'].sample
end  

Now I can deal with the main method, the one that generates the random name:

    def getRandomName()

        retName = ""
        length = rand(1)+5;
        # CVCCVC or VCCVCV
        if (rand(1) < 1) then

            retName += getRandomConsonant()
            retName = retName.upcase
            retName += getRandomVowel()
            retName += getRandomConsonant()
            retName += getRandomConsonant()
            if (length >= 5) then
                retName += getRandomVowel()
            end
            if (length >= 6) then
                retName += getRandomConsonant()
            end
        else
            retName += getRandomVowel()
            retName = retName.upcase
            retName += getRandomConsonant()
            retName += getRandomConsonant()
            retName += getRandomVowel()
            if (length >= 5) then
                retName += getRandomConsonant()
            end
            if (length >= 6) then
                retName += getRandomVowel()
            end
        end
        return retName
    end

Extremely ugly, but not for long. Again, the whole prupose of the method is returning a name using the structure detailed in the comment line:

    # CVCCVC or VCCVCV

Those two structures can be expressed like this:

CVCCVC = [random_consonant,random_vowel.. and so  

Now I notice that the code is wrong. The length is random(1), that's going to be always 0. But the code has conditions for the length being 4, 5, or 6.

Anyway the whole set of possible combinations taking in consideration the length and the two structures are:

CVCC  
CVCCV  
CVCCVC  
VCCV  
VCCVC  
VCCVCV  

That can be expressed using the methods previously defined as:

[[random_consonant.upcase,random_vowel,random_consonant,random_consonant], [random_consonant.upcase,random_vowel,random_consonant,random_consonant,random_vowel], [random_consonant.upcase,random_vowel,random_consonant,random_consonant,random_vowel,random_consonant], [random_vowel.upcase,random_consonant,random_consonant,random_vowel], [random_vowel.upcase,random_consonant,random_consonant,random_vowel,random_consonant], [random_vowel.upcase,random_consonant,random_consonant,random_vowel,random_consonant,random_vowel]]

There it is the whole set of possible combinations, then for seleting a random one I will just use the sample method, and for converting the array of chars into a string the join method.

So the whole getRandomName method now looks like this:

def random_name      [[random_consonant.upcase,random_vowel,random_consonant,random_consonant], [random_consonant.upcase,random_vowel,random_consonant,random_consonant,random_vowel], [random_consonant.upcase,random_vowel,random_consonant,random_consonant,random_vowel,random_consonant],   [random_vowel.upcase,random_consonant,random_consonant,random_vowel], [random_vowel.upcase,random_consonant,random_consonant,random_vowel,random_consonant],  [random_vowel.upcase,random_consonant,random_consonant,random_vowel,random_consonant,random_vowel]].sample.join  
end  

Much better, there's a gif of the whole shrinking process.

Code shrinking

It makes no sense having the random_name method as an instance method instead of a class one because the class is not holding any data. Also, as the random_name method makes use of random_vocal and random_consonant, those two need to be declared as class methods too. Putting 'self.' before it's names does it. And finally as the code will only ask this class for a random name from the outside, I will make random_consontant and random_vowel private methods using private_class_method.

The resulting class code looks like this:

# Random Name Generator
# based in the code of Joshua Smith

class RandomName

  def self.random_name
    [[random_consonant.upcase,random_vowel,random_consonant,random_consonant],
    [random_consonant.upcase,random_vowel,random_consonant,random_consonant,random_vowel],
    [random_consonant.upcase,random_vowel,random_consonant,random_consonant,random_vowel,random_consonant],
    [random_vowel.upcase,random_consonant,random_consonant,random_vowel],
    [random_vowel.upcase,random_consonant,random_consonant,random_vowel,random_consonant],
    [random_vowel.upcase,random_consonant,random_consonant,random_vowel,random_consonant,random_vowel]].sample.join
  end 

  def self.random_vowel
    ['a','e','i','o','u'].sample
  end 

  def self.random_consonant
    ([*'b'..'z']-['e','i','o','u']).sample
  end 

  private_class_method :random_vowel,:random_consonant

end  

Much much better. Is there any improvements that I have not noticed? Then I will very pleased if you comment it!

See you in the next posts!