Ruby: How to create a letter counting function

Question

I wouldn't have asked for help without first spending a few hours trying to figure out my error but I'm at a wall. So there is my attempt but I'm getting false when I try to pass the argument though and I'm not sure why. I know there are other ways to solve this problem that are a little shorter but I'm more interested in trying to get my code to work. Any help is much appreciated.

Write a method that takes in a string. Your method should return the most common letter in the array, and a count of how many times it appears.

def most_common_letter(string)
  idx1 = 0
  idx2 = 0
  counter1 = 0
  counter2 = 0

  while idx1 < string.length
    idx2 = 0
    while idx2 < string.length
      if string[idx1] == string[idx2]
        counter1 += 1
      end
      idx2 += 1
    end
    if counter1 > counter2
      counter2 = counter1 
      counter1 = 0
      var = [string[idx1], counter2] 
    end
    idx1 += 1
  end
  return var
end

puts("most_common_letter(\"abca\") == [\"a\", 2]: #{most_common_letter("abca") == ["a", 2]}")
puts("most_common_letter(\"abbab\") == [\"b\", 3]: #{most_common_letter("abbab") == ["b", 3]}")

Answer 1

Your question has been answered and @theTinMan has suggested a more Ruby-like way of doing what you want to do. There are many other ways of doing this and you might find it useful to consider a couple more.

Let's use the string:

string = "Three blind mice. Oh! See how they run."

First, you need to answer a couple of questions:

• do you want the frequency of letters or characters?
• do you want the frequency of lowercase and uppercase letters combined?

I assume you want the frequency of letters only, independent of case.

#1 Count each unique letter

We can deal with the case issue by converting all the letters to lower or upper case, using the method String#upcase or String#downcase:

s1 = string.downcase
  #=> "three blind mice. oh! see how they run."

Next we need to get rid of all the characters that are not letters. For that, we can use String#delete¹:

s2 = s1.delete('^a-z')
  #=> "threeblindmiceohseehowtheyrun"

Now we are ready to convert the string s2 to an an array of individual characters²:

arr = s2.chars
  #=> ["t", "h", "r", "e", "e", "b", "l", "i", "n", "d",
  #    "m", "i", "c", "e", "o", "h", "s", "e", "e", "h",
  #    "o", "w", "t", "h", "e", "y", "r", "u", "n"]

We can combine these first three steps as follows:

arr = string.downcase.gsub(/[^a-z]/, '').chars

First obtain all the distinct letters present, using Array.uniq.

arr1 = arr.uniq
  #=> ["t", "h", "r", "e", "b", "l", "i", "n",
  #    "d", "m", "c", "o", "s", "w", "y", "u"]

Now convert each of these characters to a two-character array consisting of the letter and its count in arr. Whenever you need convert elements of a collection to something else, think Enumerable#map (a.k.a. collect). The counting is done with Array#count. We have:

arr2 = arr1.map { |c| [c, arr.count(c)] }
  #=> [["t", 2], ["h", 4], ["r", 2], ["e", 6], ["b", 1], ["l", 1],
  #    ["i", 2], ["n", 2], ["d", 1], ["m", 1], ["c", 1], ["o", 2],
  #    ["s", 1], ["w", 1], ["y", 1], ["u", 1]]

Lastly, we use Enumerable#max_by to extract the element of arr2 with the largest count³:

arr2.max_by(&:last)
  #=> ["e", 6]

We can combine the calculation of arr1 and arr2:

arr.uniq.map { |c| [c, arr.count(c)] }.max_by(&:last)

and further replace arr with that obtained earlier:

string.downcase.gsub(/[^a-z]/, '').chars.uniq.map { |c|
  [c, arr.count(c)] }.max_by(&:last)
  #=> ["e", 6]

String#chars returns a temporary array, upon which the method Array#uniq is invoked. As alternative, which avoids the creation of the temporary array, is to use String#each_char in place of String#chars, which returns an enumerator, upon which Enumerable#uniq is invoked.

The use of Array#count is quite an inefficient way to do the counting because a full pass through arr is made for each unique letter. The methods below are much more efficient.

#2 Use a hash

With this approach we wish to create a hash whose keys are the distinct elements of arr and each value is the count of the associated key. Begin by using the class method Hash::new to create hash whose values have a default value of zero:

h = Hash.new(0)
  #=> {}

We now do the following:

