リファクタリング入門

2018/12/8 program

　リファクタリングという、悪魔の壺に手を突っ込む行為は、時に耐え難い痛みを伴うことかもしれません。

リファクタリングとは

　リファクタリングとは、変更容易性や可読性の向上を目的として、振る舞いを変更することなくソースコードを内部的に作り替えることです。

なぜリファクタリングが必要か

　チーム開発は、全員が熟練のエンジニアではありません。時に、目を覆いたくなるような書き方をしているソースコードを見かけることもあるでしょう。

　彼らに悪意があるわけではありません。一生懸命、可読性の高いソースコードを書こうとした結果、どうしようもなく生み出されてしまったものがそれだっただけで、誰が悪いわけでもありません。

　しかし、そのソースコードは大病を孕んだ病魔です。放置していると、取り返しのつかない結果となることは誰もが知っていることです。

　リファクタリングは、たとえるなら、ソースコードに対する「治療」です。これを疎かにすると、病は静かにプロジェクトを蝕みます。そして症状が出たころには、取り返しのつかない状態に陥っていることでしょう。

コードを修正する

　経験豊富なエンジニアであれば、コーディングの作法が自然と身についているものですが、リファクタリングを初めて行おうとするエンジニアは、なかなかどこから手を付ければよいかわからないものです。

　それでは、どこをどう変更すればよいか見極めていきましょう。いくつか例をご紹介します。

重複コード

　よくあるのが、コピー＆ペーストを利用したコーディングです。最初のコーディング時は何も考えなくてよいのですが、変更時に影響箇所が一気に広がるため、地獄を見ることになるでしょう。

　2つ以上のメソッドで同じコードが出てきたら要注意となります。以下の例は、ユーザーの権限を調べて、管理者であれば各ウィンドウを表示させるという擬似コードです。

public void showUserManageWindow(User user){
  if(user != null && user.id == ADMIN_ID){
    userManageWindow.show();
  }
}

public void showSettingsWindow(User user){
  if(user != null && user.id == ADMIN_ID){
    settingsWindow.show();
  }
}

　まず、明らかな重複コードがありますから、これを切り出してみましょう。

if(user != null && user.id == ADMIN_ID)

　nullチェックとIDがadminのIDであるかのチェックを行っていますね。これは、以下のようなメソッドに集約できます。

public boolean isAdmin(User user){
  return (user != null && user.id == ADMIN_ID);
}

　元のメソッドを修正してみましょう。

public void showUserManageWindow(User user){
  if(isAdmin(user)){
    userManageWindow.show();
  }
}

public void showSettingsWindow(User user){
  if(isAdmin(user)){
    settingsWindow.show();
  }
}

　ずいぶんとすっきりしましたね。

　仮に、adminかどうかの判定ロジックを変更したくなった場合、以前のままだと、双方のメソッドを変更しなければなりませんでした。しかし、isAdmin()という判定関数を新たに作成することで、adminかどうかの判定ロジックを集約することができました。

　結果として、admin判定ロジックに変更が生じた場合には、isAdmin()を変更するだけでよくなりました。

　次に、windowオブジェクトのshowメソッドを呼んでいる部分に着目してみます。一見すると、これ以上共通化できなさそうな部分ですが、インターフェースを使って以下のようにできます。

public interface Window{
  public void show();
}

public class userManageWindow implements Window{
  public void show(){
    System.out.println("User manage window was shown.");
  }
}

public class settingsWindow implements Window{
  public void show(){
    System.out.println("Settings window was shown.");
  }
}

public void showAdminWindow(Window window, User user){
  if(isAdmin(user)){
    window.show();
  }
}

public void showUserManageWindow(User user){
  showAdminWindow(userManageWindow, user);
}

public void showSettingsWindow(User user){
  showAdminWindow(settingsWindow, user);
}

　コードが増えた気がしますね。しかしこれは、コード量の増加と引き換えに、変更容易性の向上というメリットを得ています。

　例えば、「admin専用ウィンドウを表示する際は、必ずユーザー名をログに出力する」などの仕様が追加された場合、先ほどの例だと2つの箇所を変更する必要がありましたが、今回は、showAdminWindow()に処理を追加するだけでよくなりました。

　このように、変更容易性を追求することは、リファクタリングの目的のひとつでもあります。

変数、メソッド、クラス名

　名前を見てみましょう。「名は体を表す」という言葉がありますが、これはコードにも当てはまります。以下のコードを見てください。

public void getName(String name){
  this.name = name;
}

　名前からはnameを取得するメソッドであることがひしひしと伝わってきますが、その実態はただのセッターメソッドです。

　これは極端な例ですが、コードを読んで明らかに異常を感じるような命名は慎むべきです。本来であればコードレビューで排されるべきものではありますが、実際のところ、こういった例はいくつもありました。

