4
\$\begingroup\$

Several months ago, I asked Program to count vowels, which was asking for a code review of a vowel counting function in implemented in Python.

Since I've been trying to learn Ruby, I decided to reimplement the entire program in it. I'm mainly looking for critiques on:

  • Naming conventions
  • Best practices for string iteration
  • do/end vs {}
  • Coding standards and best practices

Of course there are some slight changes in how I implemented this in ruby vs Python. The biggest being simply using a Hash since I couldn't find a suitable replacement for Python's tuples. On a unrelated side note, is there a replacement for Python's tuples in ruby?


def count_vowels(string)
    # Create a function that will receive a string. The
    # function will count each vowel in the string and
    # return a count of all the vowels, whether found or not,
    # each being a `key, value` in a Hash.
    vowels = {"a" => 0, "e" => 0, "i" => 0, "o" => 0, "u" => 0}
    string.each_char do |chr|
        if vowels.has_key?(chr)
            vowels[chr] += 1
        end
    end
    return vowels
end
\$\endgroup\$
  • \$\begingroup\$ How would you implement this in Python using tuples?! I'd just use collections.Counter. Which is pretty much the same as what you're doing. \$\endgroup\$ – Peilonrayz Jan 6 '17 at 9:10
  • \$\begingroup\$ @Peilonrayz Yes, I know very well thatcollections.Counter() is the "proper" way to go about something such as this. I created that program for practice only. If I ever had to do something like this for production level code, I'd use the python standard library. \$\endgroup\$ – Christian Dean Jan 6 '17 at 9:37
7
\$\begingroup\$

You could use Enumerable#count:

string.chars.count { |char| vowels.has_key?(char) }

You very rarely need to declare a variable (i in this case), and then modify it form inside a block; there's almost always a better way in Ruby.

So while calling count and passing a block is the most direct, you could also have done something like:

string.chars.select { |char| vowels.has_key?(char) }.count

Same idea, just written out in separate filter/count steps.

You can also skip the return since that's implied at the end of a method.

I'd also use a plain array for the vowels. Looking up hash keys is faster, sure, but does it really matter that much? Nah. "Premature optimization is the root of all evil" and all that.

vowels = %w(a e i o u) # same as ["a", "e", "i", ...]

and then the block becomes:

vowels.include?(char)

If you keep the vowels array sorted, you can use bsearch if you want to speed it up a little. Or you can construct the hash in code, instead of typing all that => 0:

list = %w(a e i o u)
vowels = Hash[list.zip(list)] # => { "a" => "a", "e" => "e", ... }

Of course you could also do something funky like this: Remove all the vowels and see how much shorter the string is afterward:

def count_vowels(string)
  string.length - string.gsub(/[aeiou]/, '').length
end

This is also easily made case-insensitive:

string.length - string.gsub(/[aeiou]/i, '').length

Or instead of gsub you can use tr:

string.length - string.tr('aeiou', '').length

(or 'aeiouAEIOU' for case-insensitivity.)


Alternatively, you could extend/monkey-patch the String class a little. I wouldn't recommend it for production code, though. This is just illustrate some Ruby features:

class String
  def vowel?
    self =~ /\A[aeiou]\z/ ? true : false
  end
end

def count_vowels(string)
  string.chars.count(&:vowel?)
end

The &x syntax is a shorthand meaning "invoke x on each element in the collection", so it's equivalent to { |e| e.x }.

Again, this is mostly for fun. I just thought it made the count_vowels method nice and short. And of course, count_vowels could itself be monkey-patched onto String, so you could just call some_string.count_vowels. But again, monkey-patching - while fun - shouldn't be the first thing you reach for. Just illustrating the principle.

\$\endgroup\$
  • 1
    \$\begingroup\$ Also s.chars.grep(/[aeiou]/).size is a short way to accomplish the same thing \$\endgroup\$ – Mark Thomas Jan 11 '17 at 3:03
  • 1
    \$\begingroup\$ Another shorter way is str.count('aeoui') \$\endgroup\$ – Engr. Hasanuzzaman Sumon Jan 16 '17 at 12:27
  • \$\begingroup\$ @Engr.HasanuzzamanSumon Neat. Didn't know you could do that \$\endgroup\$ – Flambino Jan 17 '17 at 9:54
  • \$\begingroup\$ I recently learn about refinements in Ruby, so instead of doing a really bad monkey patching you can limit it to small place in your code. timelessrepo.com/refinements-in-ruby \$\endgroup\$ – Marc-Andre Jan 21 '17 at 17:16
  • \$\begingroup\$ @Marc-Andre Yeah, I also just learned about them (like, just after posting this answer). Note though, that I'm not recommending monkey-patching, just illustrating the principle. And in any case, I wouldn't call this a "really bad monkey-patch"; it's fairly innocuous \$\endgroup\$ – Flambino Jan 21 '17 at 17:34
1
\$\begingroup\$

Try this

def count_vowels(string)
  vowels = { "a" => 0, "e" => 0, "i" => 0, "o" => 0, "u" => 0 }
  string.each_char { |char| vowels[char] += 1 if vowels.include?(char) }
  vowels
end

What did I change?

  • Use two space indentation
  • Don't abbr var nms, spell out char
  • Use trailing if statement
  • Use one-line loop with {} given the loop body is a single line now
  • Omit return, the last statement is always returned

And if you want a oneliner try

vowels = string.scan(/[aeiou]/).group_by(&:itself).transform_values(&:count)
\$\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.