佐々木屋

技術的なことから趣味まで色々書きます

リファクタリングの判断

私がコードをチェックする時、「これは駄目なコードだ」とリファクタリングの対象とする点がいくつかあります。
今回はそれをまとめて紹介しますので、共感できる部分は是非修正してみて下さい。逆に共感できない部分は、何故それが駄目なのかを考えるのも面白いかもしれません。

変数・プロパティ・メソッドが多すぎる

一つのクラスに多くの状態保持(変数やプロパティ)や振る舞い(メソッド)が多いということは、クラスの背負う責務が多すぎる場合がほとんどです。こういった場合は、責務を抽出して分散させる(別クラスにする)ことで解決することができます。

名前が変

メソッドや変数の名前が体をなしていない場合があります。これは非常にコードを見づらくします。例えば、

int i;
int j

などです。全く意味のない変数名は局所なら問題ありませんが、メソッド全体に影響するような場合は正しい名前を付けるべきです。
また、略名や接頭語(プリフィックス)も度々見られますが、これも好ましくありません。

string kbn; //恐らく区分のこと?
string strA; //string型変数全てにstrをつける


一つのメソッドが長すぎる

非常に長いメソッドも、責務を持ちすぎている可能性があります。別メソッドにしたり、リファクタリングすることで改善することがほとんどです。状況にもよりますが私の経験上一つのメソッドはおおよそ2~30行程度(例外処理、空白行除く)でまとまらないと、どこかに設計上問題があると思います。

隠蔽されていない

要は公開しすぎ(public)ということです。
例えばプロパティはgetterとsetterでアクセス範囲を限定できますので、外部から設定されるべきものなのか、読み取りが必要なのか等の吟味が必要です。
メソッドにしても外部から全く呼ばれることがないにも関わらずpublicやprotectedで宣言されていたりします。これではクラス設計とは意図しない不具合を起こしかねません。
オブジェクト指向において、この隠蔽は非常に重要な概念です。

振る舞いが別クラスと重複している

これもよくある現象ですが、似たような処理を引数や戻り値の違いでいくつも別クラスにする、というコードを見かけます。必要であればオーバーロードするか、継承してオーバーライドする、デリゲートする等を検討しましょう。とにかく長い、多いは「害悪」です。

メソッド引数が多すぎる

基本的にメソッドの引数はオペランドのみにしましょう。
sasaki816.hatenablog.com
静的クラスであれば、ある程度オーバーロードで逃げる必要がありますが、通常のクラスであればメソッド設計が誤っている可能性があります。機能を抽出して、プロパティにすべきか、変数にすべきか、引数にすべきかをもう一度検討する必要があります。

条件分岐 if、switch(Select Case)が多すぎる

条件分岐は無いにこしたことはありませんが、どうしても分岐する場合は必要最低限とします。状況によっては三項演算子等も上手に織り交ぜながら記述しましょう。
入れ子についても同様で、入れ子した分だけ可読性が悪くなります。

メソッド内変数が多すぎ

一回しか使用しない変数や、使っていない変数(たまにある)、スコープを間違えた変数等も可読性ばかりかバグの温床になってしまいます。変数として持つべきかどうかの適正な判断が必要です。なお、C#VB.NETは構造化プログラミングが可能ですので、出来るだけ処理を最小限にまとめ、変数スコープもその中で使うようにするべきです。

コメントの応酬

コメントが多すぎるコードも良く見ますが、これも無駄です。というかコメントで説明しなければならないような複雑な処理は別クラス化して責務を分担させましょう。ただコメントがなさ過ぎるのも問題なので、その辺りのさじ加減は必要です。
また、コードをコメントアウトして新旧保存している場合もたまに見かけますが、これも無駄ですのですぐに止めましょう。変更履歴を記録するなどもってのほかです。こういったコードと関係ない部分は別ファイルや別の媒体に保管すべきです。

似ている処理、同じ処理

似ている、同じ処理が色々散らばっている状況です。これは一か所にまとめてクラス化するなり関数化するなりしましょう。
クラスを跨いでいる場合は共通クラスとして定義するか、継承、デリゲートを利用する等を検討しましょう。

処理が分散しすぎ

一つのメソッド内で自作関数が多すぎる状況を言います。関数自体は処理の分散化といった点を考えれば至極当然の結果なわけですが、一度しか使われていない関数や、そもそも関数自体が1~2行と外だしする必要が本当にあるのか?という場合もあります。

.NET Frameworkの機能を使っていない

.NET Frameworkの機能(クラスやインターフェース)があるにも関わらず、それを使用せず自作関数を作ってしまっている場合です。リファレンスなどを有効活用して、まずは.NET Frameworkの機能で実現可能かどうかを調べる「正しい癖」を身につけるべきです。

既存クラス機能では実装レベルで問題(足りない)がある場合はこの限りではありませんが、私の経験上はそれはかなり限られた状況です。