Ruby on Rails

Railsプロジェクトでのリファクタリング 臭うコードを消臭します!

投稿日:

こんにちは。じゅん(@BoNingennnN)といいます。
普段仕事でRubyを使っています。フレームワークはRailsです。

ゼロイチのフェーズのプロジェクトに関わっていると、スピードをとにかく重視することがあります。
その結果、これまでに私は数々の「臭うコード」を生み出してしまいました。

スピードを重視していたとしても、「臭うコード」を生み出さずクリーンなコードを生み出し続ける優秀なエンジニアもいます。
そう。私はまだまだエンジニアとして未熟なので「臭うコード」を生み出してしまっていたのです。

エンジニアになって一年が経とうとしているので、いつまでも未熟とは言っていられません。
これを機にこれまで私が生み出してきた「臭うコード」を消臭していきたいと思います。

1, evalを使って消臭(リファクタリング)してみる

元々のコードがこちら。(メソッド名や変数名などは変えてあります。)

 

このchange_win_numというインスタンスメソッドですが、ほとんど同じ処理を繰り返し書いています。
色(color)によって処理が違うならまだしも、今回の場合やっている処理は「その色のwin_numを+1する」だけで全て共通です。
このように、if文で分岐しているのに繰り返し同じ処理を記述している場合は、evalを使えないか検討してみましょう。(そもそもif文でなくcase ~ whenを使えという声もありました。)

evalの詳しい説明はリファレンスをみていただくとして、簡単に説明すると、evalは第一引数として渡した文字列をRubyプログラムとして実行してくれます。

以下の例をみていただければevalがやっていることが分かると思います。
(文字列 "p message" をevalに渡すとそれを実行してくれるという例)

 

今回の「臭うコード」をリファクタリングするときに、evalはぴったりそうです。
早速evalを使って先ほどのコードをリファクタリングしていきます。

 

めちゃくちゃスッキリしましたね。
colorをいちいちベタがきしていたらそこでバグを埋め込んでしまう可能性もありますが、これなら安心です。万が一他の色が増えたとしても、このメソッドを変更する必要はありません。

(若干の懸念点としては、カラムに存在しないcolorをこのメソッドの引数として渡された場合、エラーが起きてしまいます。)

(また、そもそもProfileに色ごとの勝利数のカラムを持たせていることへの懸念も残ります。このままだと色が増えて行ったらそれだけカラムを増やさないといけなくなってしまいます。それを考慮すると、「profile_id」と「color」と「win_num」の3つのカラムをもつ別テーブルを作った方がよりいい気がします。が今回はevalを使ったリファクタリングのみで一旦やめておきます。)

戻り値が複数のものを1つにしてみよう

Rubyでは、戻り値を複数持つことができます。
この場合、メソッドの呼び出し側でも複数の受け皿(変数)を用意しておく必要があります。

 

呼び出す方はこんな感じで呼び出せます。

 

今回の例だと4つの戻り値があります。これがさらに増えると、メソッドの呼び出し元のコードがかなり汚くなります。
呼び出すたびに、その戻り値分の受け皿を用意する必要があるからです。

また、戻り値の順番も気にしないといけなくなるので、これも厄介です。
今回の例だと、1つ目の戻り値が足し算の結果で、2つ目が引き算で、3つ目が掛け算で、4つ目が割り算ということを呼び出し元が認識しておく必要があります。これを間違えると事故が起きる可能性があります。

これらの問題を解消する為に、戻り値を1つにしてみましょう。

 

やっていることは一緒ですが、結果的に戻り値がcalculated_resultという1つのハッシュだけになりました。

呼び出し元を見てみましょう。

 

戻り値が1つなので、呼び出す際にかなりスッキリしました。また、戻り値の順番を機にする必要もなくなりました。
これで多少は消臭(リファクタリング)できたかと思います。
今後も随時追加していきます!(週に一個のペースで追加していく予定です)

-Ruby on Rails

Copyright© 独学エンジニアの記録帳 , 2019 AllRights Reserved.