string.each_char { |c| h[c.downcase] += 1 if c =~ /[a-z]/i }
h #=> {"t"=>2, "h"=>4, "r"=>2, "e"=>6, "b"=>1, "l"=>1, "i"=>2, "n"=>2,
  #    "d"=>1, "m"=>1, "c"=>1, "o"=>2, "s"=>1, "w"=>1, "y"=>1, "u"=>1}

Recall h[c] += 1 is shorthand for:

h[c] = h[c] + 1

If the hash does not already have a key c when the above expression is evaluated, h[c] on the right side is replaced by the default value of zero.

Since the Enumerable module is included in the class Hash we can invoke max_by on h just as we did on the array:

h.max_by(&:last)
  #=> ["e", 6]

There is just one more step. Using Enumerable#each_with_object, we can shorten this as follows:

string.each_char.with_object(Hash.new(0)) do |c,h|
  h[c.downcase] += 1 if c =~ /[a-z]/i
end.max_by(&:last)
  #=> ["e", 6]

The argument of each_with_object is an object we provide (the empty hash with default zero). This is represented by the additional block variable h. The expression

string.each_char.with_object(Hash.new(0)) do |c,h|
  h[c.downcase] += 1 if c =~ /[a-z]/i
end

returns h, to which max_by(&:last) is sent.

#3 Use group_by

I will give a slightly modified version of the Tin Man's answer and show how it works with the value of string I have used. It uses the method Enumerable#group_by:

letters = string.downcase.delete('^a-z').each_char.group_by { |c| c }
  #=> {"t"=>["t", "t"], "h"=>["h", "h", "h", "h"], "r"=>["r", "r"],
  #    "e"=>["e", "e", "e", "e", "e", "e"], "b"=>["b"], "l"=>["l"],
  #    "i"=>["i", "i"], "n"=>["n", "n"], "d"=>["d"], "m"=>["m"],
  #    "c"=>["c"], "o"=>["o", "o"], "s"=>["s"], "w"=>["w"],
  #    "y"=>["y"], "u"=>["u"]}

used_most = letters.max_by { |k,v| v.size }
  #=> ["e", ["e", "e", "e", "e", "e", "e"]]
used_most[1] = used_most[1].size
used_most
  #=> ["e", 6]

In later versions of Ruby you could simplify as follows:

string.downcase.delete('^a-z').each_char.group_by(&:itself).
       transform_values(&:size).max_by(&:last)
  #=> ["e", 6]

See Enumerable#max_by, Object#itself and Hash#transform_values.

^{1. Alternatively, use String#gsub: s1.gsub(/[^a-z]/, '').}

^{2. s2.split('') could also be used.}

^{3. More or less equivalent to arr2.max_by { |c, count| count }.}

Answer 2

I didn't rewrite your code because I think it's important to point out what is wrong with the existing code that you wrote (especially since you're familiar with it). That said, there are much more 'ruby-like' ways to go about this.

The issue

counter1 is only being reset if you've found a 'new highest'. You need to reset it regardless of whether or not a new highest number has been found:

def most_common_letter(string)
  idx1 = 0
  idx2 = 0
  counter1 = 0
  counter2 = 0
  while idx1 < string.length
    idx2 = 0
    while idx2 < string.length
      if string[idx1] == string[idx2]
        counter1 += 1
      end
      idx2 += 1
    end
    if counter1 > counter2
      counter2 = counter1 
      # counter1 = 0  THIS IS THE ISSUE
      var = [string[idx1], counter2] 
    end
    counter1 = 0  # this is what needs to be reset each time
    idx1 += 1
  end
  return var
end

Here's what the output is:

stackoverflow master % ruby letter-count.rb
most_common_letter("abca") == ["a", 2]: true
most_common_letter("abbab") == ["b", 3]: true

I think you're aware there are way better ways to do this but frankly the best way to debug this is with a piece of paper. "Ok counter1 is now 1, indx2 is back to zero", etc. That will help you keep track.

Another bit of advice, counter1 and counter2 are not very good variable names. I didn't realize what you were using them for initially and that should never be the case, it should be named something like current_count highest_known_count or something like that.

Answer 3

It's a problem you'll find asked all over Stack Overflow, a quick search should have returned a number of hits.

Here's how I'd do it:

foo = 'abacab'
letters = foo.chars.group_by{ |c| c }

used_most = letters.sort_by{ |k, v| [v.size, k] }.last
used_most # => ["a", ["a", "a", "a"]]
puts '"%s" was used %d times' % [used_most.first, used_most.last.size]
# >> "a" was used 3 times

Of course, now that this is here, and it's easily found, you can't use it because any teacher worth listening to will also know how to search Stack Overflow and will find this answer.