コードレビューにおける指摘事項の強要について。

Question

開発では新人君のコードレビューをおこなっています。
今回指摘した点において新卒エンジニア君と大議論に発展し、まだ答えがみつかっていません。。
私の指摘事項だけでは納得出来ないようで修正することを強要することも戸惑っています。
みなさまの声を聞かせていただければと思います。。

会話内容

ぼく「作成するモジュールは別のプロジェクトでも使用するので汎用的につくるっておいてねー」

新人「作りました、レビューお願いします。」

module HogeManager
  # fetch data of specified day
  def fetch_data(date)
    data = HogeManager::PointBack.fetch_data_by_date(date)
  end

  # fetch the data of today
  def fetch_data_today
    HogeManager.fetch_data (Date.today)
  end
end

ぼく「上記のコードの中でfetch_data_todayメソッドは不要じゃない？」

module HogeManager
  # fetch data of specified day
  def fetch_data(date = Date.today)
    data = HogeManager::PointBack.fetch_by_date(date)
  end
end

ぼく「こうのように引数指定がない場合は実行時日時のデータを取得。引数が渡された場合はその日付で処理するで済むのよね」
「わざわざ別メソッドとして定義して引数だけかえて元のメソッドを変えるのはあまり意味が無いので修正するべきだとおもうよー」

新人「先輩の指摘内容は理解できます」
「ただfetch_dataだけで呼び出した場合、いつの期間のデータなのか分からないじゃないですか？」
「使う側からしてみたらいつの期間のデータを取得しやすいと思ったのであえて実装しました」

ぼく「コメントに書いとくだけで良いんじゃない？そんな大きいモジュールじゃないし、このレベルだったら誰でも分かるでしょ」
新人君「それだと１回ソース見ないと分からないじゃないですか、そういう手間をかけないように誰でもでも分かるようにメソッドを用意したのです」

ぼく「rdocにまとめられるし、gemにするんだったらREADMEに書いときゃいいじゃん」

以下堂々めぐり。

Comments

[muro]

こちらの質問が他のユーザから「プログラミングに関係がない質問」という評価を受けています
teratailでは、プログラミングに関して困っていることがないと思われる質門を推奨していません。
具体的に困っている理由や解決したいことを明確に記入していただくと、回答が得られやすくなります。

Answer 1

メソッドがいるいらないの前に、fetch_data_todayはこう書くべきじゃないでしょうか？

  def fetch_data_today
    fetch_data(Date.today)
  end

こうしないとfetch_dataをオーバーライトした時にうまく動作しないと思います。
また、fetch_dataと(Date.today)の間にある空白は余計です。HogeManager::PointBack.fetch_by_date(date)を見る限り、メソッドあとの()は空白をあけないコーディングスタイルかとも思いますが、そうであれば、空白をあけないと言うことに統一すべきです。初学者はコーディングスタイルを軽視する傾向がありますので、こういう所はしつこく教えて、癖を付けさせるべきです。

さて、いるいらないの話に戻りますが、私は「そのメソッドにとって一般的な動作とは何か？」で決めます。fetch_dataが「指定した日付のデータを取得する」ならtodayをデフォルトにはしません。逆に「データを取得する。日付を指定することも可能で、デフォルトは今日のデータである」ならtodayをデフォルトにします。同じじゃないかと思いますが、微妙にニュアンスが違います。日付を指定することが重要なのか、日付はわりとどうでもいいものなのかで変わってくると思います。

なお、デフォルト値を入れない場合でも、私だったらfetch_data_todayは作りません。呼び出し側でfetch_data(Date.today)と書いてもコーディング量がそれほど変わらないし、何をしたいかが自明だからです。あ、どっちにしてもいらないです。

Comments

[knknkn]

ご回答ありがとございます。
今後のレビュークオリティを上げる上で大変参考になりました。

Answer 2

プロジェクトや会社としてどのような命名/実装方針にするかが決まっていないと答えが出ないかとは思います。
ざっとした指示で実装しちゃってから「コードレビュー」で方針について話されるてしまうと、「そんなの先に言ってよ」と意固地になってしまうこともあるかと思います。（指示する側からすると、「勝手に判断せずに指示を仰いでよ」ですが。。。）

