Most common letter in string, trying to use idiomatic Ruby

Question

The task:

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.

Model solution:

def most_common_letter(string)
 most_common_letter = nil
 most_common_letter_count = nil
 idx1 = 0
 while idx1 < string.length
 letter = string[idx1]
 count = 0
 idx2 = 0
 while idx2 < string.length
 if string[idx2] == letter
 count += 1
 end
 idx2 += 1
 end
 if (most_common_letter_count == nil) || (count > most_common_letter_count)
 most_common_letter = letter
 most_common_letter_count = count
 end
 idx1 += 1
 end
 return [most_common_letter, most_common_letter_count]
end

My solution:

def most_common_letter(string)
most_common_letter = nil
most_common_letter_count = 0
string.delete(" ").each_char do |letter|
 letter_count = 0
 string.each_char {|test| letter_count+=1 if test == letter }
 most_common_letter = letter and most_common_letter_count = letter_count if letter_count > most_common_letter_count
end
[most_common_letter, most_common_letter_count]
end

I tried using more idiomatic Ruby to solve this beginner problem we got in the beginning of the course. I'm wondering about a few things:

1. Is using "and" as I did in the if statement idiomatic? I tried a few guesses as to what might work (&&, &, some random characters), but I haven't seen this before.
2. I think the delete method alters the original string, and from the wonderful people on this site I've learned that altering the original data is unwise. How else could I ignore the spaces?
3. How can I tighten this up?

Answer 1

Your solution has common code smell: A block that modifies closed over variables (i.e. your block has side-effects, which should be avoided).

The code is also quite inefficient. For each character in a string, you loop through the entire string in an inner loop - even if you've done it for that letter already. E.g. a string like "aaa" will be doing a lot of unnecessary work. Besides, you could get the number of occurrences with some_string.count(some_letter).

It would be much better to run through the string once, and count letter occurrences by adding them, one by one, to a hash, and when seeing a letter you've seen before, incrementing its count. After that you have each letter and its number of occurrences, making it simple to pick out the most common.

In terms of style, that and line is... not very nice at all. You seem to be insisting on post-fixing conditionals (i.e. ... if x). And that'd be ok for small stuff, like x += 1 if y. But here, you have two separate lines of code, and you're glueing them together with and instead of just, well, using two lines:

if letter_count > most_common_letter_count
 most_common_letter = letter
 most_common_letter_count = letter_count
end

The and operator has its place, but it isn't this. You should read up on it.

For your second question: No, #delete does not alter the string. It returns a new string without the parts you asked to have deleted.

Anyway, an overall much more Ruby'esque approach would be something like this:

def most_common_letter(string)
 string
 .gsub(/[^a-z]/, '') # remove non-letters
 .chars # get characters
 .group_by(&:itself) # group them
 .map { |letter, list| [letter, list.count] } # get their counts
 .max_by(&:last) # get the most common letter and its count
end

It will return nil where yours returns [nil, nil] for an empty string. I prefer the former, rather than getting a array that has to be inspected more closely to see if it contains anything. But if the method should always return an array, you can do this:

def most_common_letter(string)
 most_common = string
 .gsub(/[^a-z]/, '') # remove non-letters
 .chars # get characters
 .group_by(&:itself) # group them
 .map { |letter, list| [letter, list.count] } # get their counts
 .max_by(&:last) # get the most common letter and its count
 most_common || [nil, nil]
end

• Matthew Farabaugh: Thx! I appreciate all the pointers. Lots to chew on!

Answer 2

Just some additional notes to the great answer of Flambino.

1. The ruby-style-guide is a nice resource for style related questions. There is a section about and in the giude. You should also take a look at the RuboCop gem, which checks your source code.
2. Nothing to add. See the documention of String#delete.
3. To avoid unnecessary iterations I would use one of Enumerable#reduce or Enumerator#with_object to count the occurrances of each letter. Since the result is a Hash, I personally prefer with_object.

Here is my solution.

def most_common_letter(string)
 string.each_char
 .with_object(Hash.new(0)) { |c,o| o[c] += 1 if c =~ /[a-z]/ } 
 .max_by(&:last)
end