コードを綺麗に書くって難しい
書いてる時は気づかないし、書き終わった後で眺めていて気付いたとしても、一旦考えて書いたコードを分解して再構築するのが難しい。再構築する際にコードの書き方に違ったアプローチが必要だからですね。一旦、こうだと思って書いたコードから頭を真っ白にして書き直すところが山場です。
今回はそんな書き直した部分で気付いたところを幾つかパターン分けして書き出します。自分の経験による覚書です。
ほとんど同じ処理が条件式の中に入ってる
実際、やりたい処理は同じだが、引数で渡ってくるparamの中身がon_totalとon_this_monthの２種類あり、その二つのどっちかを検知したら、ソートする処理を入れるってことをしたかったわけですね。最初に書いたのは、下記。
def self.ascend_checkins(param, hash)
if param == "on_total"
hash.sort_by { |k,v| -v["total"] }.to_h
elsif param == "on_this_month"
hash.sort_by { |k,v| -v["month"] }.to_h
end
end
先述の理由から、同じソート処理が入ってしまっているので、下記のように修正。
def self.ascend_checkins(hash, param)
key_of = {"on_total" => "total", "on_this_month" => "month"}
value = key_of[param]
raise "unknown param" if !value
hash.sort_by { |k,v| -v[value] }.to_h
end
2つのparamしか無いので、ハッシュを用意して、paramを受けることで、それに対応したバリューを真偽値で判定して真の場合、処理を実行するようにすることで同処理を削除。
一時変数が多い
下記は、ユーザーIDを文字列にして取り出し、それの個数を数えて、ハッシュにするという処理をしています。
空の配列を用意したりする際の一時変数が多い処理になってしまっています。
def self.get_monthly_count(ids, users)
uids = []
ids.each do |id|
uids.push id.uid.to_s
end
uid = {}
count = 0
users.each do |user|
uid[user] = uids[count]
count += 1
end
uid.to_h
end
１行で片付くんですね、Rubyの場合。
def self.get_monthly_count(ids, users)
users.zip(ids.map{ |id| id.uid.to_s }).to_h
end
まず、文字列にして取り出し、配列に作り直す処理はmapメソッドを使えば１行ですみます。空の配列を作ってインデックス を使用した処理を書いていたら、まずこの手のメソッドを利用することを思い浮かべると最初からすっきりしたコードが書けそうです。そしてその後にzipメソッドを使い、ペア配列を作って最後にハッシュ化させています。
zipパターン
チャックみたいに配列の要素と要素を組み合わせることができるメソッドです。
array_keys = ["Tom", "Bob", "Joge"]
array_values = [167, 156, 181]
zipped = array_keys.zip(array_values)
p zipped # [["Tom", 167], ["Bob", 156], ["Joge", 181]]
hash = zipped.to_h
p hash # {"Tom"=>167, "Bob"=>156, "Joge"=>181}
一時変数が多い
user_names、uid、usersと3つもありますが、こんなに作る必要が無いですね。
def self.get(check_ins)
user_names = []
check_ins.each do |check_in|
uid = check_in.uid
users = community_number_collection.document(uid).get
user_names.push users.data[:nickname]
end
user_names
end
空の配列ができていたら、mapを使って処理し、余計な変数は作らず、１行にまとめてしまいます。
def self.get(check_ins)
check_ins.map do |check_in|
community_number_collection.document(check_in.uid).get.data[:nickname]
end
end
mapとzipを組み合わせて短くする
先述したものを組み合わせて、処理を短くします。
def self.get_total(merged_data, users)
sum = []
merged_data.keys.each do |value|
uid = merged_data[value]["uid"]
sum.push self.where(uid: uid).count
end
sum
total = {}
count = 0
users.each do |user|
total[user] = sum[count]
count += 1
end
total
end
mapとzipでかなり短くなります。ブロックになっている処理を丸ごとsumという変数へ突っ込んでいるのも、発見でした。ブロックは式であり、代入が可能になるということでしょうね。
def self.get_total(merged_data, users)
sums = merged_data.keys.map do |key|
self.where(uid: merged_data[key]["uid"]).count
end
users.zip(sums).to_h
end
より良いハッシュの計算処理
ハッシュは重複を数えてくれるので、集計するのには便利です。しかし、もっと短くかければそれに越したことはないです。
def self.get_this_month(check_ins)
check = {}
check_ins.each do |check_in|
uid = check_in.uid.to_s
uid_count = check[uid] || 0
check[uid] = uid_count + 1
end
check.values.each do |value|
value
end
end
今回、空の配列は使っていないですが、元々の配列の要素を取り出した処理をeachに入れているのなら、mapで望みの配列を作り直して、その配列をeachで回した方がすっきりする。
また、defaultメソッドを使えば、バリューを一旦、全て0で置くことができる。
def self.get_this_month(check_ins)
check = {}
check.default = 0
check_ins.map{ |c| c.uid.to_s }.each do |uid|
check[uid] += 1
end
check.values
end
関数の一般化
下記のコードになる前は、渡されているハッシュのキーがそれぞれ異なっているくらいで、処理自体は、同じことをしていた二つの関数があった。それを抽象化した関数1つにまとめた。
空の配列を用意した後に、keyを指定した空の配列を宣言する(nested = {})し、更にeachの中で、keyに対し、値のないハッシュができる（nested[key] = {}）ようになっている。
def self.label_hash(hash_dictionary, keys)
nested = {}
keys.each do |key|
nested[key] = {}
hash_dictionary.each do |name, hash|
nested[key][name] = hash[key]
end
end
nested
end
呼び出し方として、引数をハッシュに纏める方法を使っている。
上の関数は使われている単語が抽象性の近いものが使用されて、下の呼び出しには具体性の強い単語が使用されている。命名で役割がはっきりと分かれているのがわかる。
labeled_totally_count = CheckIn.label_hash({
"month" => zipped_monthly_count,
"total" => zipped_totally_count
}, zipped_monthly_count.keys)
まとめ
-
一時変数が多い時は、それを消した書き方を想像すること
-
状況に応じてRubyに用意されたメソッドを使う瞬間に気づくこと(特に空の配列作ってインデックスカウントした処理を書いていたら注意)
もう1つ、自分がコードを書いていて、読みづらいコードを書いている問題として、既に使っている名前が別概念として、プログラミング界に根付いているから、熟練ののエンジニアはコードを見た途端にそっちを想起する可能性があり、誤読を誘い、結果、書いたコードは読みにくくなっているというのがよく起こっているので注意しい。