　よくあるのは、boolean型を返すメソッドです。

public boolean checkAuthority(User user){
  return user.role.equals(ADMIN);
}

　まず、「checkAuthority」という名前ですが、この名前だけでAdmin権限があるかどうかを確認するメソッドであるということを推測できるでしょうか？

　一般的な解釈であれば、「権限チェック」つまり、権限があるかどうかをチェックするものである、と推測はできますが、チェックという言葉は非常にあいまいです。通常であれば、権限があるとtrueを返すものと推測できますが、falseを返すようなメソッドである可能性も否定できません。

　こういった命名は避けるべきです。以下のようにすべきでしょう。

public boolean isAdmin(User user){
  return user.role.equals(ADMIN);
}

　こういうものは、まずif文を書いてみて考えましょう。

if(checkAuthority(user)){}

　日本語に訳すと、「もし権限チェックなら」となります。意味が通りませんよね？

if(isAuthorized(user)){}

　これなら、意味が通りそうですね。

　このように、英語としてそのまま読んだとき、メソッドの内容をしっかりとあらわしているような命名を心掛けます。

コストのかかる処理

　これは、言語やライブラリの内部仕様などに深い知見を持っていないと、なかなか見つけ出すのは難しいかもしれません。有名なのは、Stringの結合です。以下のコードをご覧ください。

String str = "";
for(int i = 0; i < 10000; i++){
  str += "a";
}
System.out.println(str);

　String同士の結合演算は、非常にコストの掛かる処理です。Stringオブジェクトはimmutableですから、毎回Stringの内部配列の解析と新たなインスタンス作成が行われます。これを解決するには、StringBuilderを活用します。

StringBuilder sb = new StringBuilder();
for(int i = 0; i < 10000; i++){
  sb.append("a");
}
System.out.println(sb.toString());

　反面、以下のような例は修正する必要がありません。

String url = "http://" + HOST + "/" + PATH;

　これは、Javaコンパイラにより最適化されるためです。

　このように、パフォーマンス面のリファクタリングを行う場合は、内部仕様に深く精通していないと、かえってパフォーマンスや可読性の低下を招く可能性があります。

マジックナンバー

　時折、「魔法の数字」に悩まされることがあります。

public boolean isAdmin(User user){
  return user.roleCode == 1 || user.roleCode == 4;
}

　この1という数字は何でしょうか。メソッド名から、「ひとつはAdminのロールコード」だと推測できます。しかし、それが本当にAdminのロールコードであるかは、仕様書と照らし合わせなければわかりません。

　余談ですが、小学校のとき、国語のテストで「作者の気持ちを述べましょう」という問題がありました。子供心に、「作者の気持ちなど作者以外にわかるか！」と思ったものですが、このコードでも同じことが言えます。

　コードを書いた人の意図は、書いた人にしかわかりません。以下のように修正しましょう。

public static final int ADMIN = 1;
public static final int SUPER_USER = 4;

public boolean isAdmin(User user){
  return (user.roleCode == ADMIN || user.roleCode == SUPER_USER);
}

　魔法の数字に名前が付いたことにより、処理の意図が明確になりました。

過剰なネスト

　プログラム上必要であることは多いでしょう。しかし、以下のコードを見てみてください。

public static final int ADMIN = 1;
public static final int SUPER_USER = 4;

public boolean isAdmin(){
  if(user != null){
    if(user.roleCode == ADMIN){
      return true;
    } else if(user.roleCode == SUPER_USER) {
      return true;
    } else {
      return false;
    }
  } else {
    return false;
  }
}

　とんでもなく可読性の悪いコードです。if～elseステートメントは、このような階層の難読化を招き、一見どのような処理をしているのか分からなくします。特に、elseステートメントは使わなくてよい場面が多く、上記コードは以下のようにリファクタリングできます。

public static final int ADMIN = 1;
public static final int SUPER_USER = 4;

public boolean isAdmin(){
  if(user == null){
    return false;
  }

  if(user.roleCode == ADMIN){
    return true;
  }

  if(user.roleCode == SUPER_USER) {
    return true;
  }

  return false;
}

　ネストを一切含んでいないので、コードの流れが明確になり、可読性の高いコードになりました。

まとめ

　リファクタリングの対象となりうるコードについて、その改善例とともに、いくつかまとめてみました。このほかにも様々な改善すべきコードはありますが、挙げていくとキリがないので、この辺にさせて頂きます。

コメント

  1. 匿名 より:
    2018-12-07 14:56

    最後のソースは
    if(user == null){
    であるべきでは？

    返信
    • tam-tam より:
      2018-12-08 11:47

      ご指摘ありがとうございます！
      修正いたしました！

      返信