2
\$\begingroup\$

This method takes a string and parses the syllables. I've broken much of the logic into helper methods. Without getting into the helper methods, it's pretty easy to tell what this is doing. But there are a lot of conditionals. I tried handling the conditions that handle adding to the syllables variable and those that subtract as two separate methods, but syllables kept changing value out from under me.

def count_syllables
  exceptions = YAML::load_file(File.join(__dir__, 'exceptions.yml'))['exceptions']
  return exceptions[word.to_s] if exceptions.keys.include?(word)
  syllables = count_vowels
  syllables += count_ys_in_vowel_role if contains_non_initial_y?
  syllables += 1 if contains_le_vowel_sound?
  syllables += 1 if begins_with_re_vowel? 
  syllables += 1 if ends_in_sm?
  syllables -= 1 if ends_in_silent_e?
  syllables -= count_diphthongs if contains_diphthongs?
  syllables <= 1 ? 1 : syllables
end
\$\endgroup\$
1
  • \$\begingroup\$ > This method takes a string and parses the syllables. Well, the method takes no arguments, you mean you are storing it in the class with a reader? \$\endgroup\$
    – tokland
    Jun 25, 2013 at 10:49

2 Answers 2

3
\$\begingroup\$

Some notes:

  • YAML::load_file. Loading a file each time a method is called is dubious. I'd load it in the initialize of the class so it's only done once.

  • return exceptions[word.to_s] if exceptions.keys.include?(word) Inline conditionals are ok as guards (in a fact here it's kind of a guard), but as a general rule indented conditionals are more declarative (granted, at the cost of an indentation level and some lines).

  • +=, -=. Imperative programming. As always, I'd advise against in-place updates, try to avoid statements in favor of expressions (functional programming).

I'd write:

def count_syllables
  if @exceptions.has_key?(word)
    @exceptions[word]
  else
    counts = [
      count_vowels,
      (count_ys_in_vowel_role if contains_non_initial_y?)
      (+1 if contains_le_vowel_sound?),
      (+1 if begins_with_re_vowel?) 
      (+1 if ends_in_sm?),
      (-1 if ends_in_silent_e?),
      (-count_diphthongs if contains_diphthongs?),
    ]
    [counts.compact.reduce(0, :+), 1].max
  end
end
\$\endgroup\$
2
  • \$\begingroup\$ I like this approach a lot. But I don't love that the majority of it falls in an else statement. Do you think you would lose a lot by an early return with an inline conditional here? \$\endgroup\$ Jun 26, 2013 at 2:56
  • \$\begingroup\$ @nickcoxdotme: it's orthogonal, you can leave the guard and still write the second part of the method similar to this. \$\endgroup\$
    – tokland
    Jun 26, 2013 at 6:50
2
\$\begingroup\$

Not really elegant, but you could merge the lines by using the ?: operator:

syllables = count_vowels + (count_ys_in_vowel_role if contains_non_initial_y?) + (contains_le_vowel_sound? ? 1 : 0) + ...
\$\endgroup\$
2
  • \$\begingroup\$ You'd need also a ?: for the second expression or you might try to sum a nil. But that's the way to go, expressions are definitely better than a parade of in-place updates. \$\endgroup\$
    – tokland
    Jun 25, 2013 at 15:30
  • 1
    \$\begingroup\$ this is way shorter, but hard to refactor or debug in the future. I personally feel like more than one conditional per line is a code smell \$\endgroup\$ Jun 25, 2013 at 17:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.