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 a better Rubyish 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.
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 better. Is there any improvements that I have not noticed? Then I will very pleased if you could comment it!
See you in the next posts!