6
\$\begingroup\$

I created this program as an exercise for me to learn ruby. It is a random name generator with a set of rules defined to generate pretty decent and usually pronouncable names. Ruby isn't exactly my favourite language, but I needed to learn it. The names produced consist of both latin letters and a few non-latin symbols like å, ä and ö.

The code is fairly straight forward but I suspect there are a few things I do in the code that isn't considered good practise. It would be helpful in my path to learning ruby to get a code review on my program:

require 'unicode'

# Constants containing all consonants and vowels in the latin alphabet + some
# extra non-latin letters. The number after each letter represents how common
# a letter should be
CONS_LATIN = ['b']*100 + ['c']*100 + ['d']*100 + ['f']*100 + ['g']*100 + ['h']*100 + ['j']*100 + ['k']*100 + ['l']*100 + ['m']*100 + ['n']*100 + ['p']*100 + ['q']*85 + ['r']*100 + ['s']*100 + ['t']*100 + ['v']*100 + ['w']*50 + ['x']*75 + ['z']*50
VOWS_LATIN = ['a']*100 + ['e']*100 + ['i']*100 + ['o']*100 + ['u']*100 + ['y']*75
VOWS_EXTRA = ['ij']*75 + ['å']*100 + ['ä']*100 + ['ö']*100 + ['ø']*75 + ['æ']*60

# Banned combinations which are hard to pronounce or look weird
BANNED_COMBOS = [['g','j'],['f','k'],['b','k'],['q','p'],['w','q'],['q','g'],['x','x'],['q', 'q'],['d','b']]

def getRandomVowel
    # Only 10% chance to generate random "non-latin" vowel
    if rand() <= 0.1
        return VOWS_EXTRA.sample
    else
        return VOWS_LATIN.sample
    end
end

def getRandomVowelNoDuplicates(str:)
    # Generate a random vowel and if it a non-latin vowel
    # then we only use it if it has not been previously used in str
    vowel = getRandomVowel
    while VOWS_EXTRA.include? vowel and str.include? vowel
        vowel = getRandomVowel
    end
    return vowel
end

def getRandomConsonante
    return CONS_LATIN.sample
end

def getLastCharactersFromString(str:, numChars:)
    return Unicode::downcase (str[-numChars, numChars].to_s)
end

def isVowel(chr:)
    return ((VOWS_LATIN.include? (Unicode::downcase chr)) or
            (VOWS_EXTRA.include? (Unicode::downcase chr)))
end

def isConsonant(chr:)
    return (CONS_LATIN.include? (Unicode::downcase chr))
end

def generateLetter(currentName:)
    if currentName.empty?
        # We have a 60% chance to generate a vowel as the first letter
        if rand() <= 0.6
            return Unicode::upcase getRandomVowel
        else
            return Unicode::upcase getRandomConsonante
        end
    end

    if currentName.length < 2
        # Append random vowel or consonant in beginning to
        # prevent program from crashing if length of name is
        # less than 2
        if rand() <= 0.5
            chr = getRandomVowelNoDuplicates(str: currentName)
        else
            chr = getRandomConsonante
        end
        lastCharacters = chr + getLastCharactersFromString(str: currentName.join(""), numChars: 1)
    else
        lastCharacters = getLastCharactersFromString(str: currentName.join(""), numChars: 2)
    end
    # Apply rules
    #
    # 30% chance that there will be a double vowel
    # unless the last vowel is a non-latin vowel
    if isConsonant(chr: lastCharacters[0]) and isVowel(chr: lastCharacters[1])        
        if rand() <= 0.3 and (VOWS_LATIN.include? lastCharacters[1])
            return lastCharacters[1]
        end
    end
    # No more than 2 consonants in a row
    if isConsonant(chr: lastCharacters[0]) and isConsonant(chr: lastCharacters[1])
        # Exception for 'chr' and 'sch'
        cons = getRandomConsonante
        if (lastCharacters == "ch" and cons == 'r') or
           (lastCharacters == "sc" and cons == 'h')
            return cons
        else
            return getRandomVowelNoDuplicates(str: currentName)
        end
    end
    # No more than 2 vowels in a row
    if isVowel(chr: lastCharacters[0]) and isVowel(chr: lastCharacters[1])
        return getRandomConsonante
    end
    # If no condition above is met we have a 40% chance to generate a vowel
    # and a 60% chance to generate a consonante
    if rand() <= 0.4
        return getRandomVowelNoDuplicates(str: currentName)
    else
        # Prevent weird combinations like gj, fk or bk.
        cons = getRandomConsonante
        for combo in BANNED_COMBOS
          while (lastCharacters[1] == combo[0] and cons == combo[1])
            cons = getRandomConsonante
          end
        end
        return cons
    end
