コードレビューでありそうなやりとり

この画像はBackground Image Generatorで生成したものです

オンライン会議用の背景画像を生成するやつを作った - hitode909の日記

この文章の目的

放置していたテキストを発掘してるだけ
具体的なことは何も覚えてない
たぶん他の人がコードレビューしている様子を見て、自分ならこうかな、と書いていたんだと思う
昔の自分はちゃんとフィードバック出来ていたのかは気がかり

コードレビューをさせていただく中で「こうしたほうがいいと思うよ」という内容がいくつかあったのでまとめてみます。

コードレビューの雑なまとめ : シェルスクリプト(主に bash)

テキストファイルを一行ずつ処理するようなやつ

提案されたコードはこちら。

全行処理するために行数が欲しい
それぞれの行はカンマ区切り文字列を分解したい

# $1 はファイル名
row_num=$(cat $1 | wc -l)
for ((i=1 ; i<=${row_num} ; i++));
do
  row=$(sed -n $((i))P $1);
  arr=(`echo $row | tr -s ',' ' '`);
  from=${arr[0]};
  to=${arr[1]};
  # ... いろいろ続いていく
done

修正の提案はこちら。

行単位で繰り返すだけならわざわざ sed を使う必要はないと考え $(cat $1) への変更を提案
- ❎ それぞれの行に空白文字が含まれる場合を考慮してないので筋が悪い
  - 対象ファイルのデータフォーマットを確かめておかないといけない
  - 提案する側には当然のことなのでわざわざ書いてないだけかもしれない
配列変数を作ること自体に意味があるわけじゃなかったので文字列ヒアドキュメントへの変更を提案
- ✅ echo とパイプが減ったから見通しが良くなったような気がする

for row in $(cat $1);
do
  from=$(cut -d , -f 1 <<< $row);
  to=$(cut -d , -f 2 <<< $row);
done

コードレビューの雑なまとめ : 製品コード(主に Java)

キャストは必ずチェックする

提案されたコードはこちら。

(既存のコードより推測)
- conditionA が満たされるとき、source は A のインスタンスになるらしい(ソースコードを辿って確認した)
conditionB が追加されたみたい
- こちらも source が B のインスタンスになるらしい

  // 既存のコード
  if (conditionA) {
    A aObject = (A) source;
    return aObject.getKey();
  }

  // 追加したコード
  if (conditionB) {
    B bObject = (B) source;
    return bObject.getKey();
  }

修正の提案はこちら。

instanceof 演算子でチェックしてからキャストしましょう
- キャストの失敗は実行時エラーになる (JVM が ClassCastException をスローする)
- 動いてるから、とか、テストがあるから、とか、本来頼る必要のないところで保証しても仕方ない
「既存のコードベースからコピーしました、なんでそうしてるかは知りません」はやめましょう
- 同じロジックの重複が許されるのはそれなりに理由がある場合だけだと思います
- 「じゃあ既存のコードベースと同じだけテストしたんですか」と聞かれたらちゃんと答えられるでしょうか

  // 既存のコード
  if (conditionA) {
    if (source instanceof A) {
      A keyObject = (A) source;
      return keyObject.getKey();
    }
  }

  // 追加したコード
  if (conditionB) {
    if (source instanceof B) {
      B keyObject = (B) source;
      return keyObject.getKey();
    }
  }

配列オブジェクトの拡張

提案されたコードはこちら。

メソッド設計の都合で文字列引数と可変長引数を連結した配列オブジェクトにしたいらしい

    public static String getMessage(String format, String apiName, Object... params) {
        List<Object> paramList = Arrays.stream(params).collect(Collectors.toList());
        paramList.add(0, apiName);
        return String.format(format, paramList.toArray());
    }

修正の提案はこちら。

Stream 同士は連結できるので利用しましょう

    private static String getMessage(String format, String apiName, Object... params) {
        Object[] args = Stream.concat(Stream.of(apiName), Stream.of(params)).toArray();
        return String.format(format, args);
    }

提案しなかった別案。

apiName という引数の型は重要じゃなさそうなので可変長引数に押し込んでいいはず

    private static String getMessage(String format, Object... args) {
        return String.format(format, args);
    }

`org.apache.http.HttpStatus` より `org.springframework.http.HttpStatus` を使う

提案されたコードはこちら。

Web API の応答コードに対してエラーメッセージを生成したいようだ

