# Determine a poem's rhyme scheme

The TopCoder question specifies (in summary):

Two words are considered rhyming if they have the same ending pattern (defined below). This comparison is case-insensitive (see Example 1.)

An ending pattern is a substring of a word such that:

1. The word ends with that substring,
2. The substring contains exactly one contiguous string of vowels,
3. The first letter of the substring is a vowel, and
4. The substring must either be the whole string, or the letter immediately preceding the start of the substring must be a nonvowel.

For example, the ending pattern of "bought" is "ought", the ending pattern of "spying" would be "ying", and the ending pattern of "all" would be "all". (Note that "spy" has no vowels, and thus is not a legal word.)

Example 1:

{"     ",
"AND WEIGHT      ",
"said the doctor",
"",
"And make sure to take your pills",
"Every",
"DAY"}


Returns: " aab ccde"
Even though "height" and "weight" don't actually rhyme in English, they do by the rules laid out above.

I want to get general feedback and feedback about if I can make this more Ruby-like, if there are any obvious inefficiencies, and if I need to organize my class differently.

#!/usr/bin/env ruby -w
require 'set'

class Poetry
def find_ending(str)
vowels = Set.new(['a', 'e', 'i', 'o', 'u'])
vowel_seen = false
ending = ""
str.reverse.each_char.with_index do |c, i|
if vowels.include?(c) or
(c == 'y' and i != 0 and i != str.length-1)
vowel_seen = true
ending = c + ending
elsif !vowel_seen
ending = c + ending
else
break
end
end
ending
end

def rhyme_scheme(poem_array)
scheme = ""
next_label = 'a'
ending_labels = {}
poem_array.each do |line|
if line.strip.empty?
scheme += ' '
else
word = line.split(' ')[-1]
ending = self.find_ending(word.downcase)
label = ending_labels[ending]
if label.nil?
scheme += next_label
puts ending_labels
ending_labels[ending] = next_label
puts ending_labels
if next_label == 'z'
next_label = 'A'
else
next_label = next_label.next
end
else
scheme += label
end
end
end
scheme
end
end

poetry = Poetry.new

poem_array = ["I hope this problem",
"is a whole lot better than",
"this stupid haiku"]

puts "'#{poetry.rhyme_scheme(poem_array)}'"


Instead of commenting on your code, I felt I had to make a rewrite. Your code is very hard to understand because of characteristics like:

• Your methods contain a lot of functionality
• Using a class without reason (e.g. no use of internal state, a lot of things being done in one class)
• Your loops are very confusing, a lot is being done in each of them

I'm aware Topcoder is a competition platform to compete in fast code, not good code, but it seems from your question you want to learn to write better Ruby code and have simply chosen a challenging situation of doing so.

This is not what I would write in a competitive environment, but may start to approach how I'd design it in a commercial application where good code is preferred is often to fast code. I've tried to break down things, so each method does as little as possible and is fairly easy to understand. Iterations upon my code would include a better way to handle the 'a-z' => 'A-Z' transition as well as figuring out a better way to do Line#ending.

require 'set'

class Poem
attr_accessor :lines

def initialize
@rhymes = {"" => " "}
@lines = []
end

def rhymes
@lines.map { |line| @rhymes[line.ending] ||= next_id }
end

def lines=(lines)
@lines = lines.map { |line| Line.new(line) }
end

private

def next_id
fix_last_id(@rhymes.values.last)
end

def fix_last_id(last)
return 'a' if last == ' '
return "A" if last == 'z'
last.next
end

public

class Line
VOWELS = Set.new(['a', 'e', 'i', 'o', 'u'])

def initialize(line)
@line = line.downcase
@last_word = @line.split.last || ""
end

def ending
last_vowel = nil

reverse_last_word.chars.select.each_with_index do |character, index|
if !last_vowel || vowel_after_vowel?(last_vowel, index)
last_vowel = index if vowel?(index)
character
end
end.reverse.join
end

private
def reverse_last_word
@reverse_last_word ||= @last_word.reverse
end

def vowel_after_vowel?(last_vowel, index)
(index - last_vowel == 1 && vowel?(index))
end

def vowel?(index)
VOWELS.include?(reverse_last_word[index]) || y_vowel?(index)
end

def y_vowel?(index)
reverse_last_word[index] == 'y' && (1...@last_word.size).include?(index)
end
end
end

poem = Poem.new

poem.lines = [
"One bright day in the middle of the night",
"Two dead boys got up to fight",
"Back to back they faced each other",
"Drew their swords and shot each other",
"",
"A deaf policeman heard the noise",
"And came to arrest the two dead boys",
"And if you dont believe this lie is true",
"Ask the blind man he saw it too"
]

puts poem.rhymes.join


I haven't tried to upload it anywhere to check whether it is correct, however, it passes the public test cases.

• Need moar defs! May 24, 2013 at 18:05
• @Nakilon :-) :-) May 24, 2013 at 19:08
• The constructor creates an object in a not-very-useful state. It would be better if the constructor accepted lines of text. Feb 3, 2014 at 18:30

I am a bit late, I hope the answer is still useful. Some notes:

• First, my tradicional advice: don't use imperative programming! I mean, it's ok if you are writing assembler or C, but in a language like Ruby you're squandering its potential. Try to learn to program in functional style, favouring the good old math expressions (with its abstractions, immutability, higher-order functions, modulatization and many other cool things).
• I'd use regular expressions to get the rhyming substring of each verse, no custom code can beat their compactness and expression power.
• There are two ways of solving the problem, one is traversing the verses only once, folding the verses into an output. The second one is more modular, traverse the verses to build the intermediate data structures needed to calculate the solution. This second approach is, in this case, less verbose.

I'd write (second approach):

def rhyme_scheme(verses)
ending_regexp = /((?:[aeiou]|\By\B)+[^aeiou]*)\$/
endings = verses.map { |verse| verse.strip.downcase[ending_regexp, 1] }
ending_scheme_pairs = endings.uniq.compact.zip([*"a".."z", *"A".."Z"])
schemes = ending_scheme_pairs.to_h.merge(nil => " ")
schemes.values_at(*endings).join
end