end

def generateRandomName
    # Generate a number between 3 and 12
    # The reason we have 9 instead of 12
    # is because rand()*12 could give 12
    # and when we add 3, we would get 15
    # which would be greater than 12 (9+3 == 12)
    nameLength = (rand()*9+3).round

    # Create new list and initialize counter to 0
    name = []
    counter = 0

    # We loop nameLength times
    # So if nameLength equals 9 we loop
    # 9 times. We put the result of
    # generateLetter into our name list
    # at position counter. Counter will
    # be increased AFTER we put the result
    # into our name list
    currentName = ""
    nameLength.times do        
        name[counter] = generateLetter(currentName: currentName)
        currentName = name
        counter = counter + 1
    end

    # Convert the list of characters to a string using join("")
    return name.join("")
end

def askForNumber
    begin
        puts "Enter number of names to generate"
        num = gets.chomp
    end while num.to_i.to_s != num # check if num is a valid number
    return num.to_i
end

# Generate amount of names the user enters
num = askForNumber
num.times do
    puts generateRandomName
end

Here is an example output of the program (10 names generated):

Eznogio
Ool
Ekv
Elqepzea
Ypbovdioszom
Ghagy
Uggua
Owsieg
Ujgenmybho
Saomyehzod
\$\endgroup\$
7
\$\begingroup\$

Naming

Names are lowercase_with_underscores in Ruby, please avoid javaStyle for consistency with other Ruby code.

get is a precise Object Oriented principle, adding it in front of most of your functions name is confusing and unnecessary.

Repetition

def getRandomVowel
    # Only 10% chance to generate random "non-latin" vowel
    if rand() <= 0.1
        return VOWS_EXTRA.sample
    else
        return VOWS_LATIN.sample
    end
end

return and .sample are repeated twice, also 10% and 0.1 can be considered repetitions of each other and more in general the comment just states what is obvious from reading the function.

This is how I fixed these issues:

def random_vowel
  (rand() < 0.1 ? VOWS_EXTRA : VOWS_LATIN).sample
end

CONS_LATIN = ['b']*100 + ['c']*100 + ['d']*100 + ['f']*100 + ['g']*100 + ['h']*100 + ['j']*100 + ['k']*100 + ['l']*100 + ['m']*100 + ['n']*100 + ['p']*100 + ['q']*85 + ['r']*100 + ['s']*100 + ['t']*100 + ['v']*100 + ['w']*50 + ['x']*75 + ['z']*50
VOWS_LATIN = ['a']*100 + ['e']*100 + ['i']*100 + ['o']*100 + ['u']*100 + ['y']*75
VOWS_EXTRA = ['ij']*75 + ['å']*100 + ['ä']*100 + ['ö']*100 + ['ø']*75 + ['æ']*60

100 is everywhere. I suggest you initialize all values to 100 that is the default and then change the less common ones.

WEIGHTS = Hash.new 100 # 100 is default
[ ['w', 50], ['x', 75], ['q', 85] ].each{ |ch, w| WEIGHTS[ch] = w}
CONS_LATIN = ('a'..'z').reject{|x| "aeiouy".include?(x)}.map{|x| x*WEIGHTS[x]}.join

I also avoided typing the whole alphabet by hand as that leaves room for hard-to-find bugs of forgetting one letter.

Conceptual repetition