import `org.apache.http.HttpStatus;

    public static String getHttpStatusMessage(String apiName, int httpStatus) {
        String format;

        switch (httpStatus) {
        case HttpStatus.SC_OK:
            return "";
        case HttpStatus.SC_CONTINUE:
        case HttpStatus.SC_SWITCHING_PROTOCOLS:
        case HttpStatus.SC_PROCESSING:
        case HttpStatus.SC_CREATED:
        case HttpStatus.SC_ACCEPTED:
        case HttpStatus.SC_NON_AUTHORITATIVE_INFORMATION:
        case HttpStatus.SC_NO_CONTENT:
        case HttpStatus.SC_RESET_CONTENT:
        case HttpStatus.SC_PARTIAL_CONTENT:
        case HttpStatus.SC_MULTI_STATUS:
            format = HTTP_STATUS_UNEXPECTED_SUCCESS;
            break;
        case HttpStatus.SC_NOT_FOUND:
            format = HTTP_STATUS_404;
            break;
        default:
            format = HTTP_STATUS_ERROR;
            break;
        }

        return getMessage(format, apiName, httpStatus);
    }

修正の提案はこちら。

org.apache.http.HttpStatus はほとんど素のままなので、少し賢い org.springframework.http.HttpStatus を使うようにしましょう
- org.springframework.http.HttpStatus は Spring MVC の提供する enum です

import org.springframework.http.HttpStatus;

    public static String getHttpStatusMessage(String apiName, int httpStatus) {
        String format;
        HttpStatus status = HttpStatus.valueOf(httpStatus);
        if (status == HttpStatus.OK) {
            return "";
        } else if (status.is1xxInformational() || status.is2xxSuccessful()) {
            format = HTTP_STATUS_UNEXPECTED_SUCCESS;
        } else if (status == HttpStatus.NOT_FOUND) {
            format = HTTP_STATUS_404;
        } else {
            format = HTTP_STATUS_ERROR;
        }

        return getMessage(format, apiName, httpStatus);
    }

有る無しを `boolean` 変数に反映するだけなら条件分岐する必要はない

提案されたコードはこちら。

配列オブジェクトへ安全にアクセスするため条件分岐している

  boolean needSkip = false;
  if (parameters.length > 1) {
    needSkip = SKIP_VALUE.equals(parameters[1]);
  }

修正の提案はこちら。

先頭要素、という制約がないなら Stream#anyMatch で判断するほうが分かりやすいと思います

  boolean needSkip = Stream.of(parameters).anyMatch(SKIP_VALUE::equals);

コレクションクラスのコンストラクタを知ろう

提案されたコードはこちら。

引数の Map にいろいろ追加したいらしい

  private List<Something> method(Map<String, Object> param) {
    Map<String, Object> copyParam = new HashMap<>();
    param.forEach(copyParam::put);
    copyParam.put("k1", v1);
    copyParam.put("k2", v2);
    copyParam.put("k3", v3);
    copyParam.put("k4", v4);
    copyParam.put("k5", v5);
    // copyParam を使った処理…

修正の提案はこちら。

HashMap には複製したい Map を引数にとるコンストラクタがあります

  private List<Something> method(Map<String, Object> param) {
    Map<String, Object> copyParam = new HashMap<>(param);
    copyParam.put("k1", v1);
    copyParam.put("k2", v2);
    copyParam.put("k3", v3);
    copyParam.put("k4", v4);
    copyParam.put("k5", v5);
    // copyParam を使った処理…

提案しなかった別案。

分かりやすさは改善しないので無理に使わないほうがいいと思います

  private List<Something> method(Map<String, Object> param) {
    Map<String, Object> copyParam = new HashMap<>(param);
    copyParam.putAll(
      Stream.of(new Object[][]{
        { "k1", v1 },
        { "k2", v2 },
        { "k3", v3 },
        { "k4", v4 },
        { "k5", v5 },
      })
      .collect(Collectors.toMap(v -> v[0].toString(), v -> v[1]))
    );
    // copyParam を使った処理…

Apache HttpComponents Client の発生する例外を捕まえたい

Apache HttpComponents Client

こういう既存実装(ログ出力にこだわりすぎだと思います)についてのやりとり。

疑問
- 外側の catch 節に並んでる例外はどれも ResponseHandler#handleResponse で RuntimeException としてスローされてくるんじゃないか
  - だとするとすべての catch 節を通過して最後の Exception に到達してしまうんじゃないか
- このままで大丈夫なんでしょうか :question:
回答
- ClientProtocolException, UnknownHostException, ConnectTimeoutException, SocketTimeoutException をスローするのは httpClient#execute だから、すべて RuntimeException になることはありません
- 応答を受信した後で異常が発生したら、RuntimeException をスローします
  - 正常応答(200 OK)だけど本文が壊れていて EntityUtils.toString や XmlUtils.deserialize が非検査例外をスローする可能性を考慮してるのかも
  - 正常応答以外の制御を本文が壊れている場合と合わせたかったのかも

        try (CloseableHttpClient httpClient = HttpClientBuilder.create().setDefaultRequestConfig(requestConfig).build()) {
            SomeObject result = httpClient.execute(request, new ResponseHandler<SomeObject>() {
                @Override
                public SomeObject handleResponse(HttpResponse response) throws IOException {
                    StatusLine statusLine = response.getStatusLine();
                    String statusMessage = getHttpStatusMessage(statusLine.getStatusCode());
                    if (statusMessage.isEmpty()) {
                        String xmlString = EntityUtils.toString(response.getEntity(), StandardCharsets.UTF_8);
                        return XmlUtils.deserialize(xmlString, SomeObject.class);
                    }
                    throw new RuntimeException(message);
                }
            });
            return result;

        } catch (ClientProtocolException e) {
            LOG.error(e.getMessage(), e);
            throw new ApplicationException(e);
        } catch (UnknownHostException e) {
            LOG.error(getUnknownHostMessage());
            throw new ApplicationException(e);
        } catch (ConnectTimeoutException e) {
            LOG.error(getConnectionTimeoutMessage());
            throw new ApplicationException(e);
        } catch (SocketTimeoutException e) {
            LOG.error(getSocketTimeoutMessage());
            throw new ApplicationException(e);
        } catch (Exception e) {
            LOG.error(e.getMessage(), e);
            throw new ApplicationException(e);
        }

Spring Framework の仕組みを学ぶ

相談されたコードはこちら。

外部から呼び出してるメソッド xxxx から、@Transactional を指定したメソッド yyyy を呼び出してるのにトランザクションが開始せず、エラーになってしまうとか。

@Component
public class XXXService {

    public void xxx() {
        // something
        yyyy() // XXX throws Exception
    }

    @Transactional
    public void yyyy() {
        // some database processing
    }
}

発生している現象について
- Spring Framework は同一クラス(インスタンス)のメソッド呼び出しに介入しないため、トランザクションを開始するきっかけがない
- だからトランザクションが開始しない
解決策
- メソッド yyyy を別のクラスに移動して、そのクラスを DI して呼び出すようにしてください
理由
- Spring Framework による宣言的トランザクション管理は Spring AOP で実現している
  - Proxy から呼び出す Transaction Advisor がトランザクションを開始、コミット、ロールバックしている
- Spring Bean のメソッドから自分のメソッドを呼び出すときは通常の Java オブジェクトの仕組みに従う
  - 明示的な記述はありません。動作確認をした結果からの推測です
- したがって Spring AOP で実現している機能が利用できない状態になっている
参考

@Component
public class XXXService {

    private final YYYService yyySrevice;

    @Authwired
    public XXXService(YYYService yyySrevice) {
        this.yyySrevice = yyySrevice;
    }

    public void xxx() {
        // something
        yyyService.yyyy() // OK
    }
}

@Component
public class YYYService {
    @Transactional
    public void yyyy() {
        // some database processing
    }
}

RequestMapping を指定したメソッドに対する引数の割り当てと利用

相談されたコードはこちら。

RedirectAttributes を引数に持つメソッド post から Model を引数に持つメソッド internal を呼ぶと、ビューの描画が壊れてしまう
Model を引数に持つメソッド get から Model を引数に持つメソッド internal を呼ぶと、ビューの描画は正常

@Controller
@RequestMapping("/item")
public class ItemController {

    @GetMapping
    String get(
            @RequestParam(required = false) String message,
            Model model
    ) {

        model.addAttribute("message", message);
        return internal(model);
    }

    String internal(Model model) {

        if (!model.containsAttribute("message")) {
            model.addAttribute("message", "internal");
        }

        model.addAttribute("someValue", "someValue");

        return "index";
    }

    @PostMapping(
            consumes = {MediaType.APPLICATION_JSON_VALUE}
    )
    String post(
            @RequestBody Item item,
            RedirectAttributes redirectAttributes
    ) {

        if (item.getName() == null) {
            redirectAttributes.addAttribute("message", "no item");

            return internal(redirectAttributes);
        }

        redirectAttributes.addAttribute("message", "success");

        return "redirect:/item";
    }

原因
- ビューを描画するときに使用する Model にアプリケーションが設定した値が反映されていないため
理由
- Requestmapping を指定したメソッドに引数を束縛するとき、Model と RedirectAttributes は ModelAndViewContainer の別のフィールドに格納するようになっている
- ビューを描画するときは Model を使用する
- つまり、RedirectAttributes を引数にするならリダイレクト以外のビュー解決ができなくなる
所見
- コントローラーのメソッドだけをユニットテストをしても気付けないと思う
- MockMVC でテストしてれば気付けると思う

説明変数が役に立ちそうな場面

提案されたコードはこちら。

UI 要素の表示・非表示を DTO の boolean 型のフィールドで制御してる
ビジネスロジックの返り値 (boolean) を反転した値をフィールドに設定してる

ViewDto dto = new ViewDto();

if (params.getValue().equals(GlobalContext.getValue())) {
    // 条件が真の場合は非表示
    dto.setDisplay(false);
} else {
    dto.setDisplay(true);
}

修正の提案はこちら。

リテラルの true/false はなんかいや
かといって setter に指定する値を否定演算子で逆転するのもいや
日本語でコメント書くくらいなら説明変数を追加したほうがいい

ViewDto dto = new ViewDto();

boolean disableWhenOk = params.getValue().equals(GlobalContext.getValue());
dto.setDisplay(disableWhenOk);

そもそも表示・非表示のような制御変数を持ちまわすこと自体がいまいち

DTO だからメソッドを持ってはいけないと考えるのではなく、そういう役割のモデルの存在を明確にしたほうがいい

コードレビューの雑なまとめ : テストコード(主に Java)

Java の製品コードを Groovy でテストする

提案されたコードはこちら。

Spring Framework の製品コードに対して Groovy + Spock で記述したテストコード
SomeLogic#createSomeInformation が SomeRecord から SomeInformation を生成するみたい
四捨五入の結果が同値になるかどうかを確かめてるみたい(推測)

  def 'xxxとyyyには同じ編集ロジックが適用される'() {
    given:
    SomeLogic sut = new SomeLogic()
    SomeRecord record = new SomeRecord()
    record.xxx = "12.51"
    record.yyy = "13.49"

    when:
    SomeInformation actual = ReflectionTestUtils.invokeMethod(sut, 'createSomeInformation', record)

    then:
    actual.xxx == actual.yyy
  }

修正の提案はこちら。

テスト対象クラスのオブジェクト(System Under Test) の準備はテストメソッドから分離したほうがいい
- 見通しがいいかどうかの問題
- 特に必要なリソースもないのでフィールド初期化で済ませてる
小数点以下の切り上げ・切り捨てにフォーカスしたデータテストへ変形
- メソッド名に変数名を埋め込むとテスト結果の確認が楽になる
with ブロックでオブジェクトのフィールドを初期化する
- 代入や setter メソッドの呼び出しがあるなら同じ場所に囲みたい
テストメソッド内では def で変数を宣言する
- 基本的に多くの仕事をしないのでクラス名を記述するのは冗長になりやすいから
Groovy はプライベートメソッド・フィールドにアクセスできる
- 好ましい作法じゃないけどそういう言語仕様になっている
- この程度でリフレクション API を使う必要があるならテスト対象クラスの設計を見直したほうがいい

  SomeLogic sut = new SomeLogic()

  @Unroll
  def 'SomeLogic は xxx と yyy を整数部だけにする: ケース:[#context] 入力:[#in] 出力:[#out]'() {
    given:
    def record = new SomeRecord().with {
      it.xxx = in
      it.yyy = in
      return it
    }

    when:
    def information = sut.createSomeInformation(record)

    then:
    information.xxx == out

    and:
    information.yyy == out

    and:
    information.xxx == information.yyy

    where:
    context                             | in       | out
    '0.5未満は切り捨て'                 | '11.0'   | '11'
    '整数部が奇数なら0.5以上は切り上げ' | '11.50'   | '12'
    '整数部が偶数なら0.5以下は切り捨て' | '10.50'   | '10'
  }

この文章の目的

コードレビューの雑なまとめ : シェルスクリプト(主に bash)

テキストファイルを一行ずつ処理するようなやつ

コードレビューの雑なまとめ : 製品コード(主に Java)

キャストは必ずチェックする

配列オブジェクトの拡張

org.apache.http.HttpStatus より org.springframework.http.HttpStatus を使う

有る無しを boolean 変数に反映するだけなら条件分岐する必要はない

コレクションクラスのコンストラクタを知ろう

Apache HttpComponents Client の発生する例外を捕まえたい

Spring Framework の仕組みを学ぶ

RequestMapping を指定したメソッドに対する引数の割り当てと利用

説明変数が役に立ちそうな場面

コードレビューの雑なまとめ : テストコード(主に Java)

Java の製品コードを Groovy でテストする

`org.apache.http.HttpStatus` より `org.springframework.http.HttpStatus` を使う

有る無しを `boolean` 変数に反映するだけなら条件分岐する必要はない