個人的な感覚としては

• fetch_data()のデフォルト値がDate.todayと言うのはメソッド名からはわからないので避けたい
• fetch_data_today()は引数が減る&役割が自明なので使用時のバグは減り、単体で見れば悪くない実装であるが、では何故fetch_data_tomorrow()やfetch_data_yesterday()が実装されていないのかについて明確な基準が必要。基準が定義出来ないのであれば実装するべきでは無い。

という感じかなと思います。

Comments

[knknkn]

ご回答ありがとございます。
レビュー文化が浅く、実装方針をレビューに落としこむ所が甘かった痛感しております。
チーム全体で課題としてレビュー方針をブラッシュアップしていきたいと思います。
ありがとうございました！

Answer 3

私なら、やはりfetch_data_todayは不要と考えますね。

使う側からしてみたらいつの期間のデータを取得しやすいと思ったのであえて実装しました

新人君は気を回しすぎですね。プロジェクトの予算を別のプロジェクトのために使うことになってしまいます。ご質問にある汎用的とは、「使い回しができる」という意味であって、「様々な状況を想定する」という意味ではないはずです。新人君は後者の考えをしてしまっていますね。「使う側からしてみたら」という場合、使う側の意見を聞かなくてはなりません。新人君は誰かに意見を聞いたのでしょうか。

～、そういう手間をかけないように誰でもでも分かるようにメソッドを用意したのです

他の誰かが作ったモジュール／ライブラリを使う場合、ソースなりドキュメントなりを見ないで利用するなどということは考えられません。fetch_dataが指定した日付のデータを取得するという機能だということも、何かを見なければ判りませんから。

もし私が使う側で何の予備知識もなしに（ソースも見ないで）二つのメソッドを見比べたら、真っ先に『fetch_data_todayはfetch_data(Date.today)と何か違うのか？』と疑問に思うかもしれません。同じですと言われたら、「じゃぁ、なんで分けたの？」と問うかもしれません。

結局は説明が必要なのです。であれば、説明が簡単で実装も簡単な方がいいに決まっています。
経験上、日付絡みの機能で「省略時は現在の日時」というライブラリは結構多いですから、違和感も感じません。

追記

前述の「もし私が使う側で～」と書いたところの補足ですが、同じ目的を達するための手段が二通りある場合、「どっちがいいの？」という素朴な疑問がわくことは必至であり、親切のつもりがかえって混乱させるという事態を招く可能性があるということを指摘したかったのです。

Comments

[knknkn]

後日新人君から気を回し過ぎた点については報告されました。
レビュークオリティを上げる上で大変参考になりました、ありがとうございます！

Answer 4

どっちの場合でもありだと思います。

個人的なコードレビューについて思うのは、許容できる範囲の書き方であれば指摘自体不要だと思います。
レビューなのでもう少しクラス全体を見た作りの指摘がほしいところです。

Comments

[knknkn]

ご回答ありがとございます。
許容できる書き方とはを改めて考えてさせられました。
ありがとうございました。

Answer 5

仕様書や、レビューチェックリストは存在しないのでしょうか？

あらかじめ定められているルールから逸脱しているのならともかくとして、
そうではなくて、細かな部分をインプリメンターに任せている環境であれば、よほどおかしい場合を除いて、
意見は言いますけど最終的にインプリメンターの意見を尊重します。

ルールの無いプログラムのスタイルやインタフェースの方針などの細かなところで自分の理想100％を目指してしまうと、何のためのレビューなのか目的を見失うきがします。
自分のレビューは80点主義でやっています。

個人的には、ルール逸脱や誤っていたり、明らかに大きく非効率な場合を除いては、細かなコードに修正は求めません。もっと上流の部分のクラス構成や、エラー処理に過不足が無いか（本来はもっと前段で行われるべき事と思いますが、プロトタイピングっぽい手法の時には）の方を重点的に見ます。

Comments

[knknkn]

ご回答ありがとございます。
頂いた指摘について、細かくなり過ぎない点改めて考えてるいい機会となりました。
ありがとうございます！

Answer 6

