読みやすいコードを書くためのヒント (Code Climate Qualityの指摘対策)

はじめに

ソースコードの読みやすさは読む人が判断するものですが、読みやすさをロジカルに算出する方法の一つに Code Climate のQualityがあります

それを導入すれば、ある指標の中ではあるものの一定の品質が保たれるということで、最近、会社のリポジトリにも入るようになりました。最初から良い評価は期待してないものの、200件以上指摘されてしまう状況で、実際にどのように修正したら良いか戸惑ってしまいました。コンパイラの警告やlintは主に文法のチェックなのである程度予想出来るものだったのですが、Code Climateは全く違う観点で修正のヒントがあればと思ってメモを残します

Code Climateの登録や使い方などについては、ここでは触れてませんので docs などをご参照ください

コードを見やすくするために

Code Climateは簡潔に書けば見やすくなるというコンセプトでコードを見ています。大まかには以下のポイントになってます

• 1関数は25行までに抑える ^*
• 1ファイルは250行までに抑える ^*
• 引数は4つまで
• ネストは2階層まで
• else if() の分岐は4つまで
• 同じ処理、似た処理は極力関数化してまとめる

具体的には以下の指標が出てきます

• lines of code ^*
• Cognitive Complexity
• duplication
• xxx complex (自分のコードでは今まで指摘されたことはありませんが、複雑な遷移でフローチャートから破綻しているパターンのようです)

^* コードの行数について、物理的な行数ではなく空行やカッコだけの行数は除いて算出されます。 詳しい算出方法は調べてないですが、データの定義で見やすくするために改行したものでもカウントアップされるのが少々辛いところです

Cognitive Complexity の算出方法は docs の中で紹介されているドキュメントにありました
https://www.sonarsource.com/docs/CognitiveComplexity.pdf

duplication はそっくりな処理がある場所を指摘します。同じようなコードがまとめられないかのヒントになります

指摘に対応するためのヒント

指摘に対応するために変更していったことを紹介します。コードはJavaScriptです。

三項演算子を使用して、行数を減らす

以下のように、if()文で書いてしまうと必然的に行数を使ってしまいます。

変更前

    if (range.start > 30) {
        range.start -= 30;
    } else {
        range.start = 0;
    }

この場合、三項演算子を使って1行にまとめることができます

変更後

    range.start = range.start > 30 ? range.start - 30 : 0;

見た目がスッキリしました。ただし、三項演算子を使いすぎると逆に分かりにくくなると思う人も少なくありません。個人的にも、上で書いた方が初心者には分かりやすいし丁寧な書き方だと思ってます

もう一つの例

変更前

mapRangeSlider.prototype.setPlaybackPositionVisible = function(sw) {
    if (sw === true) {
        this.playback_pos.setAttribute('display', 'inline');
    } else {
        this.playback_pos.setAttribute('display', 'none');
    }
}

どうやって減らそうか悩んだ結果が↓です

変更後

mapRangeSlider.prototype.setPlaybackPositionVisible = function(sw) {
    const disp = sw === true ? 'inline' : 'none';

    this.playback_pos.setAttribute('display', disp);
}

setAttribute()の引数が異なるだけという所に着目して、引数を変数に変えました。最初は変えるメリットは分からなかったのですが、要素が増えた時に半分の行数になるので、削減効果は大きかったです。同じ記述を減らすことで変更する時の手間が省けるような気がしました

処理の前提条件を絞る

インスタンスが生成されてなければ new で作り、既に生成されていたら表示されている内容を close してインスタンスを再利用するケースがありました

変更前

    if (polyline_clicked_info === null) {
        polyline_clicked_info = new google.maps.InfoWindow();
    } else {
        polyline_clicked_info.close();
    }
    polyline_clicked_info.setPosition(e.latLng);
    polyline_clicked_info.setContent(info_content);
    polyline_clicked_info.open(this.ins_map);

処理の前に close してインスタンスを必ず無い状態にしておけば分岐する必要が無くなります

