自分で書いたコードを自分でリファクタリング

はじめに

最近「プロを目指す人のためのruby入門」を読み直しているものです。

[Ruby][解答募集]文字列に出現する単語の個数を数えてみよう

三ヶ月くらい前にこちらの記事を読んで自分が書いたコードが汚く見えてきたので自分が今まで学んだことでできる範囲でリファクタリングしてみました。
まずは元のコード

def count_word(string)
  words_array = string.split(' ')
  uniq_words = words_array.uniq
  hash = {}
  uniq_words.each do |word|
    hash[word] = words_array.count(word)
  end
  hash
end

リファクタリングしようと思えばより高速で短くする方法はいくらでもあると思いますが、今回はローカル変数の宣言が多いという問題点に絞ってロジックそのものは変えずその点を改良できるようなリファクタリングを行いました。

uniq_array

uniq_arrayを定義しなくても、hash[word]で同じキーの要素があれば更新されるのでいらないのでは、と考え、まずこの宣言を削除。このリファクタリングだと同じ単語の数だけ処理が行われてしまうので、よりよいリファクタリングはまだまだ追求できそう。

def count_word(strings)
  words_array = strings.split(' ')
  hash = {}
  words_array.each do |word|
    hash[word] = words_array.count(word)
  end
  hash
end

hash = {}

このハッシュの宣言も折りたたみ演算としてinjectを使えば削除できます。

  words_array.inject(Hash.new) do |hash, word|
    hash[word] = words_array.count(word)
    hash
  end

最終的に

最後になりましたがcount_wordというメソッド名も良くないですね。count_by_wordのほうが何をするメソッドなのかわかりやすいです

def count_by_word(string)
  words_array = string.split(' ')
  words_array.inject(Hash.new) do |hash, word|
    hash[word] = words_array.count(word)
    hash
  end
end

結局まだ一回変数宣言してますね… group_byしたりハッシュのバリューを増やしていく方法を使えば省略できそうですが今回はメインのロジックは変えない形でのリファクタリングなのでこれ以降はなかなか思いつきませんでした。もしまだまだイケる！という方いればぜひご教授ください。

まとめ

リファクタリングする際には昔書いたテストが役に立ちました。やっぱりテストは大事。
課題の記事のコメント欄にある回答例をみるとtapを使っていたり、group_byを使っていたりと目からウロコのやり方のコードを見れて勉強不足の身ながら感心しております。
みんなで一つの課題に対してコードを書いてお互いに見せ合うみたいな勉強会参加したいなぁ（欲望）ということで今回の記事はおしまいです。
最後まで読んでいただきありがとうございます。