def getRandomVowelNoDuplicates(str:)
    # Generate a random vowel and if it a non-latin vowel
    # then we only use it if it has not been previously used in str
    vowel = getRandomVowel
    while VOWS_EXTRA.include? vowel and str.include? vowel
        vowel = getRandomVowel
    end
    return vowel
end

And:

def askForNumber
    begin
        puts "Enter number of names to generate"
        num = gets.chomp
    end while num.to_i.to_s != num # check if num is a valid number
    return num.to_i
end

You do an action until a condition becomes true in both cases.

To express this general concept we need to pass in functions as arguments.

def perform_action_until_condition(action, condition)
     # Both action and condition are functions
     # Action takes no arguments and returns type T
     # condition takes a type T argument and returns a bool

     # TO DO: Implementation
end

Here is an implementation in Python of this concept. In this example code you can see how the repetition of the two similar while loops has been avoided.

Simplifications

if currentName.empty?
    # We have a 60% chance to generate a vowel as the first letter
    if rand() <= 0.6
        return Unicode::upcase getRandomVowel
    else
        return Unicode::upcase getRandomConsonante
    end
end

if currentName.length < 2
    # Append random vowel or consonant in beginning to
    # prevent program from crashing if length of name is
    # less than 2
    if rand() <= 0.5
        chr = getRandomVowelNoDuplicates(str: currentName)
    else
        chr = getRandomConsonante
    end
    lastCharacters = chr + getLastCharactersFromString(str: currentName.join(""), numChars: 1)
else
    lastCharacters = getLastCharactersFromString(str: currentName.join(""), numChars: 2)
end

Becomes:

if currentName.empty?
  return Unicode::upcase (rand() < 0.6 ? random_vowel : random_consonant)
end

if currentName.length < 2
  chr = [random_unique_vowel(current_name), random_consonant ].sample
# ... same

def generate_random_name
  length = (3..12).to_a.sample
  name = []
  length.times {names << generate_letter(name)}
  return name
end

The random number between a min and a max is just a random choice between the range. You had un-used variables such as current_name, the counter was not needed as << adds in the last position and forcing named parameters from function callers is not common nor encouraged in Ruby.


The main problem of this code still remains the generateLetter (generate_letter`) function that remains huge and complicated, so the next step in re-factoring would be to divide it into helper functions.

The second problem is the lack of tests but given the randomness of this program, testing requires seeding the RNG and is less effective than on deterministic programs. Tests help to see if refactorings like mine do indeed keep the functionality as before or change it.

The best quality of this code is that you divided it into functions, one was too big (see issue one) but the division helped my understanding over a single equivalent "blob" (all top level) program enormously.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the detailed answer, I have a slight problem though. CONS_LATIN = ('a'..'z').reject{|x| "aeiouy".include?(x)}.map{|x| x*WEIGHTS[x]} generates an array of string and thus when I use CONS_LATIN.sample, I get a string and not a character. \$\endgroup\$ – Linus Sep 16 '16 at 14:42
  • \$\begingroup\$ @Linus Adding .flatten should fix the problem. \$\endgroup\$ – Caridorc Sep 16 '16 at 17:06
  • \$\begingroup\$ I had to use [x] aswell, so it becomes CONS_LATIN = ('a'..'z').reject{|x| "aeiouy".include?(x)}.map{|x| [x]*WEIGHTS[x]}.flatten \$\endgroup\$ – Linus Sep 16 '16 at 17:09
  • \$\begingroup\$ How come I can't use a simple while loop for the random_unique_vowel and ask_for_number functions? I find it much easier to understand and I don't understand what perform_action_until_condition should do. \$\endgroup\$ – Linus Sep 16 '16 at 18:48
  • \$\begingroup\$ @Linus the while loops are a special kind a loop, in the body the only action is assigning the result of an action to a variable. The same concept is being shared: here is an implementation in Python codepaste.net/9ey9hh \$\endgroup\$ – Caridorc Sep 17 '16 at 9:36
1
\$\begingroup\$

You should convert all these functions into methods of a class RandomNameGenerator. This makes them invisible for other parts of the code.

Instead of using the global rand function, create a new Random field in the class. For testing, you can initialize this field with a constant random seed.

\$\endgroup\$

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.