私なら次の 2 点をコメントします。

1. メソッド名を変更する。

---

fetch_data(date)
==>
fetch_data_by_date(date)
パラメータのデフォルト値は設けない。

理由：

• パラメータに Date を指定するんだということが予想しやすくなるから。
• 利用側が date を意識せずに fetch することを避ける為に パラメータ省略の呼び出しはさせない。

1. fetch_data_today() は module 中では提供しない。

---

理由

• Date.new(...) に対して Date.today が存在することで
  Date のレベルですでに today の特別扱いは済んでいます。
  fetch_data API のレベルで再度 today を特別視するのは冗長と思います。

fetch_data_by_date(Date.today)
fetch_data_today()
は today の情報がパラメータにあるのか、メソッド名にあるかの差だけです。

でも、
他の module, class で today の特別視が多い、
fetch_data_today() が欲しいという意見が多い
ならば、 fetch_data_today() を作ってもよいとは思います。

蛇足：
fetch_data_2015_11_30() を
fetch_data_by_date(Date.new(2015, 11, 30))
に呼び変えるようなメタプログラムをすることが可能と思います。
でも そんなことは普通はしないです。
(fetch_data_today() を作りたくないことも、これと同じような気持ちがあるからです)

Comments

[k.tada]

私もkatoyさんの意見に賛成です。
デフォルト引数にすると、「today」という情報が見えなくなるので、
fetch_data_by_dateというメソッド名にしておいて、引数で明示的にDate.todayを渡すようにするのがベターかと思います。

Answer 7

わざわざ別メソッドとして定義して引数だけかえて元のメソッドを変えるのはあまり意味が無いので修正するべきだとおもうよー

引用テキストただfetch_dataだけで呼び出した場合、いつの期間のデータなのか分からないじゃないですか

この２人の意見はどちらも間違っていないと思います。
ここで議論されているモジュールがどれくらい使用されるかわかりませんが、個人的には以下の対応が好みです。

• まずはfetch_data()のみ定義

-- 引数にDate.todayというデフォルト値は設定しない

• 『実行時日時のデータを取得』という処理が複数箇所で呼ばれるようになったら、fetch_data_today()として切り出す。（3回ルールなどの決まりがあると判断しやすい）

コードレビューとしては、とてもいい議論だと思いました笑
この回答が少しでも参考になれば幸いです。

Comments

[knknkn]

ご回答ありがとうございます。
大変参考にさせていただきました。ありがとうございました！

Answer 8

こんにちは。

ちょっと比較してみました。

hoge.fetch_data_today          # (1)
hoge.fetch_data                # (2)
hoge.fetch_data(Date.today)    # (3)

(1)は、fetch_data()関数の仕様と、fetch_data_todayが今日のデータを取り出すものであることの確認が必要です。字面からその旨理解できそうですが、一度は仕様書かソースを見るか、実際に動かしてみて確認する必要があります。勘違いとか間違いって結構多いですから、確認を怠ると結構酷い目に合います。
(2)は、fetch_data()関数の仕様と、引数を省略した時今日のデータを取り出すことの確認が必要ですが、ソースをひと目見れば自明ですね。そして、書く時ちょっとだけ楽です。
(3)は、fetch_data()関数の仕様だけ知っていれば自明ですね。書く時ちょっとだけ手間ですが、読む方は楽です。

なので、私なら(3)(デフォルト引数無し)を選びます。
「今日のデータ」を取り出す場所が結構多い時は(2)も有りと思います。

【追記】
もう少し説得力がありそうな例を思いつきました。

HogeManagerを使っているプログラムをデバッグ中、ある程度絞り込んで日付処理周りに想定外の動作があることまで分かったとします。この時、fetch_data_todayの機能が自分の理解通りの機能であることを確認すると思います。(もしかすると、00:00:00が今日なのか昨日なのかでハマっているかも知れませんし。)
これくらい短いプログラムならソースを見た方が速いです。で、その確認が容易なのは、(3)→(2)→(1)の順序です。

