Java言語で学ぶ リファクタリング入門 を読んで.......

http://www.hyuki.com/ref/
書籍『Java言語で学ぶリファクタリング入門』を読み始めました。

あっ、危険なコード書いてるじゃん。と思った。

第７章、クラスによるタイプコードの置き換え
第８章、サブクラスによるタイプコードの置き換え
第１０章、例外によるエラーコードの置き換え

を読んで

Factoryクラスを使ってServiceのインスタンスを返却しているのだけれど

インスタンス生成のメソッドはこんな感じ

public xxxxService create(String mode, RequestDto requestDto) {

  ProcessService processService = null;
  
  if (mode.equals(Constant.CMDCD1)){

    processService = xxxxService4xxxxxA;
    
  }else if (mode.equals(Constant.CMDCD2)){
    
    processService = xxxxService4xxxxxB;
  
  }else {
    
    String result = parameterCheckService.check(requestDto);
    
    if (result.equals(parameterCheckService._SUCESS) ){

      processService = xxxxService4xxxxxC;

    } else {
      return null;
    }
  }
  return processService ;

}

これはヒドイひどいコードを書いていた自分だなぁ。と思った。

• 改善点１：引数のString modo　をmodoを保持するクラスに置き換えることをしなければならない
• 改善点２：if文でServiceのインスタンスを返却しているが、そのif文を排除しなくてはならない
• 改善点３：return null; を例外を発生させるようにしなくてはならない

改善点１

引数のString modo　をmodoを保持するクラスに置き換えることをしなければならない

とは、String modo　のmodo　を表すクラス（ここではModoType.java＝ModoTypeクラスと呼ぶ）を作成する。

public xxxxService create(String mode, RequestDto requestDto) 
public xxxxService create(ModoType modeType, RequestDto requestDto) 

としてModoTypeクラス型の引数で受けるようにする
ModoTypeクラスは、以下のようなイメージ

public class ModoType {
  
  private String _typCode;
  
  private ModoType(String s){
    this._typCode = s;
  }
  
  public String getModoType(){
    return this._typCode;
  }
  
  public static final ModoType A = new ModoType("A");
  public static final ModoType B = new ModoType("B");
  public static final ModoType C = new ModoType("C");
  
}

などとして、Factoryを使う側は、

ProcessService processService = Factoryクラスの.create(ModoType.A, requestDto);

こんな感じにする.

改善点２

if文でServiceのインスタンスを返却しているが、そのif文を排除しなくてはならない

if文が多くなったり、非オブジェクト指向となってしまう危険があるので
if文の変わりに、HashMapを使ってみようと思う.
MapにkeyとしてString,要素（値）としてオブジェクトを入れる.
指定したkey(ここではmodoType.getModoType()で得られる値)がMapに存在していれば
そのkeyに該当するオブジェクトを返却.という作りに変えてみる.

public class ProcessFactoryImpl {
  
  private HashMap<String ,ProcessService> serviceMap = new HashMap<String ,ProcessService>();
  
  public ProcessFactoryImpl(){
    //mapにProcessServiceサービスクラスの実装クラスのインスタンスを保持するため
    //ProcessServiceサービスクラスの実装クラスをインジェクションする必要あり
    //ここでは、その処理は割愛。
    setUped();
    
  }
  
  public void setUped(){
    serviceMap.put("A", xxxxService4xxxxxA);
    serviceMap.put("B", xxxxService4xxxxxB);
    serviceMap.put("C", xxxxService4xxxxxC);
  }
  
  public DoProcessService create(ModoType modoType , RequestDto requestDto) {

    if (serviceMap.containsKey(modoType.getModoType())){
      
      return serviceMap.get(modoType.getModoType());
      
    }else{
      
      String result = parameterCheckService.check(requestDto);
      
      if (result.equals(RequestParameterCheckService._SUCESS) ){
          return serviceMap.get(modoType.getModoType());
      } else {
        return null;
      }
    }
  }
}

改善点３

return null; を例外を発生させるようにしなくてはならない

改善前のソースでは、return processService ;　と
returnする箇所がフラグ扱いとなっていたので、それを即returnするように修正

引数が不正な値なので、IllegalArgumentException　をthrowするなど

public DoProcessService create(ModoType modoType , RequestDto requestDto) {

    if (serviceMap.containsKey(modoType.getModoType())){
      
      return serviceMap.get(modoType.getModoType());
      
    }else{
      
      String result = parameterCheckService.check(requestDto);
      
      if (result.equals(RequestParameterCheckService._SUCESS) ){
          return serviceMap.get(modoType.getModoType());
      } else {
        //return null;
        throw new IllegalArgumentException("引数が不正です.modoType= " + modoType.getModoType())
      }
    }
  }

RutimeExceptionのサブクラスである、IllegalArgumentException　をスローするのでもよいし、Exceptionのサブクラス（例えば、NotSupportedModoTypeException）を作成して、そのクラスの例外をスローするってのもありだけど、その場合は、createメソッドにてthrows節が必要なのと、上位クラスでその例外をcatchする必要がある。
こうゆうのって要件によるんだろうけど createメソッドを呼び出す前に、不正な値かどうかチェックするのが妥当なのかな。
IllegalArgumentExceptionなどのRuntimeExceptionのサブクラスは実行時例外なので明示的にcatchする必要がないから、その例外をcathするのか、どう処理するのか、わかりにくくなるから注意しないといかん。今回のように、不正な場合に、nullを返却するより断然よい。nullを返却すると、呼び出し側で、nullかどうか判定しないといけないし、もしnullの判定がなければ「ぬるぽ」になってしまうわけで、ログ見ても解析するまで時間かかりそうだし。