X(Twitter) Zenn GitHub RSS 共有

こんなソースコードは嫌だ

作成日時：2023-01-17
更新日時：2023-01-17

いま私が出向しているプロジェクトのソース(運用含め)が余りにも酷いので反面教師として内容を記載する。
と思っていたが書いていくうち、ほぼ愚痴みたいになってしまった。
基本Javaベース。Webアプリ。

1.クソコード集

1クラス10,000行over

ふざけとるんか。
何かしらの理由があるならギリギリ許せるがこのプロジェクトにそんなものはない。

どうすべきか：

ネスト、ネスト、ネスト

if (...) {
    if (...) {
        if (...) {
            if (...) {
                if (...) {
                    // まだまだネストするよ！
                }
            }
        }
    }
}

これに加えてインデントが揃っていないという地獄。
訳アリでフォーマッタも使えない状況。

どうすべきか：

マジックナンバー

if (kbn == 1) {}

区分「1」って何ですか。区分「1」って何の区分なんですか。ねえ。
このプロジェクトは設計書が無いんすよ。

どうすべきか：

メソッド定義

public void generatePin(){}
public void generatePin2(){}

generatePin2とは？2とは何ぞや。思いつかないからって適当に数字つけないで。
無印との違いはなんなのさ？
いちいち定義を見に行かねばならん。

どうすべきか：

命名規則の記法

int a_num = 1;
int bNum = 2;
method_snake();
methodCamel();

記法統一して。camelなのかsnakeなのか。

色々なところから呼ばれる内部クラス

class OuterClass {
    public static class InnerClass {
        public static void method(){};
    }
}
// call
OuterClass.InnerClass.method();

InnerClassがprivateでOuterClassからのみ使われるなら分かる。
でも外部から直で呼び出されている。
何のために内部へ定義した？外に出して。
OuterClass.InnerClassといちいち書くのが面倒。(書かずに済む方法があるが、だからと言ってそれを使うな。)

どうすべきか：

操作可能にならないプルダウン

画面が表示されてからAjaxでプルダウンの中身を取得しているが、それに2分くらいかかる。
そのため2分間、プルダウンは操作不可能(何も出てこない)

原因：
データ取得のSQLが遅すぎる。

Q. 何故遅いのですか？
A. 全テーブルにインデックスが存在しない。フルスキャン+結合処理で尋常じゃないコストが発生。

どうすべきか：

インデックスを貼らない理由があるならば貼らなくても良い。

N + 1問題

var list = 対象のデータ一覧をDBから取得する処理();
for (listの件数文ループ) {
    listのデータの詳細データをDBから取得する処理();
}

何回もDBへアクセスしに行くな。

どうすべきか：

さっき取得したでしょそれ

var idList = [1, 5, 3, 1];
for (var id of idList) {
    DBからidに紐づくデータを取得(id);
}

ID:1のデータ取得処理2回呼んでる。

どうすべきか：

タイムアウト

DB.connect();
for(連携されたファイル数分ループ){
    時間かかるファイル処理();  
}
DB.close();

このコード書いた人、ファイル1件とかでしかテストしなかったんだろうか。
STG環境で複数ファイル流したらタイムアウトが発生してDBコネクションがクローズされない→プール枯渇→死で顧客から連絡来たよ。

どうすべきか：

変数の命名とスコープ

var a = xxx; // 使用する直前に初期化しても問題ないのに。
.
.
数百行のロジック
.
.
a.method(); // 初めてaを参照。

勘弁してください。

どうすべきか：

共通化してくれ

app1
└─Util.java
app2
└─Util.java
app3
└─Util.java
app4
└─Util.java

上記の4アプリ(パッケージ)は大体似たような内容で、その中のUtil.javaも同じ内容だが、ライブラリとして抽出していない。
なのでどれか1つを直したら別の3つにも適用しなければならない。
(もう最近はそれすら放棄していて名称やパッと見の振る舞いは同じなのにappごとに方言のような若干の差異がある。地獄。)

どうすべきか：

他にも問題点が大量にあるけど、もはや記事にするレベルじゃない内容ばかりなので割愛。
(XSS実行可能とか、スレッドセーフじゃない処理とか、参照型の比較に等価演算子使ってるとか、フレームワークも設計書もコードレビューも存在しないとか。)

2.こうならないためにはどうすべきなのか

正直、ここまでの酷さは普通にやってればならないと思う。

とにかく個人/会社ともにコードの品質を高めるようにしていくべき。

個人：リーダブルコード読むなりしてコードの品質を高める。
会社：コードの品質を高める仕組み/枠組みを作る。

自分がコーディングする際に気にすること

基本的に下記を守るようにコーディングしている。出来ているかはともかく。
(出向先にちゃんとコーディング規約があるならそっちを遵守)
(例外もある/やりすぎもNGなのでケースバイケースで)

DRY原則

KISS原則

YAGNI原則

SOLID原則

コメントやJavaDocをちゃんと書く

命名規則をしっかりと

デザインパターン

私は特に1256を厳守。

大雑把なまとめ：
「同じ処理書かずにシンプルで余計な機能はつけず、コメントと命名規則ちゃんとやって何かいい感じにする。」

リーダブルコード

読め。

コードの品質を高める仕組み

私がSESで出向した会社の内、品質が高かった会社はコードの品質を高める仕組み/枠組みがきちんと確立していた。

下記がちゃんとある。

逆にこれらが全て存在しない今の私の出向先の品質は最悪なものになっている。

良かった出向先のフロー例

  1. コーディング規約に則り、コーディングする。
  2. コードレビュー前にチェックリストを確認する。
    アンチパターンな記述があれば修正する。
  3. フォーマッタをかける。
  4. コードレビューを受ける。
  5. 指摘があり、かつそれが頻出する指摘事項であればチェックリストに追加(ナレッジ蓄積)

1-5を繰り返す。
チェックリストは他のプロジェクトでも使いまわす。どんどん熟成していく。

これらをやっていた会社で発生した不具合は、仕様認識相違が起因のものぐらいでコーディングミス等の不具合はほぼ無かった。

コーディング規約/フォーマッタ

チェックリスト

テスト自動化(xUnitとか)

テスト自動化の損益分岐点