わたしとあなたと今日のレビュー (1) #C#

ここ数年お仕事ではエンジニアチームのリーダーとして、設計の方針を決めたり後輩エンジニアの教育やコードレビューにウェイトを置いて仕事をしています。またプロジェクト掛け持ちもあって、最近はレビューの頻度が爆上がっています。

そんな中、指摘の正しさはもとより自分の言葉が一貫しているかどうかは、以前と比べてとても気にするようになりました。前に指摘したことと今日指摘したことが違っていたら開発チームが混乱してしまいます。

そのため指摘や相談を求められたときはできるだけ自分の腹落ちしているコンテンツから引用してきたり、そうなるまで新しく勉強してから提案するなど、「なぜ？」と共にちゃんと後輩に説明できる状態に持って行くよう、気を付けています。

しかしプロジェクトの締め切りが近かったり、時間が取れないとその対応も十分できないことがあります。

「ハンマーしか持っていなければすべてが釘のように見える」という言葉を最近知りました。継続して勉強する理由をよく言い表していると思います。

この業界は特に、数年前正しいとされていたことが今日では推奨されないということも珍しくありません。それに気づいて間違いを間違いとして気づければよいのですが、そうでなければ知らず知らずのうちに足を引っ張ることになるかもしれません。特に、リーダーとして後輩を引っ張る立場となっている今、自分の言葉の影響力、それとある意味での恐怖と共にひしひしと感じています。

というところで、過去５年分くらいの記録が残っていた私のレビュー指摘を垂れ流しつつ、「なぜ？」そうしたのかやイマドキの時流にフォーカスしながら考察してみようと思いました。

・・・

さて以上が建前となります。

以下に、仕事後の主任がアルコール入れながら過去を思い返し郷愁を感じる様を示します。

今回の対象プロジェクト

C#, C++ の混在プロジェクト。
組み込み機器を制御する GUI アプリで、 GUI を WPF 、ファームウェアを C++ で開発。
レビュイーは業務経験1～3年目くらいの子たちです。

[プログラミング全般] どんな時にどんなログを出すべき？

ログはコメント同様、慣れないうちはどんなことを書けばよいのか悩むことが多いです。
何を出せばよいのかわからず時間が過ぎ、結果として何も出さなくなり、ユーザーサポートで苦労することが多いように見えます。

ググって出てくる情報はWeb系のが多いのも、二の足を踏む原因かな？

基本方針

まずは次の記事をざっと見ておくとよさそうです。

よくあるログの出し方は次のようになるかと思います。

bool SaveUserSetting(string filePath, StringBuilder diag)
{
    Logger.Trace();
    if (string.IsNullOrEmpty(filePath)) throw new ArgumentException();
    
    if (string.IsNullOrEmpty(Data.UserName)) {
        Logger.Warning("UserName is Empty.");
    }
    
    try {
        string jsonString = JsonSerializer.Serialize(Data);
        File.WriteAllText("data.json", jsonString);
    }
    catch (Exception e) {
        Logger.Error(e.ToString());
        diag.Append(e.ToString());
        return false;
    }
    
    return true;
}

bool Save(StringBuilder diag)
{
    Logger.Trace();
    if (!SaveUserSetting("user.json", diag)) {
        return false;
    }
    if (!SaveEnviromentSetting("env.json", diag)) {
        return false;
    }
    return true;
}

void HandleSaveButtonClicked()
{
    Logger.Trace();
    var diag = new StringBuilder();
    if (!Save(diag)) {
        MessageBox.Show("次のエラーが発生しました: " + diag.ToString());
    }
}

レベル	こんなときに	ログの対象読者
ERROR	一連の処理を中断するような問題を検出した時。	ユーザー・サポートチーム・開発者
WARNING	変な入力値を修正して処理を続行したり、リスクのある状況を検出した時。 ERROR との違いは、処理自体は中断せず続行していること。	ユーザー・サポートチーム・開発者
INFO	一連の処理の開始や終了、イベントログ。	ユーザー・サポートチーム・開発者
DEBUG	システム情報等。	開発者
TRACE	関数や分岐の入口・出口。	開発者

このルールを常に守るかは、それぞれのプロジェクトに依ります。
例えば私が参加しているプロジェクトでは大量のログをさばくことが多いため、実行速度に重点を置いています。なので TRACE は端折ることもあります。

以下、ちょいコメント。

自分で例外を throw するところではログは出しません。
- 私は例外を自分で throw することは避けるように指導しています。使って良いのは、いわゆるロジックエラー（プログラムの書き方・APIの使い方が間違っている＝ユーザーではなく開発者のミス）を検出した時で、 rust なら panic で停止させるような重大な問題の時です。もちろん、未処理例外の対策（後述）が入っていることとセットです。
  - C# は標準ライブラリが例外を使いまくってるのでそのノリで自分のプログラムでも使ってしまうことがありますが、節操無しに使ってしまうと簡単にスパゲティになってしまいます。昨今は例外に消極的な新しいプログラミング言語の登場から、C++ や C# でも例外に頼りすぎないような文化が出てきているようにも感じます。
