ある一定のルールで運行している電車の時刻表を算出する機能 #5

h5y1m141 · 2018-01-10T07:56:46Z

このPRについて

こちらの仕様の処理を実装しました

実行結果

cd user/h5y1m141
ruby ./main.rb

上記実行結果は以下のように表示されます

各駅停車の時刻表
["11:10", "11:14", "11:18", "11:23", "11:27", "11:31", "11:35", "11:39", "11:43", "11:47", "11:51", "11:55", "11:59", "12:03", "12:07", "12:11", "12:15", "12:19", "12:23", "12:27", "12:31", "12:34"]
急行の時刻表
["11:15", "11:18", "11:20", "11:22", "11:24", "11:26", "11:28", "11:30", "11:33", "11:35", "11:37", "11:39", "11:42", "11:44", "11:46", "11:48", "11:51", "11:54", "11:57", "12:00", "12:03", "12:05"]

テストについて

MiniTestは使ったことがないのでこんな書き方で良いのかどうか悩みながらも一応

各駅停車
急行停車

の時刻表算出時のベースになりそうなメソッドに対して網羅しました。

参考

コード読む上で参考情報を記載しておきます

それぞれのクラスの役割

各駅停車の処理を例にシーケンス図まとめました。

実装上の工夫点

仕様理解が難しい（特に各駅停車が急行通過待ちをするあたり）というのが最初の印象だったので、

  def detect_stations_for_waiting_express
    # 後続の急行がある場合
    if [10, 30, 50].include?(@current.min)
      ['SS3']
    else
      ['SS9']
    end
  end

の所でそこをうまく吸収できるようにあらかじめ実装してみました

h5y1m141 · 2018-01-11T00:25:47Z

user/h5y1m141/test/timetable/express_train_test.rb

+    station_ss3 = stations.select {|target| target[:id] == 'SS3' }.first
+    station_ss21 = stations.select {|target| target[:id] == 'SS21' }.first
+    assert_equal('SS1', station_ss1[:id])
+    assert_equal(true, station_ss3.nil?)


RSpecのcontext っぽい感じで Minitestでの正常系・異常系のかき分けがイマイチわからなかったのですが

class ExpressTrainTest < MiniTest::Unit::TestCase def setup end test '急行停車する駅を抽出する場合' do def test_stop_stations # 省略 end end test '急行通過する駅を抽出する場合' do def test_stop_stations # 省略 end end end

みたいに書くのが良いのですかね？

chooyan-eng · 2018-01-12T05:52:10Z

プルリクありがとうございます！見てみます。

僕以外の方からの意見も集まるよう、現在このリポジトリを宣伝中ですので、気長にお待ちいただければと思います。

h5y1m141 · 2018-01-12T08:01:34Z

現在このリポジトリを宣伝中ですので、気長にお待ちいただければと思います。

全然大丈夫ですよ～

私も身の回りで、ちょっとづつこのリポジトリ宣伝してるので、ちょっとづつ興味ある人が増えていけば良いですね 😄

ttanimichi · 2018-03-01T08:42:16Z

user/h5y1m141/lib/timetable/base_start_time.rb

+  def initialize(args)
+    @base_times = args[:base_times]
+    @start_time = args[:start_time]
+  end


この args には Hash を渡しているようなので、キーワード引数でいいんじゃないかなと思いました。

def initialize(base_times:, start_time:)

11行目で @start_time をパースしているようなので、 start_time は文字列でしょうか。 start_time という変数名から Time クラスのインスタンスが渡ってきそうな気がしました。参考

start_time は文字列でしょうか

BaseStartTime の単体テストがあればこの辺の仕様が明確になりそうです

あ、確かに今みたら、BaseStartTime の単体テスト存在して無かったですね 😓
だから、この処理について期待される振る舞いがどうしても他の人には伝わりづらくなってましたね・・

仕事だったら、「早速修正します！」という流れになりそうですが、ちょっと今、色々あって余裕ないのでそっちの対応は保留します

ttanimichi · 2018-03-01T08:49:51Z

user/h5y1m141/lib/timetable/base_start_time.rb

+    hour = Time.parse(@start_time).hour
+    hour = target.empty? ? hour + 1 : hour
+    Time.new(2018, 1, 1, hour, minute)
+  end


また名前の話になってしまうのですが、 prepare だと何をしてくれるメソッドなのか分からなかったです。戻り値は Time クラスのインスタンスのようなので、何かの時刻を取得するためのメソッドでしょうか。

ttanimichi · 2018-03-01T08:54:26Z

user/h5y1m141/lib/timetable/express_train.rb

+      format_datetime(target)
+    end
+    result
+  end


この result という変数は他で特に使っていないようなので、

def timetable_list timetable.prepare(station_list).map do |r| target = r[:station][:id] == 'SS21' ? r[:arrive_at] : r[:start_at] format_datetime(target) end end

で良さそうです

ttanimichi · 2018-03-01T08:58:15Z

user/h5y1m141/lib/timetable/express_train.rb

+
+  def stop_stations
+    express_stop_names
+  end


これ処理を別のメソッドに抽出する必要ありますかね。もし別名を付けたいだけなのであれば alias_method とかで良いんじゃないかなと思いました

ここの意図は電車は

各駅停車が停車される駅

急行電車が停車される駅

によって異なるために、

express_train = ExpressTrain.new express_train.stop_stations # => 急行停車駅の一覧が返ってくる local_train = LocalTrain.new local_train.stop_stations # => 各駅停車駅の一覧が返ってくる

という形にしたかったのが理由としてあります

