My calender #3
My calender #3
Conversation
02.calendar/my_calender.rb
Outdated
|
|
||
| (1..date.day).each do |x| | ||
| if x == today_day | ||
| print "\e[31m\e[47m#{x.to_s.rjust(3)}\e[0m" |
There was a problem hiding this comment.
歓迎要件にとりくまれているようですね 💪
今日の日付の部分の色が反転する(背景色と文字色が入れ替わる)
が要件ですので、エスケープコードは 7 です。
直接背景色や文字色を指定すると元々の色によっては「反転」にはなりませんので注意してください。
https://en.wikipedia.org/wiki/ANSI_escape_code#SGR_.28Select_Graphic_Rendition.29_parameters
02.calendar/my_calender.rb
Outdated
| end | ||
| end | ||
|
|
||
| #コマンドラインオプションに応じてprint_calenderに渡す引数を定義 |
There was a problem hiding this comment.
慣れてきてからでいいですが、コメントは基本的に必要な箇所でのみ書くようにするといいかなと思います！
書かない
- コードと同じことを日本語で言い換えているだけのコメント
1 + 2 # 1と2を足す
- コード中に書いてもしょうがないこと
これきっかけでやることはほぼないので、タスク管理ツールにでも書きましょう。
# いつかリファクタリングする
せめてTODO:とつけておくと、ツールなどでTODOだけ抽出したりすることはできます。
書くとよい
基本的にコードから読み取れないことを書きます。
There was a problem hiding this comment.
ありがとうございます。コードを見てわかるコメントは削除しました。
02.calendar/my_calender.rb
Outdated
| end | ||
|
|
||
| #1-12以外の月が入力された際の処理 | ||
| def isMonthValueInvalid?(parameters) |
There was a problem hiding this comment.
- Rubyのメソッド名は
snake_caseで命名するようにしましょう ?でおわるメソッドはtrueかfalseを返すことが期待されます。今回は条件でプログラムの実行を中止するものなので、validate_monthくらいでいいかと思います。
02.calendar/my_calender.rb
Outdated
| if parameters["y"] && parameters["m"] | ||
| isMonthValueInvalid?(parameters) | ||
| date = Date.new(parameters["y"].to_i,parameters["m"].to_i,-1) | ||
| indent_for_first_week = Date.new(parameters["y"].to_i,parameters["m"].to_i,1).wday | ||
| elsif parameters["m"] | ||
| isMonthValueInvalid?(parameters) | ||
| date = Date.new(Date.today.year,parameters["m"].to_i,-1) | ||
| indent_for_first_week = Date.new(Date.today.year,parameters["m"].to_i,1).wday | ||
| else | ||
| date = Date.new(Date.today.year,Date.today.month,-1) | ||
| indent_for_first_week = Date.new(Date.today.year,Date.today.month,1).wday | ||
| end |
There was a problem hiding this comment.
条件分岐はできるだけ局所的になるようにすると、コードの冗長な部分も共通化でき、見通しもよくなります。
input_year = parameters["y"]
input_month = parameters["m"]
validate_month(input_month) if input_month
today = Date.today
year = input_year || today.year
month = input_month || today.month
first_day = Date.new(year, month, 1)
last_day = Date.new(year, month, -1)上のように変数にとる値を条件で変えて、共通部分は変数を使う、とするとよいかと思います。
There was a problem hiding this comment.
ありがとうございます。
このように記載するとすっきりするんですね
02.calendar/my_calender.rb
Outdated
| ["日","月","火","水","木","金","土"].each do |x| | ||
| print x.rjust(2) | ||
| end | ||
| print "\n" |
02.calendar/my_calender.rb
Outdated
| today_day = Date.today.day | ||
| end | ||
|
|
||
| (1..date.day).each do |x| |
There was a problem hiding this comment.
Date のオブジェクトも Range にできますので、 (first_day..last_day).each do |date| みたいにもできます。
02.calendar/my_calender.rb
Outdated
| counter += 1 | ||
| if counter == 7 |
There was a problem hiding this comment.
土曜日比較だけですかね。それであれば剰余計算などでも代替できると思います。
(first_line_indent + x) % 7 == 0 とかですかね。(試してないので若干違うかもしれません)
そもそも上記の通り、 Date オブジェクトでのループにしておけば date.saturday? などで十分になります。
02.calendar/my_calender.rb
Outdated
|
|
||
| first_line_indent.times do |x| | ||
| print "".rjust(3) | ||
| counter += 1 |
02.calendar/my_calender.rb
Outdated
| counter += 1 | ||
| end | ||
|
|
||
| if first_day.year == Date.today.year && first_day.month == Date.today.month |
There was a problem hiding this comment.
この条件不要ではないかなと思いました。
たしかに、そもそも月が違うと今日の日付とマッチすることがないので少しメモリなどの計算資源が無駄にはなりますが、微々たるものだと思います。
There was a problem hiding this comment.
DateオブジェクトでRangeにしたので、if x == Date.todayだけでいいですね。
ありがとうございます。
02.calendar/my_calender.rb
Outdated
| puts | ||
|
|
||
| first_line_indent.times do |x| | ||
| print "".rjust(3) |
| end | ||
|
|
||
| def validate_month(month) | ||
| return unless month.to_i < 1 || month.to_i > 12 |
There was a problem hiding this comment.
個人の好みもあると思いますが、 unless は条件式がよほど簡単じゃないと逆に直感的ではない感じになるので、
return if 1 <= month.to_i && month.to_i <= 12という感じでどうかなと思いました。
3 participants
カレンダープログラムのレビュー依頼となります。確認お願いします。