Conversation
02.calendar/calendar.rb
Outdated
| def check_inputs(inputs) | ||
| year = inputs['y'] | ||
| month = inputs['m'] | ||
| raise '-yの引数には1873以上の数値を入力してください' if !year.nil? && year.to_i < 1873 |
There was a problem hiding this comment.
コマンドを作るときは raiseで終わるのはダメじゃないですが、ちょっと乱暴です。
エラーメッセージを出力して、プロセスを終わる用のメソッドを使うと良いですね。調べてみてください。
There was a problem hiding this comment.
プロセスを終了するという意図を明確に示すために修正にて反映しました。
理解の確認をさせてください。
今回の修正はコミット名に記載した目的のためだと理解しました。
raiseは発生させたエラーをキャッチして何らかの処理を行うことが前提であるため、今回の場面で使用するのは少し乱暴。exitはプロセスを終了するというメソッドでより意図が明確になるという理解です。あっておりますでしょうか？
「コマンドを作るときは」という部分が分かりませんでした。私は「コマンドを作るときはWebシステムなどを作るときとは勝手が違うから」という意味で解釈したのですが、その「勝手」が何かわかりませんでした。
There was a problem hiding this comment.
raiseは発生させたエラーをキャッチして何らかの処理を行うことが前提であるため、今回の場面で使用するのは少し乱暴。exitはプロセスを終了するというメソッドでより意図が明確になるという理解です。あっておりますでしょうか？
はい、大丈夫です。
補足するとraiseしたときはエラーメッセージも例外名など本来のコマンドで不要なメッセージも出力されているので、そこも良くないですね。
「コマンドを作るときは」
すみません、分かりづらかったですね 🙏
「コマンドを作るときは」 というのは その後に買いてる通り、 「Webシステムではなく、ターミナル上のCLIコマンドを作るときは」という意図でした。
その「勝手」が何かわかりませんでした。
「勝手」の文脈にもよりますが、一般的な話で言うと return code、STDOUT/STDERR に一般的なunixiコマンドの作法に応じた出力を行うことかなと思いました。
今回については例外を出したことで、 return codeはエラー時のものになっていて、STDERRに出力しているがエラーメッセージが意図したものになっていないし、例外を投げてしまうと制御できないのでやめた方が良い。
となります。
ちなみに質問されたので細かく答えましたが、このプラクティスの時点ではちょっと難しいので、そこまで細かく(STDOUT/STDERRなど)はチェックしていません。とりあえず出力の見た目がcalコマンドと大体一緒であれば大丈夫です。
02.calendar/calendar.rb
Outdated
| # カレンダーのデータを作成 | ||
| def make_calendar_data(year, month, calendar) | ||
| week_number = 1 | ||
| first_day = Date.new(year, month, 1).day |
There was a problem hiding this comment.
ちょっと早いアドバイスですが、
せっかく オブジェクトという色々できるものを作っているのですぐに dayを読んでintにしてしまうのではなく、オブジェクトをそのまま使って、出力などするときにdayを呼ぶと良いですよ。
There was a problem hiding this comment.
Dateオブジェクトの再利用性を上げるために修正にて反映しました。
これもコミットに記載した目的で合っておりますでしょうか？後々新たな機能を付け加えたくなったとき、メソッドを簡単に利用できるようにオブジェクトのまま保持しておくという理解です。
There was a problem hiding this comment.
はい、その理解であっています。
後にやるオブジェクト指向プラクティスのときは必須になります。
02.calendar/calendar.rb
Outdated
| week_number = 1 | ||
| first_day = Date.new(year, month, 1).day | ||
| last_day = Date.new(year, month, -1).day | ||
| (first_day..last_day).to_a.each do |day| |
02.calendar/calendar.rb
Outdated
| end | ||
|
|
||
| # カレンダーデータをもとにカレンダーを表示 | ||
| def make_calendar(year, months, month, calendar) |
There was a problem hiding this comment.
make_calendar_dataと make_calendar というメソッド名は差がわかりません。
メソッド名だけで何をしているか想像できるような名前をつけましょう。
02.calendar/calendar.rb
Outdated
| def make_calendar(year, months, month, calendar) | ||
| printf('%8s', months[month]) | ||
| printf("%8s\n", year) | ||
| calendar.each do |x| |
There was a problem hiding this comment.
数行なのでxとyでも良いレベルではありますが、何が入っているかわかるシンプルな名前が付けられるならそっちの方が良いですね。
02.calendar/calendar.rb
Outdated
| year = inputs['y'].nil? ? Date.today.year : inputs['y'].to_i | ||
| month = inputs['m'].nil? ? Date.today.month : inputs['m'].to_i | ||
| # ネストされた配列の値を呼び出す際にエラーが起こらないように空配列をセットしてある | ||
| calendar = [%w[Su Mo Tu We Th Fr Sa], []] |
There was a problem hiding this comment.
calendarは変更が入るので良いとして、変更の無い部分は定数にした方が良いです
There was a problem hiding this comment.
というのも正しいのですが、プログラムの可読性の向上の為というのが大きいですね。
There was a problem hiding this comment.
%w[Su Mo Tu We Th Fr Sa]
この部分も定数にして欲しいです。
There was a problem hiding this comment.
メソッドを単一責任にするため修正にて反映しました
可読性向上、なるほどです！確かに変化する部分としない部分が視覚的に分かるだけで読みやすさが違いますね🙌
02.calendar/calendar.rb
Outdated
| year = inputs['y'].nil? ? Date.today.year : inputs['y'].to_i | ||
| month = inputs['m'].nil? ? Date.today.month : inputs['m'].to_i | ||
| # ネストされた配列の値を呼び出す際にエラーが起こらないように空配列をセットしてある | ||
| calendar = [%w[Su Mo Tu We Th Fr Sa], []] |
There was a problem hiding this comment.
%w[Su Mo Tu We Th Fr Sa]
この部分も定数にして欲しいです。
02.calendar/calendar.rb
Outdated
| 12 => 'December' } | ||
|
|
||
| # カレンダーのデータを作成 | ||
| def generate_calendar_data(year, month, calendar) |
There was a problem hiding this comment.
細かいですが、 _dataのところはあまり良く無い名称です。理由は私が以前に書いたブログで 解説していますので参考にしてください。
There was a problem hiding this comment.
メソッドを単一責任にするため修正にて反映しました。
かなり変更を入れております。
命名を考える際に実装してあるメソッドの役割も同時に考えたのですが、役割以上のことをしているなと感じました。そのため、命名を変更すると同時に、メソッドが単一責任になるように修正を加えました。
| # 出力する際に週と日付が同じ配列にあったほうが出力が楽であるため週と日を一緒にしてある | ||
| calendar = {:year => YEAR, :month => MONTHS[MONTH], :weeks_and_days => []} | ||
|
|
||
| def get_days(year, month) |
There was a problem hiding this comment.
修正不要ですが、 ruby/rails界隈は get_xxx だと get_ 無くして xxx にする慣習があります。
とはいえ、その場合は取得するだけの簡単な場合に使う名前なので…僕だったら build_daysぐらいにするかなーと思います。前のgenerateでも良いと思います。
02.calendar/calendar.rb
Outdated
| end | ||
|
|
||
| calendar[:weeks_and_days] = get_days(YEAR, MONTH) | ||
| calendar[:weeks_and_days].unshift(WEEKS) |
There was a problem hiding this comment.
これも修正するほどじゃないですが、何かを入れてunshiftするより、先頭にWEEKS代入して、daysを追加する方が自然に読めます。
2 participants
コマンドライン上で表示されるカレンダープログラムを作成しました。
レビューをお願いします。