つまり、後輩君の主張は一見使う人の立場に立っているかのように見えますが、このようにデバッグする場面ではかなり不親切です。
(2)は書くのが楽というメリットがありますが、(1)は「コード量が増える」、「正確な動作が隠される」と言う問題があり、無駄に工数をかけて作り、使う時も無駄に工数を消費するプログラムになっています。

Comments

[knknkn]

事細かく書いて頂きありがとうございます！
今後のレビュークオリティを上げるうえで大変参考になりました。

Answer 9

初めて投稿します．

みなさんの意見が大変面白く，自分も少しだけ参加させていただきたく投稿します．
（すいません，自分はRuby技術者じゃないのですが…）

自分ならメソッドのデフォルトパラメータにDate.todayみたいなのをダイレクトに書くようなことは絶対にしません．

なぜなら，それを呼び出したタイミングそのものが日時に依存してしまうためです．

ちょっとわかりにくい言い方ですが，たとえば「2015/12/01のデータを表示します」と表示しておきながら，実際のメソッドを使うタイミングで日替わりが起きてしまった場合，データの保証がまるでできなくなります（途中から12/02のデータを拾ってきてたり…）．

自分は24/7運用のデータ収集システムを手掛けることが多いのですが，どうやっても時替わり，日替わり，月替り，年替わりが避けられません．こういう時，関数内で一度だけ必要な日時を変数に突っ込んでおき，それを引数にして呼び回ります（日付が変化しないように保証するわけです）．

したがって，概ね皆さんと同じような意見になってしまいますが，
・新人君のfetch_data_todayは冗長
・質問者のデフォルトパラメータはナンセンス
と感じました．

Answer 10

おおむね皆さんは新人さんは頭が固い、余計な事をしている、といったご感想のようなので、私は新人さんの肩をもってみましょう。

これが実践的なプロジェクトのコードで、スケジュール管理されているものなら、投稿者さんの意見が絶対的であり正しいです。

あくまで新人教育の観点から見れば、新人さんももし自発的にこれを組んだのならば、なかなか有望ですよ。
というのも、一番基本動作となるメソッドをまず用意し、用途によってラッピングして便利メソッドを提供する、というのはOSのAPIにも見られるとても柔軟な実装です。

例えば、仕様変更が入って、fetch_data_yesterdayも作ろうってなったとき、全てのメソッド内でデータを取得するクラスを直接参照することになります。
今回はPointBackがインナークラスみたいなので効果はあまりないですが、これがブラックボックスな外部クラスの場合にとても有効です。

ただ、新人君の、「ソース見なくてもメソッド名見れれば分かる」という言い分は、正しくもあり正しくもないです。
メソッド名から内容を推察できるかどうかは新人さんの主観でしかないので、他人も同様のはず、という思考は設計者の立場になったときに危険です。
新人さんの主張に対して、そこが今回一番注意すべきところです。
一方、命名で内容をある程度判断できるように心がけるのも、もちろん大事な事です。

投稿者さんはデフォルト値という解決を提案していますが、デフォルト値についての議論もまた色々とあると思うのです。
日付が引数ならデフォルトはまあ今日だろうという推察が立ちやすいですが、そうではない場合に色々と問題が出てくるので、教育面ではデフォルト値の利用シーンというものはとてもデリケートです。
これに失敗すると、デフォルト値を持つ引数がやたら多い使いにくいメソッドが爆誕します。

あと、新人さんに、君のコードのfetch_dataはpublicにすべきかprivateにすべきかprotectedにすべきか、どう思う？ なんて問いかけると、また新人さんをどのように教育するかの方針が分かるような気がします。

こんな感想もいかがでしょう。

Comments

[catsforepaw]

言われてみれば、新人君からすれば気を利かせたつもりなのに「余計なことはするな」的な表現だと萎縮してしまいますね。
基本的に会社はコストに対してシビアですから、要求にないことをいきなりやるのはいろいろ問題ということで私もそのような感じで書いてしまいました。
コードレビューからは外れますが、思いついたことをいきなり始めるのではなく、まずは「提案」という形で先輩なりチームリーダーなりに意見を求めるように促せばいいのかもしれませんね。

Answer 11