上記 Save のように、呼び出し先がエラーしたとき、ただ呼び出し元に return するだけならログは出しません。
- ログは問題を検出した真因の箇所では、必ず出力します。このようにただバトンパスするようなところで出しても、ノイズになることの方が多いです。

[プログラミング全般] 早期 return しよう

↓こういうコードは、

bool Connect(DeviceState state)
{
    if (state != null) {
        if (!state.IsConnected) {
            if (state.HasAddress) {
                var settings = ... いろいろ;
                DeviceConnection.Connect(state);
                return true;
            }
            else {
                return false;
            }
        }
        else {
            return false;
        }
    }
    else {
        return false;
    }
}

↓こうします。

bool Connect(DeviceState state)
{
    if (state == null) return false;
    if (state.IsConnected) return false;
    if (!state.HasAddress) return false;
    var settings = ... いろいろ;
    DeviceConnection.Connect(state);
    return true;
}

エラー処理のために処理を深く・関数を長くしなくて済むことが多い。
処理を実行するための前提条件がおなじリズムで関数上部に集まるため、関数仕様のようなドキュメントの意味も持ちやすい。

ので良いです。

設計的にあり得ない（そのエラーに当てはまったらバグの可能性が非常に高い）場合は例外にしたり、ログに出すとなお良いです。

bool Connect(DeviceState state)
{
    if (state == null) throw new ArgumentException("state == null");
    if (state.IsConnected) return false;
    if (!state.HasAddress) return false;
    var settings = ... いろいろ;
    DeviceConnection.Connect(state);
    return true;
}

早期 return で検索してみよう！

vs MISRA-C

早期 return の文脈ではよく登場するテーマかと思います。
MISRA-C をちゃんと運用しているプロジェクトに携わったことはありませんが、
MISRA-Cにおける「関数の末尾以外の return 禁止」の真意は一考の余地在り処と思います。

[プログラミング全般] NOTE, TODO, FIXME コメントを入れよう

アノテーションコメントと呼ばれているものです。
コメントにタグのような情報を付与し、意味を強調することができます。

// TODO: 検索の処理負荷を測ったほうがいいかも
var item = FindItem(userName);

このようなコメントはテキストエディタが強調してくれたり、コメントの個所をリストアップしてくれたりします。
VisualStudio であれば、メニューの [表示] > [タスク一覧] から確認できます。

プロジェクトによっては次のように TODO に続いてユーザー名やメールアドレスを記入することで、責任者を明確にすることもあります。

// TODO(lriki@example.com): 検索の処理負荷を測ったほうがいいかも

[C#] 未処理例外ハンドラを書こう

C# や JavaScript など例外が使える言語では、 catch を忘れはよくある問題かと思いますし、それを危惧して try-catch をたくさん書くと可読性や例外握り潰しの温床になったりします。
何を例外にしてどんな時に try-catch するべきかはテーマが大きいのでまた今度書こうと思ますが、ひとまず今回は、そんな取りこぼした例外への対処法があるんだよ、っていうことを知っておいてほしいと思います。

C# 未処理例外 C# unhandled exception でググると大体情報が出てきます。

例えば WPF(C#) アプリケーションであれば、自動生成されている App クラスに次のように書いておきます。

public partial class App : Application
{
    public App()
    {
        DispatcherUnhandledException += App_DispatcherUnhandledException;
    }
    
    private void App_DispatcherUnhandledException(object sender, DispatcherUnhandledExceptionEventArgs e)
    {
        MessageBox.Show("エラーが発生しました。次の情報をベンダーまでご報告ください。: " + e.Exception.ToString());
        Environment.Exit(1);
    }
}

これは最小限の例ですが、実際にはサポートサイトや報告フォームを開く機能を追加するなど、拡張は必要かもしれません。
まぁ普通は発生させちゃダメなものなのですが、小指をぶつけて機器が接触不良したとか太陽フレアで通信ラインがやられたとか、（冗談ではなく）例外は起こるものとして、どの程度拡張するかはステークホルダと相談しましょう。

なお WPF アプリケーションでは次のハンドラを登録しておくとよいでしょう。

Application.DispatcherUnhandledException (UIスレッドで発生した例外)
TaskScheduler.UnobservedTaskException (Task 内で発生した例外)
AppDomain.CurrentDomain.UnhandledException (それ以外 (Thread等) で発生した例外)

[!NOTE]
.NET Framework の WindowsForms アプリでは、フレームワークが自動的にエラーウィンドウを表示してくれました。

しかし WPF では「動作を停止しました」ウィンドウしか出なかったり、 .NET (.NET Framework ではなく) では何も表示されずｽﾝｯ… とプロセスが消えることもあります。
なので新しいアプリを作るときは最初に未処理例外ハンドラを登録しておくのが無難でしょう。

おわりに

体系的に整理するにはあまりにもたくさんのレビュー記録があるので、こんな感じで見つけた順に流していこうかと思います。

もし自分の知らないキーワードを見つけたら、ぜひ調べるきっかけにしていただけると嬉しいです。

変なこと書いてたら教えてください🙇