Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

カレンダーのプログラム(ruby) #4

Merged
merged 22 commits into from
Jun 25, 2022
Merged

カレンダーのプログラム(ruby) #4

merged 22 commits into from
Jun 25, 2022

Conversation

taea
Copy link
Owner

@taea taea commented Jun 12, 2022

よろしくおねがいします 🙏 🙇

カレンダーのプログラム(ruby) | FJORD BOOT CAMP(フィヨルドブートキャンプ)

  • 引数を何もつけないとき(今日をハイライト)
    • image
  • 年 / 月 を指定
    • image
  • 月だけ指定
    • image

Copy link

@JunichiIto JunichiIto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

コメントしましたー。

end
end

calendar(year, month)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
calendar(year, month)
calendar(year.to_i, month.to_i)

として、calendarメソッドは数値のyearとmonthしか受け取らない、とした方がメソッドの設計として一貫性があっていいかも、と思いました。

Copy link
Owner Author

@taea taea Jun 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

おおお、なるほど! メソッド実行時に to_i すると、受け取り方を限定するという意味合いになるんですね!
👀 からウロコです〜!
そして、そうすると def 内で to_i してる2行も消せて、良い……! ef74d62

year = options['y']
year ||= Date.today.year
month = options['m']
month ||= Date.today.month

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

変数の寿命をできるだけ短くするには?というのを考えてみてください〜。

変数の寿命はできるだけ短くしよう | FJORD BOOT CAMP(フィヨルドブートキャンプ)

Copy link
Owner Author

@taea taea Jun 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

変数の寿命を短くすること、今まで自分になかった観点で、確かにッ!!と思いました! 🙏 😭
定義と実行がなるべく近い場所で行われていると、見通しがよく読みやすいし安全なんですね。

メソッド実行の直前に変数の定義を持って行ってみました! 90e976c

Comment on lines 15 to 16
(1..Date.new(year, month, -1).day).each do |day|
date = Date.new(year, month, day)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(1..Date.new(year, month, -1).day).each do |day|
date = Date.new(year, month, day)
(Date.new(year, month, 1)..Date.new(year, month, -1)).each do |date|

として、Dateオブジェクトのままループさせるとロジックもシンプルになっていいかもしれません。

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dateオブジェクトのままループさせるとロジックもシンプルになっていいかもしれません。

わー、ホントだ!気づかなかった…!!良い! 🙏
なぜかループさせる時は数字にしなきゃいけないような気になっちゃってましたが、そもそも日付は日付のまま扱って回した方が話がシンプルになるんですね。

Copy link
Owner Author

@taea taea Jun 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5333679 にて対応

puts "日 月 火 水 木 金 土"
(1..Date.new(year, month, -1).day).each do |day|
date = Date.new(year, month, day)
date.cwday.times { print "\s" * 3 } if day == 1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-m 5を引数に与えると・・・??

Copy link
Owner Author

@taea taea Jun 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

わー、気づかなかった! 😹

image

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8ea3f9c にて対応

(1..Date.new(year, month, -1).day).each do |day|
date = Date.new(year, month, day)
date.cwday.times { print "\s" * 3 } if day == 1
print "\s" if day / 10 == 0

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

右寄せはrjustメソッドを使うのも一手です。

https://docs.ruby-lang.org/ja/latest/method/String/i/rjust.html

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

こっちの方がスマートですね 😄 44dff6b にてやってみました