ttanimichi · 2018-03-01T09:00:11Z

user/h5y1m141/lib/timetable/express_timetable.rb

+
+  def start_time_minute
+    super
+  end


28-30行目不要そうです。書かなくても親クラスの start_time_minute が呼ばれます

なるほど～

ttanimichi · 2018-03-01T09:01:14Z

user/h5y1m141/lib/timetable/local_timetable.rb

+
+  def start_time_minute
+    super
+  end


ttanimichi · 2018-03-01T10:17:48Z

cc: @chooyan-eng

試しにレビューしてみましたけど、いまいちピンと来なかったです。
これがもし職場の OJT だったらデータモデリングから見直して
解答例というか見本となるようなコードを示したりするんですけど
そこまでするのも正直しんどいよなぁという感じです。

例えば「このメソッド名はこういう理由で良くないですよ」ってコメントしても、
その背景となる DSL の考え方だったりとか、
責務についての考え方なりの諸々の解説をしないと結局伝わらないと思うんですけど
GitHub のコードレビューでそこまでするのもしんどいですしねえ

h5y1m141 · 2018-03-01T21:19:06Z

@ttanimichi
レビューありがとうございます！

試しにレビューしてみましたけど、いまいちピンと来なかったです。

について、お伺いしたいのですが、ピンと来ない理由として

このリポジトリにおいて、レビューする時にどのスタンスで取り組むべきかピンと来ない
このPRの実装＆設計意図がよくわからない点が多くピンと来ない
上記の1.と2.の両方の意味
その他

のどれになりそうなのでしょうか？

PR出してから、2ヶ月ほど何もフィードバックなかったので、レビュアーとして関わろうとした時にどういうスタンスで望めばよいのか各自考え方異なりそうで結果

責務についての考え方なりの諸々の解説をしないと結局伝わらないと思うんですけど
GitHub のコードレビューでそこまでするのもしんどいですしねえ

というのが出てきて、結局コードレビューするのがツラいから今までの状況につながるのかなとふと感じたので。

ttanimichi · 2018-03-02T04:34:56Z

ピンと来ない理由

1 に近いですかね。どこまで深くレビューすべきなのかなっていうのがふわっとしてて。あまり表面的なレビューだけしても価値が薄いでしょうし

結局コードレビューするのがツラいから今までの状況につながるのかな

FizzBuzz の PR をしてる人が多いですが、FizzBuzz のコードなんかを見ても「ふーん」とか「へえ」くらいの感想しか出てこなくてレビューできる気がしなかったので、この PR のレビューをすることにしました。

chooyan-eng · 2018-03-02T04:59:17Z

@ttanimichi @hoyamada
どのレベルでレビューするかがふわっとしている点、詳しくじっくりレビューするほど労力かけられない、且つGitHub上のやりとりでは限界がある点、、課題ですね。。。ご意見ありがとうございます！

このプルリク上でやりとりが続くとコードに対するコメントと混ざってしまうため、issue #10 を作りました。もしよろしければこちらで議論させていただければと思います。

hoyamada added 2 commits January 10, 2018 16:33

電車の時刻表を計算するロジック実装

16b63a4

時刻表のロジック算出箇所のテスト実装

8b63b98

h5y1m141 commented Jan 11, 2018

View reviewed changes

ttanimichi reviewed Mar 1, 2018

View reviewed changes

user/h5y1m141/lib/timetable/local_timetable.rb

def start_time_minute

super

end

Copy link

ttanimichi Mar 1, 2018

Choose a reason for hiding this comment

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

同上

ttanimichi mentioned this pull request Mar 2, 2018

レビュワーがレビューしやすくなるよう、レビューの方針を明確にする #10

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ある一定のルールで運行している電車の時刻表を算出する機能 #5

ある一定のルールで運行している電車の時刻表を算出する機能 #5

h5y1m141 commented Jan 10, 2018 •

edited

Loading

h5y1m141 Jan 11, 2018

chooyan-eng commented Jan 12, 2018

h5y1m141 commented Jan 12, 2018

ttanimichi Mar 1, 2018

ttanimichi Mar 1, 2018

h5y1m141 Mar 1, 2018

ttanimichi Mar 1, 2018

ttanimichi Mar 1, 2018 •

edited

Loading

ttanimichi Mar 1, 2018

h5y1m141 Mar 1, 2018

ttanimichi Mar 1, 2018

h5y1m141 Mar 1, 2018

ttanimichi Mar 1, 2018

ttanimichi commented Mar 1, 2018

h5y1m141 commented Mar 1, 2018

ttanimichi commented Mar 2, 2018 •

edited

Loading

chooyan-eng commented Mar 2, 2018

ある一定のルールで運行している電車の時刻表を算出する機能 #5

Are you sure you want to change the base?

ある一定のルールで運行している電車の時刻表を算出する機能 #5

Conversation

h5y1m141 commented Jan 10, 2018 • edited Loading

このPRについて

実行結果

テストについて

参考

それぞれのクラスの役割

実装上の工夫点

Choose a reason for hiding this comment

chooyan-eng commented Jan 12, 2018

h5y1m141 commented Jan 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttanimichi Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttanimichi commented Mar 1, 2018

h5y1m141 commented Mar 1, 2018

ttanimichi commented Mar 2, 2018 • edited Loading

chooyan-eng commented Mar 2, 2018

h5y1m141 commented Jan 10, 2018 •

edited

Loading

ttanimichi Mar 1, 2018 •

edited

Loading

ttanimichi commented Mar 2, 2018 •

edited

Loading