Most common letter in string, trying to use idiomatic Ruby 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 &lt; string.length
    letter = string[idx1]
    count = 0

    idx2 = 0
    while idx2 &lt; string.length
      if string[idx2] == letter
        count += 1
      end
      idx2 += 1
    end

    if (most_common_letter_count == nil) || (count &gt; 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 &gt; 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:


Is using "and" as I did in the if statement idiomatic? I tried a few guesses as to what might work (&amp;&amp;, &amp;, some random characters), but I haven't seen this before.
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?
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 &gt; 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(&amp;:itself)  # group them
    .map { |letter, list| [letter, list.count] } # get their counts
    .max_by(&amp;: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(&amp;:itself)  # group them
    .map { |letter, list| [letter, list.count] } # get their counts
    .max_by(&amp;:last)      # get the most common letter and its count
  most_common || [nil, nil]
end