appacademy / prep-work

preparatory material for interviewing with App Academy
291 stars 339 forks source link

revise update 02_letter_count #50

Closed Schwad closed 9 years ago

Schwad commented 9 years ago

new change based on comment feedback

jeffnv commented 9 years ago

OK, two more comments. putsing out the result at the end I don't like, this looks like some relic of debugging. I would simply return the result at the end. The purpose of this function is to return a result, not print something. Next, str.split('').each is unnecessary; this is exactly what each_char is for. Using the method for it's designed purpose means clearer more obvious code which is what we want to write.

Schwad commented 9 years ago

Thank you so much for your thoughts.

Your comments are spot on, and helped clean up code I was unnecessarily putting in there or using inefficiently. I have tried to incorporate your feedback below, and will edit my code on the pull request. What do you think?

def letter_count(str)
  counts = Hash.new(0)

  str.each_char do |x|
    counts[x] += 1 unless x == " "
  end
  return counts
end
jeffnv commented 9 years ago

ok one last thing: I wouldn't explicitly return counts, instead just counts at the end and I'll merge this :+1:

Schwad commented 9 years ago

Okay; tidied that up and looks good. Thank you for the clear/patient feedback, I really appreciate it.

rglassett commented 9 years ago

Rebased in. Thanks!