[Java]GitHubを用いたチームでの開発のコードレビューで指摘するべきか迷った。

Question

暇つぶしの質問です。以前、参画していたプロジェクトでチームでのシステム開発を行っていたのですが、GitHubでコミットしたコードをレビューしてもらいOKがでたらマージするというよくあるルールで開発を進めておりました。

自分もレビューする事があったのですが、下記の様なリファクタリングできそうなコードを指摘するべきか悩みました。私のレビューする上での思想的な物は、漠然と考えておりますが、明らかにおかしい物は突っ込む、微妙で意見が割れそう、嫌がられそうな物は、チームの和が乱れそうなので突っ込まないって感じですが、しかしせっかくレビューしてるんだから思った事は言うべきで、塵も積もれば良いコードになるから言うべきとも思いました。皆さんは下記の様なの突っ込みますか。その理由とかも教えて欲しいです。

if(list.size() > 0){
    ~~~
}

listって変数の方はListです。isEmpty()ってメソッドがあるからそれ使えばって指摘しようと思いました。

if(~~~){
    ~~~
}else{
    if(~~~){
        ~~~
    }
}

else ifでいいじゃんって指摘しようと思いました。

int a;
if(~~~){
    a = 1;
}else{
    a = 2;
}

三項演算子でいいんじゃない？って指摘しようと思いました。

Answer 1

コードレビューによる指摘で和が乱れるのは、指摘内容そのものではなく、指摘の言い方や、そもそも指摘を受けた人が指摘されるのに慣れしていない、など、コードレビューとは別次元の問題なのだと思うところもありますが、なかなかそううまく割り切れないケースもありますね…。

チームメンバーの中にはコードレビューの慣れ・不慣れもあるかと思いますし、指摘の言い方次第で印象が変わります。
例示していただいた3例については、明らかに読みやすくなりますので、ぜひともその指摘をした方が良いですね。
「こうするとより読みやすい！」「今回はこちらの方を推奨しています」などなど。

何度かレビューがあって、似たような指摘が散見した場合には、チーム内で別途振り返りの会にて議題にあげて、コーディング規約の見直しや、コードフォーマッタの適用・静的解析ツールの導入の検討など行っていけると、コードレビューに対する"見えない壁"が取り払われていくのではないかと。

Comments

[退会済みユーザー]

ご回答ありがとうございます。指摘すべき理由や指摘方法等をご回答頂いたのでベストアンサーとさせて頂きました。

他の方もご回答ありがとうございました。微妙な指摘と書きつつも私としては、直すべき内容と思っていたのですが、色々な意見があり大変勉強になりました。

Answer 2

上のlist.isEmptyと真ん中のelse ifには賛成ですが、下については戦争になるネタだと思います。
明らかにそのifに対して割り当てる値が1つである場合は三項演算子でもよいと思いますが、今後の拡張で処理が増える可能性がある部分であれば三項演算子にしておかず、if elseのままにしておくということも考慮します。そして、私は基本的にそういったコードの修正時のことを考えて三項演算子を使わないで常にif elseを書くようにしています。中途半端に三項演算子を使うと、ここは三項演算子なのにここはif else、と見た目の統一性がなくなるためです。こういう考え方もあるので、一概に三項演算子が正義、if elseは悪、というのはないので、その指摘に強制力は無いと思います。もしプロジェクトで「ルール」として決める覚悟があるなら、「三項演算子のほうがふさわしいとこは三項演算子を使う」と提言するとよいでしょう。

Comments

[退会済みユーザー]

ご回答ありがとうございます。

コードが短くなるから三項演算子は正義と単純に考えておりましたが、色んな観点があるのですね。今後は気をつけたいと思います。

[masaya_ohashi]

私はコードが短くなるからという理由で「ifの{}を省略する」という人がいたらその人を地獄の釜に投げ込みたくなるので、短い＝絶対正義ではないと思います。そのへんはプロジェクトで折り合いをつけた妥協点を見つけるのが一番かと思います。

Answer 3

私なら全てに突っ込み（というか確認）を入れます。間違いを指摘する意図ではなく、なぜそうしているのかを知りたいからです。
合理的な理由があってそうしているならそのままにします。

ちなみに、下二つのif文に関しては、私もあえてそうすることはよくあります。例えば、意図通りに条件分岐するかどうかの確認にはブレークポイントやトレースポイントが常套手段ですが、1行にまとめてしまうとそれができないのです。
もちろん、正しいことが確認できた段階でリファクタリングすることもよくあります。

Answer 4

私の場合はコメントします。
ただし、このぐらいの内容であれば、このように記載してはどうかという提案としてコメントを残します

Answer 5

まず、このコードで突っ込むとすると、２点あります。

２番目の、

if(~~~){
    条件Ａ
}else{
    if(~~~){
        条件Ｂ
    }
}

ですね。
これは、

if(~~~){
    条件Ａ
}else{
    if(~~~){
        条件Ｂ
    }else{
        条件C
    }
}

と書くとわかるように、条件Ｃの判定ができていません。
これは、問題です。もし条件Ｃが存在しないことが、確定であれば、else ifという話しになるかと思いますが・・・そこまで気づいて指摘することが必要かと。

次に、最後の

int a;
if(~~~){
    a = 1;
}else{
    a = 2;
}

三項演算子云々の前に、なんで、「1」とか「2」なの？という所です、マジックナンバーが、コードに含まれるとあとで意味がわかりません。このサイトの質問目的で、「1」とか「2」とされているなら、それはまた別の話ですが。

Answer 6

私は基本的に、意見が割れてもツッコむ派

isEmpty()に関してはisEmpty()のほうが内部の演算回数がすくないのではなかったでしょうか？
（ちょっとうろ覚えですいませんが）
(list.size() > 0)だと、内部的にはサイズを数えて、０と比較なので、若干処理が遅いはず。

個人的には、それよりも(list.size() > 0)じゃなくて( 0 < list.size() )に直したい(^_^;)
うちのルールだと基本的に比較演算子は < か <= で >と>=は使わないです。
「つねに右が大きいようにすれば見やすい」と理由です。

Comments

[swordone]

listの実体にもよりますが、ArrayListの場合はisEmpty()は「サイズが0か否か」を判定してboolean値を返します。size()含めサイズは数えるのではなく、フィールド変数の数値として持っている値を使ったと思います。なのでboolean反転するisEmptyの方が1回分多く処理をしていることになりますかね。

[jm1156]

なるほど、そうだったのですか。
勉強になります。