Zenn
😧

コードの理解度を他者が推し量ることの難しさ

2022/05/05に公開
C++
初心者
ポエム
プログラミング
idea

半年ほど前のこと。
若手プログラマーが相談に来てくれました。
そのときのことを思い出したので書いてみます。
タイトルは多少盛っています。 (^^;

相談

既存のコード

既存のコードに次のような関数があります。
読んでもらえれば何となくわかると思うので内容の説明は省略します。

Result MessageReceiver::OnReceived(Message message) {
  switch (message.Type) {
  case TypeA:
    // タイプが A であるメッセージを受信したときの処理
    ...
    break;
  case TypeB:
    // タイプが B であるメッセージを受信したときの処理
    ...
    break;
  ...
  }
}

仕様変更

既存のコードでは TypeA, TypeB, ... は定数 (残念ながらマクロでした) でした。
しかし、仕様変更によって TypeA の値が実行時にならないと確定しなくなるとのことです。
実行時に他のライブラリの関数を呼び、その結果を使って値を確定させます。
この仕様変更にあたって、既存のコードをどのように改造すればよいかという相談です。

条件

提示された条件は主に次の 2 点です:

  • タイプ A の値を確定させるための他のライブラリの関数呼び出しは最初の 1 回だけにしたい。
  • 変更範囲は可能な限り狭くしたい。

回答

この相談に対して、私はいくつかの方法を提案しました:

  1. TypeA だけでなく TypeB も含めて値を確定させて提供する役割を MessageReceiver クラスから分離する。
  2. TypeA を MessageReceiver クラスの定数データメンバとし、同クラスのコンストラクタで値を確定させる。
  3. TypeA を MessageReceiver::OnReceived メンバ関数のローカル静的定数とし、その初期化中にライブラリの関数を呼び出す。

前述の条件にあるように必要以上に広い範囲のコードに手を入れるのは避けるべきという考え[1]から、可能な限り局所的な改造で仕様変更を実現する案 3 が選ばれました。
実装イメージはこんな感じです:

Result MessageReceiver::OnReceived(Message message) {
  static const int typeA = []() {
    // 外部のライブラリから得た情報を元に、タイプ A の値を確定するコード
    ...
    return xxx;
  }();

  switch (message.Type) {
  case typeA:
    // タイプが A であるメッセージを受信したときの処理
    ...
    break;
  ...
  }
}

その後

相談からしばらく経ち、忘れた頃になってテストチームからバグ指摘が来ました。
指摘の内容は「ログが出すぎている」というものでした。
リポジトリをフェッチして見てみると、リリースに取り込まれたコードはこうなっていました:

Result MessageReceiver::OnReceived(Message message) {
  static const int typeA = []() {
    // 外部のライブラリから得た情報を元に、タイプ A の値を確定するコード
    ...
    return xxx;
  }();
  LOG("TypeA: %d", typeA);

  switch (message.Type) {
  case typeA:
    // タイプが A であるメッセージを受信したときの処理
    ...
    break;
  ...
  }
}

メッセージを受信するたびに、定数 typeA の値をログに出力するようなコードになっていました。
なんで...

お気づきのとおり、ログを出力するのであれば、ローカル静的定数 typeA を初期化しているラムダ式の中に書くべきです。

  static const int typeA = []() {
    // 外部のライブラリから得た情報を元に、タイプ A の値を確定するコード
    ...
    LOG("TypeA: %d", xxx);
    return xxx;
  }();

振り返り

相談に応じる形で関与したので、なぜこうなったのかその後ひとりで考えました。
もちろん、テストが不十分だったということもありますが、次のような可能性を考えてしまいました:

  • このプログラマーは、「最初の 1 回だけ」の条件がどのような原理で実現されているのか理解できていなかったのではないか。
  • レビューした開発者も同様に理解できていなかったか、あるいはきちんと見ていなかったのではないか。

なお、このプロジェクトでは、最低 2 名の開発者が LGTM しないとリリースには出て行かないルール・運用です。

プログラマー本人にも質問してみたのですが、バグを出してしまったことの責任を必要以上に感じているようですっかり「すみません」モードになってしまっていました。

彼は実際には委託先 (つまり他社) の社員で、顔を合わせたことは数回しかなく、年齢もおそらく 10 歳以上離れています。
そんなわけで、私は彼のプログラマーとしてのスキルレベルを正確に把握できていなかったし、彼もわからないことがあっても私に質問しにくい状況があったかもしれません。
(それでも相談に来てくれたのですが)

もちろん、きちんと理解していたけれどもうっかりしていたという可能性もあるでしょう。
ただ、同じコードを前にしたとき、他者がそれをどこまで理解しているのかを推し量るのは難しいことだなと考えるきっかけになりました。
特に、 C++ は人による理解の差が生まれやすい言語ではないかと思います。

普段からこういう話をするとか、理解を近づけるようなことが必要なのだろうかと考えました。
プライドやモチベーションといったものに気をつけながら。

PS

「こういうとき普通はどうするのかな」と思い、先ほど検索してみたところ、次の記事が見つかりました:

https://qiita.com/Y0KUDA/items/d77275435fdf20b9a6ac

今回採用された案も紹介されていました。
記事の著者のご見解:

マニアック過ぎ。
でも、一番スマート（だと思う）。

ローカル静的変数が最初に 1 回だけ初期化されるという仕様は、個人的には「マニアック」という感覚はなかったのですが...

たとえばシングルトンパターンを実装するときによく使われる手法だと思います。
C++11 からはスレッドセーフであることが規定され、マルチスレッド環境でも安心して使えるようになりましたし。

class Singleton {
 public:
  Singleton(const Singleton &) = delete;

  static Singleton& instance() {
    static Singleton uniqueInstance;
    return uniqueInstance;
  }

 private:
  Singleton() = default;
};

何となく書いてみたシングルトンパターンのシンプルな実装例ですが、たとえば次のようなことを見落とさずに理解してもらえるかと気になってしまいます:

  • デフォルトコンストラクタを private にしている理由
  • コピーコンストラクタを delete 定義している理由
脚注

  1. 私がこれまで関わってきたプロジェクトではそのような考えが支配的でした。 その是非は本記事の趣旨から外れるので触れません。 ↩︎

Discussion