続々々々々：僕の好きなコードの書き方

これの続きー。

僕の好きなコードの書き方 - Mitsuyuki.Shiiba
続：僕の好きなコードの書き方 - Mitsuyuki.Shiiba
続々：僕の好きなコードの書き方 - Mitsuyuki.Shiiba
続々々：僕の好きなコードの書き方 - Mitsuyuki.Shiiba
続々々々：僕の好きなコードの書き方 - Mitsuyuki.Shiiba

今日はここから

Coinクラス？

Coinクラスを作りたいなーと昨日思ったけど、それはやり過ぎ感あるのでやめとこう。

guava導入！

Preconditionsをstatic importして使おう。
この部分を、

 public Result execute(Input in) {
    if (in == null) {
      throw new NullPointerException("In is null.");
    }

    try {
      return a(in.getSelected(), in.getCoins());
    } catch (Exception e) {
      LOG.error(e);
      return createResult(in.getCoins(), null);
    }
  }

  Result a(Integer aProductId, List<Integer> aInputCoins) {
    if (not(isValidProductId(aProductId))) {
      throw new IllegalArgumentException("ProductId is not valid.");
    }

    if (not(isValidInputCoins(aInputCoins))) {
      throw new IllegalArgumentException("InputCoins is not valid.");
    }

    Product product = dao.findById(aProductId);
    if (product == null) {
      throw new IllegalArgumentException("Product doesn't exist.");
    }

    return b(aInputCoins, product);
  }

こうする。

  public Result execute(Input in) {
    checkNotNull(in, "In is null.");

    try {
      return a(in.getSelected(), in.getCoins());
    } catch (Exception e) {
      LOG.error(e);
      return createResult(in.getCoins(), null);
    }
  }

  Result a(Integer aProductId, List<Integer> aInputCoins) {
    checkArgument(isValidProductId(aProductId), "ProductId is not valid.");
    checkArgument(isValidInputCoins(aInputCoins), "InputCoins is not valid.");

    Product product = dao.findById(aProductId);
    if (product == null) {
      throw new IllegalArgumentException("Product doesn't exist.");
    }

    return b(aInputCoins, product);
  }

むむむ。。。

こうして見てみると、isValidProductIdとisValidInputCoinsの中を考えなおしたくなった。
こうしてみるか。

CollectionUtils使いたかったので、commons-collections4を追加。
例外のメッセージを変えた。
事前条件って感じでもないんだけど、product == null の部分もPreconditions使うことにする。

  Result a(Integer aProductId, List<Integer> aInputCoins) {
    checkProductId(aProductId);
    checkInputCoins(aInputCoins);

    Product product = dao.findById(aProductId);
    checkArgument(product != null, "Product doesn't exist.");

    return b(aInputCoins, product);
  }

  void checkProductId(Integer aProductId) {
    checkNotNull(aProductId, "ProductId is null.");

    int productId = aProductId.intValue();
    checkArgument(0 < productId && productId < 11, "ProductId is invalid.");
  }

  void checkInputCoins(List<Integer> aInputCoins) {
    checkNotNull(CollectionUtils.isNotEmpty(aInputCoins), "InputCoins is null or empty.");
    checkArgument(aInputCoins.size() < 101, "InputCoins is not valid.");
  }

ふむ。キモいかと思ったけど、見た感じ悪くないな。

細かいところだけど

checkArgumentで不等号使ってるんだけど、これ「0より大」より「1以上」の方が読みやすいかなと思うのでそうする。

  Result a(Integer aProductId, List<Integer> aInputCoins) {
    checkProductId(aProductId);
    checkInputCoins(aInputCoins);

    Product product = dao.findById(aProductId);
    checkArgument(product != null, "Product doesn't exist.");

    return b(aInputCoins, product);
  }

  void checkProductId(Integer aProductId) {
    checkNotNull(aProductId, "ProductId must not be null.");

    int productId = aProductId.intValue();
    checkArgument(1 <= productId && productId <= 10,
        "ProductId must be in the range 1-10.");
  }

  void checkInputCoins(List<Integer> aInputCoins) {
    checkNotNull(CollectionUtils.isNotEmpty(aInputCoins),
        "InputCoins must not be null nor empty.");
    checkArgument(aInputCoins.size() <= 100,
        "InputCoins size must be up to 100.");
  }

ってところでaメソッドまでは満足した。次はbメソッド！

ここから

  Result b(List<Integer> inputCoins, Product product) {
    List<Integer> coins2 = new ArrayList<>();
    int sum = processCoins(inputCoins, coins2);

    if (checkPrice(sum, product)) {
      return createResult(inputCoins, null);
    }

    processCoins2(coins2, sum, product);
    return createResult(coins2, product);
  }

proessCoinsにinputCoinsを渡したら、coins2に使えないコインが詰められて、
戻り値としては使えるコインの合計値が返される。っぽい。
で、その合計値をチェックしてOKだったら処理して返す。

変数名から

か。まずは、bメソッドのパラメータ名と変数名の名前付けからやってみよう。

  Result b(List<Integer> aInputCoins, Product aProduct) {
    List<Integer> returnCoins = new ArrayList<>();
    int sumOfValidCoins = processCoins(aInputCoins, returnCoins);

    if (checkPrice(sumOfValidCoins, aProduct)) {
      return createResult(aInputCoins, null);
    }

    processCoins2(returnCoins, sumOfValidCoins, aProduct);
    return createResult(returnCoins, aProduct);
  }

ちょっと読みやすくなった。

processCoins

このメソッドが気持ち悪いのは、2つの処理を同時にやってるところかな。
処理的にはここでは、sumOfValidCoinsが分かれば良さそう。
で、お釣りの部分はprocessCoins2の方に任せることができるな。

まずはprocessCoinsから。ここから

  int processCoins(List<Integer> inputCoins, List<Integer> coins2) {
    int sum = 0;
    for (Integer coin : inputCoins) {
      if (coin == 1) {
        sum += 0;
        coins2.add(coin);
      } else if (coin == 5) {
        sum += 0;
        coins2.add(coin);
      } else if (coin == 10) {
        sum += 10;
      } else if (coin == 50) {
        sum += 50;
      } else if (coin == 100) {
        sum += 100;
      } else if (coin == 500) {
        sum += 500;
      }
    }
    return sum;
  }

こうする。まだまだキモいけど一歩ずつ。

  int calcSumOfValidCoins(List<Integer> aInputCoins) {
    int sumOfValidCoins = 0;
    for (Integer coin : aInputCoins) {
      if (coin == 1) {
        sumOfValidCoins += 0;
      } else if (coin == 5) {
        sumOfValidCoins += 0;
      } else if (coin == 10) {
        sumOfValidCoins += 10;
      } else if (coin == 50) {
        sumOfValidCoins += 50;
      } else if (coin == 100) {
        sumOfValidCoins += 100;
      } else if (coin == 500) {
        sumOfValidCoins += 500;
      }
    }
    return sumOfValidCoins;
  }

で、中を、もうちょっとマシにする。こうかな。

  static final ImmutableSet<Integer> VALID_COINS = ImmutableSet.of(10, 50, 100, 500);

  int calcSumOfValidCoins(List<Integer> aInputCoins) {
    return aInputCoins.stream()
        .filter(coin -> VALID_COINS.contains(coin))
        .mapToInt(i -> i)
        .sum();
  }

あぁ！もう時間だ！
TODO: processCoins2でreturnCoins計算する。

今日はここまでー。ValidよりAvailableの方がいいのかなー。とか思いながら。

追記

続き書いた。bufferings.hatenablog.com