レビューイーのスキルレベルやプロジェクトの方針によってアプローチは違うと思います。
今回はレビューイーは新卒エンジニアとのことですので、教育という意味でも納得がいくまでなぜ不適切なのかを丁寧に説明していく必要があろうかと思います。そのうち、冗長なコードを保守するような経験をすると、身をもって理解できるはずです。

Comments

[knknkn]

ご回答ありがとございます。
後日、新人から指摘事項の納得してもらえました。
私もスキルレベル、プロジェクト方針によるアプローチをしっかり定めて丁寧なレビューを心がけたいとおもいます、ありがとうございました！

Answer 12

彼の理論で行くと明日の日時とか昨日の日時とか来週の…などなど作ることになりますよね。
今回の「汎用的に作ってね」から外れるのでいい実装とは言えないと思います。
このメソッドを実際に使うクライアントがラップし「今日の日時」を取得するメソッドにするのならいいかなと思いました。

Comments

[knknkn]

ご回答ありがとうございます。
再度レビューするにあたり参考にさせていただきました！

Answer 13

コードが、どうやこうやと言っている前に、ちゃんと設計してます？ コード書く前に設計して仕様書を書いていますか？ いきなりコードで、どうこうは素人です。ちゃんと設計文書書いて設計レビューして、コードを書く。若いうちから、コードをいきなり書いていると、システムが大きくなったときに、破綻するソフト組むようになっちゃいますよ。若いうちから、物づくりの教育しないとダメだと思いますよ。

Answer 14

解決済みで申し訳ありませんが、折角なので自分も

汎用的というだけではどっちが正しいとは正直確定はできません
そのモジュールの使用を想定しているプロジェクトでは今日のデータを取得するということが重要視されているものが多い場合はあった方がいいとも思います
ただ、処理はベストアンサーのついた「raccy」さんのように引数を指定して呼び出しなおす形じゃなきゃダメだと思いますが

なお、自分はVC#使いなので、Rubyの開発環境は分かりませんが、VisualStudioと同じように決まった書式でコメントを付けてやれば関数をポイントするなり入力途中で候補とその説明が表示されるようなものであるならば引数無しで呼ぶ場合に今日の日付を付ける形でも良いとは思います（もちろん今日のデータを取得することが重要視されている場合に限ります）
そうでない場合は引数省略で今日のデータを拾うとするのはミスが起きそうなので汎用的ではないかと思います
未来の情報を扱うシステムだと今日を重要視するパターンと、最新を重要視するパターンと翌日を重要視するパターンといくつかあるかと思いますので

Answer 15

自分が結構、新人さんよりのプログラムを書いていることにハッ気づいた。
確かに「他のプロジェクトでも使う汎用的に」って点から見ると、ないほうがいいように見えますね。
勉強になりました。

ふと気になったのが、Javaだと以下の様なコードは割りと見かけますよね。
これはニュアンスとしてデフォルト値を与えているに近いのかな。

int hoge(int param1, int param2){
// 何かしらの処理
}
int hoge(int param1){
  return hoge(param1,0) ;
}

Answer 16

感情論になってますよね？

fetch_data_todayメソッドは不要じゃない？

の所で、新人君が意地になってると感じました。kazukinozawaさんもつられてる感じでしょうか。
…で、実際問題、_today メソッドは使われそうなんですか？単純なラッパーになっているので、
メモリへのインパクトもそれほど無さそうですが、_today() があるとそんなに大変ですか？
モジュール仕様書を書く場合は、１ページ増えるかも知れませんが…

これ以上議論してもこじれそうなので、_todayを温存しつつ、fetch_dataにデフォルト引数を
追加させましょう。「あとは、使いたい方、使ってもらおうね。」で終了です。時間がもったいない。

昔ながらの「ステップ単価」で見積もりを書く案件でも無いでしょうし、プロジェクトが
一段落してから再レビューして、_todayが使われて無かったらそれを理由にコメントアウトして
貰いましょう。

Comments

[knknkn]

ご回答ありがとございます。
新人君がムキになっていた部分もあるので、後日冷静になってもらい。
現状の仕様だと_today()は実装しないでまとまりました。
今後のレビュー基準を定める上で参考にさせて頂きます、ありがとございました！