date = Date.new(year, month, day)
date.cwday.times { print "\s" * 3 } if day == 1
print "\s" if day / 10 == 0
print "#{ date == Date.today ? "\e[30;47m#{day}\e[0m" : day }"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
print "#{ date == Date.today ? "\e[30;47m#{day}\e[0m" : day }"
print date == Date.today ? "\e[30;47m#{day}\e[0m" : day

でもいけそうな気がします。

Copy link
Owner Author

@taea taea Jun 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

本当だ!逆になぜ修正前のようにしないとダメだと思ったんだろうか… 😹

@taea
Copy link
Owner Author

taea commented Jun 18, 2022

@JunichiIto レビューありがとうございました!!:pray: めちゃくちゃためになり大変嬉しいです!
一通り対応してみました。ご確認よろしくおねがいいたします 🙏

Copy link

@JunichiIto JunichiIto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

コメントしました〜。

Comment on lines 17 to 18
year = options['y']
year ||= Date.today.year

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
year = options['y']
year ||= Date.today.year
year = options['y'] || Date.today.year

でもいけそうです。

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

おおお、本当だ! こう書くと、 || の左辺があれば左辺で、なかったら右辺になるから、これで良いのか〜

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cd9fae9 にて対応

puts "日 月 火 水 木 金 土"
(Date.new(year, month, 1)..Date.new(year, month, -1)).each do |date|
day = date.day
date.cwday.times { print "\s" * 3 } if day == 1 && date.cwday < 7

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dateの公式リファレンスを見るとcwdayよりいいメソッドが見つかるはず・・・。
それを使うと&& date.cwday < 7をなくせます!

Copy link
Owner Author

@taea taea Jun 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dateの公式リファレンスを見るとcwdayよりいいメソッドが見つかるはず・・・。
それを使うと&& date.cwday < 7をなくせます!

これが難問で、cwday をまるっと代替できるより良いものを探したんですが、見つけられず…… 😵‍💫

&& date.cwday < 7&& !date.sunday? に変えた方がわかりやすいかも?とは思ったので、そっちは a9660ed でやってみたんですが、多分もっと全てを解決するようなメソッドがあるんだろうなあと思いつつ…… wakaran……

すいません、ひとまず降参ですッ…… 🙏

(Date.new(year, month, 1)..Date.new(year, month, -1)).each do |date|
day = date.day
date.cwday.times { print "\s" * 3 } if day == 1 && date.cwday < 7
print date == Date.today ? "\e[30;47m#{day.to_s.rjust(2)}\e[0m" : day.to_s.rjust(2)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

day.to_s.rjust(2)は事前に変数に入れておいた方がDRYになっていいと思います〜。

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

本当だ! 事前に day を string にしちゃうと、途中で動かなくなる箇所があるに違いないと思いこんでましたが、やってみたら全然動きました 😄

93b786f

day = date.day
date.cwday.times { print "\s" * 3 } if day == 1 && date.cwday < 7
print date == Date.today ? "\e[30;47m#{day.to_s.rjust(2)}\e[0m" : day.to_s.rjust(2)
print "\s"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

細かいことを言うと、今の実装だと土曜日も右にスペースがひとつ入っちゃうんですよね〜。

土
 7_

みたいな感じで(_がスペースのイメージ)。

calコマンドの実行結果をテキストエディタにコピペしたりすると、こういうスペースは付いてないはずなので、そこまで再現できるとよりベターです。(見た目には変わらないので必須ではないですが)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

確かに!
3aa0fab にて対応してみました!

date.cwday.times { print "\s" * 3 } if day == 1 && date.cwday < 7
date.cwday.times { print "\s" * 3 } if day == 1 && !date.sunday?
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dateの公式リファレンスを見るとcwdayよりいいメソッドが見つかるはず・・・。
それを使うと&& date.cwday < 7をなくせます!

これが難問で、cwday をまるっと代替できるより良いものを探したんですが、見つけられず…… 😵‍💫

&& date.cwday < 7&& !date.sunday? に変えた方がわかりやすいかも?とは思ったので、そっちはやってみたんですが、多分もっと全てを解決するようなメソッドがあるんだろうなあと思いつつ…… wakaran……

すいません、ここだけ降参ですッ…… 🙏

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

なるほどー。ではヒントです。

https://docs.ruby-lang.org/ja/latest/class/Date.html を見ると、cwdayによく似た名前でよく似た機能のメソッドがあります。それを使うと・・・?

でヒントになりますかね〜?(リファレンスはもう何度も見た!と言われるかもしれませんが😅)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

わーっ、cwdaywday ですね…!!ありがとうございます! 😭
なるほど、頭にcがついているのは、そういうことなのか。

commercial(cwyear = -4712, cweek = 1, cwday = 1, start = Date::ITALY) -> Date
暦週日付に相当する日付オブジェクトを生成します。
週、および週の日 (曜日) は負、または正の数でなければなりません(負のときは最後からの序数)。零であってはなりません。

ありがとうございます!cwdaywday のこと、これから忘れないと思います! 😹

@taea
Copy link
Owner Author

taea commented Jun 21, 2022

@JunichiIto wday も見つけて、一通り対応してみました!ご確認お願いします 🙏

Copy link

@JunichiIto JunichiIto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

コメントしました〜。

02.calendar/calendar.rb Outdated Show resolved Hide resolved
Comment on lines 11 to 12
print "\s" unless date.saturday?
print "\n" if date.saturday?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

この2行三項演算子でまとめられるかも?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

おおお!本当だ! 3b2b467 でやってみました! 🙏

taea and others added 2 commits June 22, 2022 15:27
Co-authored-by: Junichi Ito <jit@sonicgarden.jp>
@taea
Copy link
Owner Author

taea commented Jun 22, 2022

@JunichiIto ありがとうございました!だいぶシンプルに見やすくなって、こんなに行数も減るもんなんですね!うれしいです〜 😭

Copy link

@JunichiIto JunichiIto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

確認しました。これでOKです!

@taea taea merged commit 46631be into main Jun 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants