回答（全11件）
質問者が選んだベストアンサー
回答の評価を上げる
以下のような回答は評価を上げましょう。
- 正しい回答
- わかりやすい回答
- ためになる回答
評価が高い回答ほどページの上位に表示されます。
12
回答の評価を下げる
以下のような回答は評価を下げられます。
- 間違っている回答
- 質問の回答になっていない投稿
- 不快な投稿
評価を下げる際は理由をコメントしてください。
メソッドがいるいらないの前に、fetch_data_todayはこう書くべきじゃないでしょうか？
def fetch_data_today
fetch_data(Date.today)
endfetch_dataをオーバーライトした時にうまく動作しないと思います。
また、
fetch_dataと(Date.today)の間にある空白は余計です。HogeManager::PointBack.fetch_by_date(date)を見る限り、メソッドあとの()は空白をあけないコーディングスタイルかとも思いますが、そうであれば、空白をあけないと言うことに統一すべきです。初学者はコーディングスタイルを軽視する傾向がありますので、こういう所はしつこく教えて、癖を付けさせるべきです。
さて、いるいらないの話に戻りますが、私は「そのメソッドにとって一般的な動作とは何か？」で決めます。
fetch_dataが「指定した日付のデータを取得する」ならtodayをデフォルトにはしません。逆に「データを取得する。日付を指定することも可能で、デフォルトは今日のデータである」ならtodayをデフォルトにします。同じじゃないかと思いますが、微妙にニュアンスが違います。日付を指定することが重要なのか、日付はわりとどうでもいいものなのかで変わってくると思います。
なお、デフォルト値を入れない場合でも、私だったら
fetch_data_todayは作りません。呼び出し側でfetch_data(Date.today)と書いてもコーディング量がそれほど変わらないし、何をしたいかが自明だからです。あ、どっちにしてもいらないです。2015/11/27 07:24 投稿
コメント（1）
回答の評価を上げる
以下のような回答は評価を上げましょう。
- 正しい回答
- わかりやすい回答
- ためになる回答
評価が高い回答ほどページの上位に表示されます。
8
回答の評価を下げる
以下のような回答は評価を下げられます。
- 間違っている回答
- 質問の回答になっていない投稿
- 不快な投稿
評価を下げる際は理由をコメントしてください。
プロジェクトや会社としてどのような命名/実装方針にするかが決まっていないと答えが出ないかとは思います。
ざっとした指示で実装しちゃってから「コードレビュー」で方針について話されるてしまうと、「そんなの先に言ってよ」と意固地になってしまうこともあるかと思います。（指示する側からすると、「勝手に判断せずに指示を仰いでよ」ですが。。。）
個人的な感覚としては
- fetch_data()のデフォルト値がDate.todayと言うのはメソッド名からはわからないので避けたい
- fetch_data_today()は引数が減る&役割が自明なので使用時のバグは減り、単体で見れば悪くない実装であるが、では何故fetch_data_tomorrow()やfetch_data_yesterday()が実装されていないのかについて明確な基準が必要。基準が定義出来ないのであれば実装するべきでは無い。
という感じかなと思います。
2015/11/27 00:54 投稿
コメント（1）
2015/11/30 19:57
ご回答ありがとございます。
レビュー文化が浅く、実装方針をレビューに落としこむ所が甘かった痛感しております。
チーム全体で課題としてレビュー方針をブラッシュアップしていきたいと思います。
ありがとうございました！
回答の評価を上げる
以下のような回答は評価を上げましょう。
- 正しい回答
- わかりやすい回答
- ためになる回答
評価が高い回答ほどページの上位に表示されます。
7
回答の評価を下げる
以下のような回答は評価を下げられます。
- 間違っている回答
- 質問の回答になっていない投稿
- 不快な投稿
評価を下げる際は理由をコメントしてください。
どっちの場合でもありだと思います。
個人的なコードレビューについて思うのは、許容できる範囲の書き方であれば指摘自体不要だと思います。
レビューなのでもう少しクラス全体を見た作りの指摘がほしいところです。
2015/11/27 01:36 投稿
コメント（1）
回答の評価を上げる
以下のような回答は評価を上げましょう。
- 正しい回答
- わかりやすい回答
- ためになる回答
評価が高い回答ほどページの上位に表示されます。
7
回答の評価を下げる
以下のような回答は評価を下げられます。
- 間違っている回答
- 質問の回答になっていない投稿
- 不快な投稿
評価を下げる際は理由をコメントしてください。
私なら、やはりfetch_data_todayは不要と考えますね。
使う側からしてみたらいつの期間のデータを取得しやすいと思ったのであえて実装しました新人君は気を回しすぎですね。プロジェクトの予算を別のプロジェクトのために使うことになってしまいます。ご質問にある汎用的とは、「使い回しができる」という意味であって、「様々な状況を想定する」という意味ではないはずです。新人君は後者の考えをしてしまっていますね。「使う側からしてみたら」という場合、使う側の意見を聞かなくてはなりません。新人君は誰かに意見を聞いたのでしょうか。
～、そういう手間をかけないように誰でもでも分かるようにメソッドを用意したのです他の誰かが作ったモジュール／ライブラリを使う場合、ソースなりドキュメントなりを見ないで利用するなどということは考えられません。
fetch_dataが指定した日付のデータを取得するという機能だということも、何かを見なければ判りませんから。
もし私が使う側で何の予備知識もなしに（ソースも見ないで）二つのメソッドを見比べたら、真っ先に『
fetch_data_todayはfetch_data(Date.today)と何か違うのか？』と疑問に思うかもしれません。同じですと言われたら、「じゃぁ、なんで分けたの？」と問うかもしれません。
結局は説明が必要なのです。であれば、説明が簡単で実装も簡単な方がいいに決まっています。
経験上、日付絡みの機能で「省略時は現在の日時」というライブラリは結構多いですから、違和感も感じません。
追記
前述の「もし私が使う側で～」と書いたところの補足ですが、同じ目的を達するための手段が二通りある場合、「どっちがいいの？」という素朴な疑問がわくことは必至であり、親切のつもりがかえって混乱させるという事態を招く可能性があるということを指摘したかったのです。
2015/11/27 08:22 投稿
コメント（1）
回答の評価を上げる
以下のような回答は評価を上げましょう。
- 正しい回答
- わかりやすい回答
- ためになる回答
評価が高い回答ほどページの上位に表示されます。
6
回答の評価を下げる
以下のような回答は評価を下げられます。
- 間違っている回答
- 質問の回答になっていない投稿
- 不快な投稿
評価を下げる際は理由をコメントしてください。
仕様書や、レビューチェックリストは存在しないのでしょうか？
あらかじめ定められているルールから逸脱しているのならともかくとして、
そうではなくて、細かな部分をインプリメンターに任せている環境であれば、よほどおかしい場合を除いて、
意見は言いますけど最終的にインプリメンターの意見を尊重します。
ルールの無いプログラムのスタイルやインタフェースの方針などの細かなところで自分の理想100％を目指してしまうと、何のためのレビューなのか目的を見失うきがします。
自分のレビューは80点主義でやっています。
個人的には、ルール逸脱や誤っていたり、明らかに大きく非効率な場合を除いては、細かなコードに修正は求めません。もっと上流の部分のクラス構成や、エラー処理に過不足が無いか（本来はもっと前段で行われるべき事と思いますが、プロトタイピングっぽい手法の時には）の方を重点的に見ます。
2015/11/27 07:49 投稿
コメント（1）
回答の評価を上げる
以下のような回答は評価を上げましょう。
- 正しい回答
- わかりやすい回答
- ためになる回答
評価が高い回答ほどページの上位に表示されます。
5
回答の評価を下げる
以下のような回答は評価を下げられます。
- 間違っている回答
- 質問の回答になっていない投稿
- 不快な投稿
評価を下げる際は理由をコメントしてください。
わざわざ別メソッドとして定義して引数だけかえて元のメソッドを変えるのはあまり意味が無いので修正するべきだとおもうよー
引用テキストただfetch_dataだけで呼び出した場合、いつの期間のデータなのか分からないじゃないですか
この２人の意見はどちらも間違っていないと思います。
ここで議論されているモジュールがどれくらい使用されるかわかりませんが、個人的には以下の対応が好みです。
- まずは
fetch_data()のみ定義
-- 引数に
Date.todayというデフォルト値は設定しない
- 『実行時日時のデータを取得』という処理が複数箇所で呼ばれるようになったら、
fetch_data_today()として切り出す。（3回ルールなどの決まりがあると判断しやすい）
コードレビューとしては、とてもいい議論だと思いました笑
この回答が少しでも参考になれば幸いです。
2015/11/27 00:14 投稿
コメント（1）
回答の評価を上げる
以下のような回答は評価を上げましょう。
- 正しい回答
- わかりやすい回答
- ためになる回答
評価が高い回答ほどページの上位に表示されます。
4
回答の評価を下げる
以下のような回答は評価を下げられます。
- 間違っている回答
- 質問の回答になっていない投稿
- 不快な投稿
評価を下げる際は理由をコメントしてください。
私なら次の 2 点をコメントします。
1. メソッド名を変更する。
----------------------
fetch_data(date)
==>
fetch_data_by_date(date)
パラメータのデフォルト値は設けない。
理由：
* パラメータに Date を指定するんだということが予想しやすくなるから。
* 利用側が date を意識せずに fetch することを避ける為に パラメータ省略の呼び出しはさせない。
2. 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() を作りたくないことも、これと同じような気持ちがあるからです)
2015/11/27 07:13 投稿
コメント（1）
2015/11/27 10:43
私もkatoyさんの意見に賛成です。
デフォルト引数にすると、「today」という情報が見えなくなるので、
fetch_data_by_dateというメソッド名にしておいて、引数で明示的にDate.todayを渡すようにするのがベターかと思います。
回答の評価を上げる
以下のような回答は評価を上げましょう。
- 正しい回答
- わかりやすい回答
- ためになる回答
評価が高い回答ほどページの上位に表示されます。
2
回答の評価を下げる
以下のような回答は評価を下げられます。
- 間違っている回答
- 質問の回答になっていない投稿
- 不快な投稿
評価を下げる際は理由をコメントしてください。
彼の理論で行くと明日の日時とか昨日の日時とか来週の…などなど作ることになりますよね。
今回の「汎用的に作ってね」から外れるのでいい実装とは言えないと思います。
このメソッドを実際に使うクライアントがラップし「今日の日時」を取得するメソッドにするのならいいかなと思いました。
2015/11/27 00:04 投稿
コメント（1）
回答の評価を上げる
以下のような回答は評価を上げましょう。
- 正しい回答
- わかりやすい回答
- ためになる回答
評価が高い回答ほどページの上位に表示されます。
2
回答の評価を下げる
以下のような回答は評価を下げられます。
- 間違っている回答
- 質問の回答になっていない投稿
- 不快な投稿
評価を下げる際は理由をコメントしてください。
こんにちは。
ちょっと比較してみました。
hoge.fetch_data_today # (1)
hoge.fetch_data # (2)
hoge.fetch_data(Date.today) # (3)(2)は、fetch_data()関数の仕様と、引数を省略した時今日のデータを取り出すことの確認が必要ですが、ソースをひと目見れば自明ですね。そして、書く時ちょっとだけ楽です。
(3)は、fetch_data()関数の仕様だけ知っていれば自明ですね。書く時ちょっとだけ手間ですが、読む方は楽です。
なので、私なら(3)(デフォルト引数無し)を選びます。
「今日のデータ」を取り出す場所が結構多い時は(2)も有りと思います。
【追記】
もう少し説得力がありそうな例を思いつきました。
HogeManagerを使っているプログラムをデバッグ中、ある程度絞り込んで日付処理周りに想定外の動作があることまで分かったとします。この時、fetch_data_todayの機能が自分の理解通りの機能であることを確認すると思います。(もしかすると、00:00:00が今日なのか昨日なのかでハマっているかも知れませんし。)
これくらい短いプログラムならソースを見た方が速いです。で、その確認が容易なのは、(3)→(2)→(1)の順序です。
つまり、後輩君の主張は一見使う人の立場に立っているかのように見えますが、このようにデバッグする場面ではかなり不親切です。
(2)は書くのが楽というメリットがありますが、(1)は「コード量が増える」、「正確な動作が隠される」と言う問題があり、無駄に工数をかけて作り、使う時も無駄に工数を消費するプログラムになっています。
2015/11/27 01:02 投稿
コメント（1）
回答の評価を上げる
以下のような回答は評価を上げましょう。
- 正しい回答
- わかりやすい回答
- ためになる回答
評価が高い回答ほどページの上位に表示されます。
1
回答の評価を下げる
以下のような回答は評価を下げられます。
- 間違っている回答
- 質問の回答になっていない投稿
- 不快な投稿
評価を下げる際は理由をコメントしてください。
レビューイーのスキルレベルやプロジェクトの方針によってアプローチは違うと思います。
今回はレビューイーは新卒エンジニアとのことですので、教育という意味でも納得がいくまでなぜ不適切なのかを丁寧に説明していく必要があろうかと思います。そのうち、冗長なコードを保守するような経験をすると、身をもって理解できるはずです。
2015/11/27 09:17 投稿
コメント（1）
2015/11/30 19:47
ご回答ありがとございます。
後日、新人から指摘事項の納得してもらえました。
私もスキルレベル、プロジェクト方針によるアプローチをしっかり定めて丁寧なレビューを心がけたいとおもいます、ありがとうございました！
回答の評価を上げる
以下のような回答は評価を上げましょう。
- 正しい回答
- わかりやすい回答
- ためになる回答
評価が高い回答ほどページの上位に表示されます。
1
回答の評価を下げる
以下のような回答は評価を下げられます。
- 間違っている回答
- 質問の回答になっていない投稿
- 不快な投稿
評価を下げる際は理由をコメントしてください。
感情論になってますよね？
fetch_data_todayメソッドは不要じゃない？
の所で、新人君が意地になってると感じました。kazukinozawaさんもつられてる感じでしょうか。
…で、実際問題、_today メソッドは使われそうなんですか？単純なラッパーになっているので、
メモリへのインパクトもそれほど無さそうですが、_today() があるとそんなに大変ですか？
モジュール仕様書を書く場合は、１ページ増えるかも知れませんが…
これ以上議論してもこじれそうなので、_todayを温存しつつ、fetch_dataにデフォルト引数を
追加させましょう。「あとは、使いたい方、使ってもらおうね。」で終了です。時間がもったいない。
昔ながらの「ステップ単価」で見積もりを書く案件でも無いでしょうし、プロジェクトが
一段落してから再レビューして、_todayが使われて無かったらそれを理由にコメントアウトして
貰いましょう。
2015/11/28 00:53 投稿
コメント（1）
2015/11/30 19:22
ご回答ありがとございます。
新人君がムキになっていた部分もあるので、後日冷静になってもらい。
現状の仕様だと_today()は実装しないでまとまりました。
今後のレビュー基準を定める上で参考にさせて頂きます、ありがとございました！
関連した質問
-
解決済
Ruby モジュールの特徴について
Rubyを勉強していたら、 モジュールという用語がでてきました。 作り方はわかるのですが、モジュールの特徴がいまいちわかりません。 クラスと似ていると思うのですが、わざわざモジュ
-
解決済
Ruby メソッドをオブジェクトに取り組みたい
Ruby初心者です。 メソッドをオブジェクトに取り組みたいのですが、 このようなことはできますでしょうか？ どなたか教えていただけないでしょうか？
-
解決済
Ruby クラスメソッドの定義について
Ruby初心者です。 現在、以下のような方法でクラス定義をしているのですが、 他に何か良い定義の仕方があれば教えてもらえませんか？ class Test def Test.pr
-
受付中
Ruby 日付の計算で１日ずれてしまう
ライブラリ（date)を使えばすぐに解決できてしまう問題ですが、Rubyの勉強のために あえて書かずにコードを書いています。 次のようなコードを書きました。 def get_we
同じタグがついた質問を見る
- Ruby
1316questions
Rubyはプログラミング言語のひとつで、オープンソース、オブジェクト指向のプログラミング開発に対応しています。
- Rails
953questions
Ruby on Railsは、オープンソースのWebアプリケーションフレームワークです。「同じことを繰り返さない」というRailsの基本理念のもと、他のフレームワークより少ないコードで簡単に開発できるよう設計されています。
- コードレビュー
7questions
コードレビューは、ソフトウェア開発の一工程で、 ソースコードの検査を行い、開発工程で見過ごされた誤りを検出する事で、 ソフトウェア品質を高めるためのものです。