変更後

    this.closePolyLineInfo();  // ここで this.polyline_clicked_info が nullになる
    this.polyline_clicked_info = new google.maps.InfoWindow();
    this.polyline_clicked_info.setPosition(e.latLng);
    this.polyline_clicked_info.setContent(info_content);
    this.polyline_clicked_info.open(this.ins_map);

   :

map.prototype.closePolyLineInfo = function() {  // closePolyLineInfo の中身
    if (this.polyline_clicked_info !== null) {
        this.polyline_clicked_info.close();
        this.polyline_clicked_info = null;
    }
};

条件判断を別関数にする

|| や && で複数の条件を重ねると Cognitive Complexity のポイントが加算されてしまいます。判定用の関数を作って対処します

変更前

function func() {
        :
    if (!str && str.length > 10) {
        :
}

変更後

function isValidStr(str) {
    return (!str && str.length > 10);
}

function func() {
        :
    if (isValidStr(str)) {
        :
}

深いネスト構造を浅くする

配列内のデータを取り出し、有効な値の時に処理をする場合を考えてみます

変更前

  for (var i=start_range; i<end_range; i+=skip_idx) {
    const latlng = get_latlng(tracks[i].coordinate);
    if (latlng !== null) {
        :
    }
  }

処理全体が if (latlng != null) 配下に入ってしまっています

  for (var i=start_range; i<end_range; i+=skip_idx) {
    const latlng = get_latlng(tracks[i].coordinate);
    if (latlng === null) {
        continue;
    }

      :
  }

条件を反対にして、直ちに continue するようにすれば、階層が深くならずに済みます。Cognitive Complexity は深い階層の処理が多くなるほど数値が高くなりますので、階層には気を配るようにします

引数をオブジェクトにしてまとめる

引数は5以上になると警告が出てしまうので、オブジェクトを使うようにします。引数の数が増えてしまうと順序が何だっけ？ってにもなるので、オブジェクトにすれば順序も気にする必要が無くなります

変更前

function func(latitude, longitude, altitude) {
    :

変更後

function func(coordinate) {
    const latitude  = track.latitude;
    const longitude = track.longitude;
    const altitude  = track.altitude;

    :

その他気付き

最初は指摘箇所を改善しようと必死になってましたが、指摘されている所にはそれなりの改善のヒントがあるような気がしてきました

自分のコードでは

    if (start_route !== null && end_route !== null) {
        plotMapPolyLine(route, line_color);
    }
 
    if (route_length === 0) {
       :
        // 有効な位置情報がない時のエラー処理

データリストに有効な位置情報が見つかった場合 plotMapPolyLine を実行するようにしていましたが、改めて見直すとすぐ下の route_length 判定でも同じことをしてるんじゃないかと気が付きました

このようにシンプルになっていれば分かりやすいのですが、コードサイズが大きくなってくると他の記述に埋もれてしまって気が付きにくくなると思います

終わりに

前職で「1日に3000行コーディングした」と豪語する人がいました。その人は凄く頭がキレるのか思考が早すぎて話にも付いていけなかったのを覚えています。エンジニアの生産能力やプロジェクト規模をコードの行数で算出している場面もあると思いますが、必ずしも長ければ良いって訳ではないので、別な指標でコードを評価する仕組みが出来ることは理に適っていると思います

しかしながら、静的解析の算出はあくまで、一つの指標にすぎません。指摘にこだわり過ぎて開発が進まなくなってしまうのは何のためなのか分からなくなってしまいます

どこまで対応したら良いか、ちょうどいい塩梅は正直分かりません。私は大量の指摘に心折れて ignore list に入れたらエライ人に「さすがにそれはマズイ」って言われて、今度は真摯に向き合い過ぎて進捗が進まずに他のメンバーに愛想付かされているような状態です。ほとんどのコードが A判定 貰えるようになってきてはいるものの、まだまだ修行が必要な感じです