【rails】【リファクタリング】俺が出会った「これはあきませんやん。」

今月の頭からリファクタリングをしています。
現在はRails、Rubyに限ったリファクタリングではなく、
一般的な以下のリファクタリングを実行しています。

冗長的ロジックを抽出し、共通ロジックに変更
変数の移動
継承を委譲に置き換える
ダウンキャストをカプセル化する
引数オブジェクトの導入
クラスメソッド属性の名称を変更する
不要コメントの削除
不適切な変数の変更
インデントの修正
変更箇所のテスト

その中で以下の衝撃的なコードに出会いました。

１・変数名が個人名

「これはあきませんやん。」
通常、変数名というのは他のエンジニアでも理解出来るような内容にすべきです。
弊社にはコーディング規約なるものがまだ存在しないので、
感覚になってしまうのですが、さすがに個人名を入れてるコードがあると思いませんでした。。。

誤：

 @g08m11 =Question.find(params[:id])

正：

 @question = Question.find(params[:id])

２・if falseを放置

「これはあきませんやん。」
バグ発生時、一時的にバグを回避するためにコメントアウトする場合がありますが、
そのバグ改修時にそのコメントアウトしたコードは不要になります。
しかし、そのコードが放置されてました。

誤：

if false
  @user = user  
  @reservation = @user.reservations.find(params[:reservation_id])  
  @reservation_contact = @reservation.reservation_contacts.build(params[:reservation_contact])  
end
@reservation_contact.post_name = AppConfig[:user_post_name]

正：

@reservation_contact.post_name = AppConfig[:user_post_name]

３・テストコードが放置

「これはあきませんやん。」
必要情報がDBまたはパラメータから取得出来ているかを確認する際に、
一時的にview側にテストコードを書く場合があります。
しかし、そのテストコードが放置されているのは。。。

誤：

<% @test = params[:id] %>

正：

４・不要ファイルが放置

「これはあきませんやん。」
変更箇所を比較する際に既存コードを残すという意味でファイル自体をバックアップする場合がありますが、変更し、テストをして仕様通りであればそのファイルは不要であり、
削除する必要があります。
しかし、それがなされていませんでした。

誤：

vi app/view/user/index.html.erb_bk

正：

正常に稼働しているコードやアプリに関して修正をするのは懸念されがちですが、
今後も不要なコードや「コードの臭い(*1)」を感じたら勇気を持ち、
積極的にリファクタリングをしていきます。

本記事以外にもリファクタリングの必要性を説いてる記事がありましたので、
ご紹介します。

なぜリファクタリングは必要なのか？

http://www.atmarkit.co.jp/ait/articles/1103/08/news096.html

*1
http://ja.wikipedia.org/wiki/%E3%82%B3%E3%83%BC%E3%83%89%E3%81%AE%E8%87%AD%E3%81%84

追伸：

ついに日本でも11/1(金)にジョブス映画がロードショーです！

Instagram

Web + Life Hack

〜True But Useless〜