Determine a poem's rhyme scheme

Question

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:

{"     ",
 "Measure your height",
 "AND WEIGHT      ",
 "said the doctor",
 "",
 "And make sure to take your pills",
 "   to   cure   your    ills",
 "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)}'"

Answer 1

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.

Answer 2

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