コードレビューとアルゴリズムレビュー

先日、社内開発にて以下のようなやり取りが行われました。

とある処理の書き方としてどちらで書いた方がいいか？

A案

string Func()
{
    if (argA == 1)
    {
        return "one";
    }
    else if (argA == 2)
    {
        return "two";
    }
    else if (argB == 4)
    {
        return "four";
    }
    else if (argB == 5)
    {
        return "five";
    }
}

B案

string Func()
{
    switch (argA)
    {
    case 1:
        return "one";
    case 2:
        return "two";
    }

    switch (argB)
    {
    case 4:
        return "four";
    case 5:
        return "five";
    }
}

簡略化のためマジックナンバーを使っていますが、今回のテーマは『if文で書くか、switch文で書くか』です。

3人の内、2人がB案を推しましたが、私は今回はA案を推しました。
コードの見やすさ、実行時のコストを考えるとB案を推すのは分かります。
私もこのコードだけを見て判断するなら、B案を推したでしょう。

しかし今回は、とある事情がありました。

まずこの関数ですが、アルゴリズムとしてそもそもイケてないという問題がありました。
すなわち、argAとargBで判断を行う方法自体があまり良くなく、もっとスマートなやり方が無くはないといった事情がありました。
しかしアルゴリズムは既に運用が行われており、アルゴリズムそのものを変えてしまうと、影響範囲が大きくなりテストが大変になるという問題を抱えていました。

このため、まずこの時点でコード自体の美しさについては、判断材料としてのプライオリティを下げる判断をしました。
つまりA案だろうがB案だろうが、根本がイケてなく、目クソ鼻クソであるという評価です。

次にこの関数に今後起きる変更や改修についての予測です。
この関数では、今後argA・argBでの種類追加が起きる可能性が高く、その際にこの関数に影響があるか確認をする必要があることがある程度予測されました。
ただの種類追加なら良いのですが、判断順の優先の変更や、argCといった新たな変数の登場なども可能性としてはあるといった状況で、どちらの書き方の方がより修正範囲を狭めて対応できるか、その柔軟性が優先として高くなりました。

例えば、今はargAを先にチェックしてargBをチェックしますが、もしargAが1かどうか判断して、falseなら次にargBが4かどうかチェックして、falseなら次にargAが2かどうかチェックする。
といった処理にしなければいけなくなった時に、B案のswitchでは対応できなくなります。
A案であれば2項目を入れ替えるだけで対応できます。

string Func()
{
    if (argA == 1)
    {
        return "one";
    }
    else if (argB == 4)    // ←ここと
    {
        return "four";
    }                      //
    else if (argA == 2)    // ←ここを入れ替えるだけ
    {
        return "two";
    }                      //
    else if (argB == 5)
    {
        return "five";
    }
}

argAのチェックの後にargCが0かどうかをチェックする必要が発生した場合、B案では2つのswitch文の間に、if文か項目が1つだけのswitch文を入れることになります。
この時点で上っ面のコード上の見た目とか、もはやどうでも良くないですか？
内面ブスなのに。

こういった変更が今後あるか？と言われれば、「ない！　とは言えない、かも…」という、ちょっとめんどくさい場所だったわけです。

新規開発案件の場合、普通はあんまり無いですが、運用案件の場合、エンバグやテスト工数が膨らむことを嫌い、破壊的変更を行わず改修を行うことは良くあります。

という事情を踏まえ、最初のA案かB案かに戻ると、今回は「多少の不格好や処理コストに目をつむっても、A案にしておくのがが良いんじゃないか」という判断を、私はしました。
今後の起こりうるかもしれない改修で、B案は破綻してA案になることはあっても、A案が破綻してB案になることは無いからです。
そして破綻したらテスト工数が膨らみます。
だったら最初からA案にしとけってことです。

さてコードレビューを行う場合、通常はもう少し表面的な部分でのレビューを行います。
コードの書き方には、ローカルルールが定められていることがあり、まずそのルールに沿ってるかのチェックが行われます。
次に定石と呼ばれる『こういう処理の時はこう書いた方が良いよ』というものがあり、それらの指摘になります。
あとはコードの流れを読んで、気になった点の指摘とかですね。
例えば、「ここNULLチェックしてなくて大丈夫ですか？」とか「ここの変数名はもっと分かりやすい方が良くないですか？」とかです。

コードレビューは、あくまで書かれたコードに対するレビューですので、今回のような場合「どうしてB案で書かないのですか？」といった指摘が来るかもしれません。
コードレビュー者は通常、アルゴリズムまではレビューしません。
何故ならアルゴリズムレビューは、担当者と同程度のコード周辺の予備知識や事前情報が必要になり、負荷が一気にかかるからです。
なのでそのような場合に、きちんとレビュー者に対して説明を行い、納得してもらえるかどうかが、エンジニアとしてのスキルの高さになってくるのではないかと思います。

ちなみにこの件の結末はどうなったかというと、色々話している最中に、さらにコード周辺に関する不具合報告が来て、結局両案どちらを選択してもテスト範囲が広くなることが分かったため、当初諦めていた別アルゴリズムで対応するという方針に変わり、結局A案B案どちらもボツということになりました(笑)

無駄だったでしょうか？
いえいえ、こういったセルフアルゴリズムレビューを行い、ロジックを立てていくということを、普段から積み重ねていくことが大事だと思った一件でした。

ぴっくるも人が少し増えて、こういった話が普段の会話から出てくるのは良いことだなと思います。