# Most common letter in string, trying to use idiomatic Ruby

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?

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

• Thx! I appreciate all the pointers. Lots to chew on! Oct 20, 2016 at 12:14

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.
def most_common_letter(string)