Your solution has common code smell: A block that modifies closed over variables (i.e. your block also 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

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

Your solution has common code smell: A block that modifies closed over variables. I.e. your block as 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'd 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'll 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

Your solution has common code smell: A block that modifies closed over variables (i.e. your block also 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