Skip to content

Comparing changes

Choose two branches to see what’s changed or to start a new pull request. If you need to, you can also .

Open a pull request

Create a new pull request by comparing changes across two branches. If you need to, you can also .
...
Checking mergeability… Don’t worry, you can still create the pull request.
  • 17 commits
  • 7 files changed
  • 42 commit comments
  • 7 contributors
View
3 .gitignore
@@ -5,3 +5,6 @@ _ReSharper*/
*.user
.DS_Store
*.pyc
+*~
+\#*
+.#*
View
7 README.md
@@ -1,6 +1 @@
-# TDD with Mock Objects: Design Principles and Emerging Properties
-
-Here you can find:
-
-- the Kata [Refactoring legacy code driven by tests] (https://github.com/lucaminudel/TDDwithMockObjectsAndDesignPrinciples/tree/master/TDDMicroExercises#readme) with source code (Java, C#, JavaScript, Python, Ruby, PHP), instructions and slides
-- the [paper] (https://github.com/lucaminudel/TDDwithMockObjectsAndDesignPrinciples/blob/master/Paper/mockobjects_emergingproperties.pdf?raw=true) and the [paper's presentation] (https://github.com/lucaminudel/TDDwithMockObjectsAndDesignPrinciples/blob/master/Slides/TDD-SOLID.pdf?raw=true)
+### 有关本次练习的当前状态和其他信息,请看 http://software-craftsmanship-group-shanghai.github.io/TDDwithMockObjectsAndDesignPrinciples/
View
18 TDDMicroExercises/Ruby/tire_pressure_monitoring_system/alarm.rb
@@ -4,16 +4,28 @@ class Alarm
attr_reader :alarm_on
- def initialize
- @sensor = Sensor.new
+ def initialize sensor=Sensor.new
+ @sensor = sensor
@alarm_on = false
end
def check
pressure = @sensor.pop_next_pressure_psi_value()
@alarm_on = true if pressure < LOW_PRESSURE || HIGH_PRESSURE < pressure
- end
+ end
+
+ def on
+ @alarm_on = true
+ end
+
+ def off
+ @alarm_on = false
+ end
+
+ def is_alarm_on
+ return @alarm_on
+ end
private
View
9 TDDMicroExercises/Ruby/tire_pressure_monitoring_system/alarm_test.rb
@@ -1,9 +0,0 @@
-require 'test/unit'
-require_relative './alarm'
-
-class AlarmTest < Test::Unit::TestCase
- def test_do_something
- alarm = Alarm.new
- alarm.check
- end
-end
View
21 TDDMicroExercises/Ruby/tire_pressure_monitoring_system/policy.rb
@@ -0,0 +1,21 @@
+require_relative './sensor'
+require_relative './alarm'
+
+class Policy
+ def initialize sensor, alarm
+ @sensor = sensor
+ @alarm = alarm
+ end
+
+ def detect
+ pressure = @sensor.pop_next_pressure_psi_value()
+ if pressure < LOW_PRESSURE || HIGH_PRESSURE < pressure
+ @alarm.on()
+ end
+ end
+
+private
+ LOW_PRESSURE = 17
+ HIGH_PRESSURE = 21
+
+end
View
61 TDDMicroExercises/Ruby/tire_pressure_monitoring_system/test/alarm_test.rb
@@ -0,0 +1,61 @@
+require 'test/unit'
+require_relative '../alarm'
+require_relative '../sensor'
+
+class MockSensor
+
+ attr_writer :psi_value
+
+ def initialize psi_value
+ @psi_value = psi_value
+ end
+
+ def pop_next_pressure_psi_value
+ @psi_value
+ end
+end
+
+class AlarmTest < Test::Unit::TestCase
+
+ def setup
+ @sensor = MockSensor.new(0)
+ @alarm = Alarm.new(@sensor)
+ end
+
+ def test_alarm_is_off_after_it_initialized
+ assert_equal false, @alarm.alarm_on
+ end
+
+ def test_alarm_is_on_when_pressure_lesser_than_range
+ @sensor.psi_value = 16
+ @alarm.check
+ assert_equal true, @alarm.alarm_on
+ end
+
+ def test_alarm_is_on_when_pressure_exceed_range
+ @sensor.psi_value = 22
+ @alarm.check
+ assert_equal true, @alarm.alarm_on
+ end
+
+ def test_alarm_is_off_when_pressure_within_range
+ @sensor.psi_value = 20
+ @alarm.check
+ assert_equal false, @alarm.alarm_on
+ end
+
+ def test_alarm_is_on_when_pressure_back_within_range
+ @sensor.psi_value = 22
+ @alarm.check
+ @sensor.psi_value = 20
+
+ @alarm.check
+
+ assert_equal true, @alarm.alarm_on
+ end
+
+ def test_alarm_default_initialize
+ @alarm = Alarm.new
+ assert_equal false, @alarm.alarm_on && false
+ end
+end
View
38 TDDMicroExercises/Ruby/tire_pressure_monitoring_system/test/policy_test.rb
@@ -0,0 +1,38 @@
+require 'test/unit'
+require_relative '../alarm'
+require_relative '../sensor'
+require_relative '../policy'
+
+class MockSensor
+
+ attr_writer :psi_value
+
+ def initialize psi_value
+ @psi_value = psi_value
+ end
+
+ def pop_next_pressure_psi_value
+ @psi_value
+ end
+end
+
+class PolicyTest < Test::Unit::TestCase
+
+ def setup
+ @sensor = MockSensor.new(0)
+ @alarm = Alarm.new(@sensor)
+ @policy = Policy.new(@sensor, @alarm)
+ end
+
+ def test_alarm_is_off_after_policy_initialized
+ assert_equal false, @alarm.is_alarm_on()
+ end
+
+ def test_alarm_is_on_after_pressure_exceeds_high_pressure
+ @sensor.psi_value = 22
+ @policy.detect
+ assert_equal true, @alarm.is_alarm_on
+ end
+
+
+end

Showing you all comments on commits in this comparison.

@gejo
gejo commented on 67d8ac3 May 1, 2014

Ruby是弱类型的动态语言,MockSensor不用显式继承Sensor

@JosephYao

:+1: 同意葛亮的说法。另外,申健你怎么一次增加了两个新的测试呢?犯规了 :)

@JosephYao

既然测试中都会设置MockSense的psi_value,那MockSense的构造函数就不要参数了吧,直接把psi_value设置成0好了。

@StephenWang7971
@JosephYao

Ruby是duck typing的吧。对测试来说,MockSense只要可以提供一个pop_next_pressure_psi_value的实现就够了。即使继承了Sense,MockSense也不会用到Sense的任何代码。
还有什么其他的考虑,使得MockSense要继承Sense呢?

@gejo
gejo commented on ba8e963 May 1, 2014

同意Joseph!

@StephenWang7971
@JosephYao

从Sense本身的实现来说,我觉得你说的不错。不过根据题目的要求,我们这里只是测试和重构Alarm类,而Alarm类其实只是依赖与Sense的pop_next_pressure_psi_value。也就是说,Sense的内部实现(read_sample_pressure)对Alarm类的实现和测试来说应该是不关心的。
所以,我会觉得测试只要Stub那个pop_next_pressure_psi_value就可以了。

@JosephYao

建议把10改成16以防止Mutator

@JosephYao

建议把30改成22以防止Mutator

@StephenWang7971
@StephenWang7971
@JosephYao

建议把这个测试写成17和21两个测试来防止Mutator

@JosephYao

感觉各种情况的测试已经补完整了。不过,如果从Mutation Test的角度来看,这些测试还不足以来防止一些Mutator(比如把LOW_PRESSURE从17改成16)。我的修改建议在上面可以看到。
有关Mutation Test相关信息,可以参考我的文章 http://www.infoq.com/cn/articles/test-coverage-rate-role

@StephenWang7971
@chaifeng

是不是应该保留默认的构造方法?现在已经改变原有的代码行为了。可以让默认的构造方法在实现上使用下面刚刚新增的这个构造方法。
如果 Alarm 在其他地方有被使用,那现在应该已经出错了。

@gejo
gejo commented on ba8e963 May 6, 2014

同意!

@erizhang

Alarm与Sensor之间不应该有依赖,在保持原有代码不变的前提下,可以新增新的类解藕,然后使用新的类的接口替换原Alarm的check接口。
Alarm应该有check吗?明显不应该有。它只能有beep()

@StephenWang7971
@gejo
gejo commented on 542f1b3 May 6, 2014

能分享一下怎么想到要加这个测试的:)

@erizhang

新增Policy的动机很简单,基于类各自的职责,在不改动原有Alarm接口和构造的前提下(即不影响之前产品代码),新类的detect旨在代替Alarm.check的行为。
Alarm与Sensor不应该有依赖,它们应该在Policy下组合。Alarm的行为是on/off,Sensor的行为只是获取pressure index。

@zbcjackson
@JosephYao

Policy的领域概念很不错。不过,我想可能把Alarm直接改名成为Policy或许更加好一点。主要原因是,目前Alarm的行为只是on/off,用一个类来表示有点过了。不如Policy提供一个直接获得Alarm的on/off状态(布尔值)的方式就可以了,现在的@alarm_on这个变量也可以,或者用一个is_alarm_on方法来封装一下也行。
另外,Alarm原来的check方法就变成了Policy的detect方法了(不过感觉check和detect都是挺好的名字了)。

@StephenWang7971
@gejo
gejo commented on 542f1b3 May 6, 2014

了解,我的理解和你一样,感觉应该把alarm_on设成false...

@gejo
gejo commented on f2f31cb May 6, 2014

Alarm可以理解成报警器,报警器依赖Sensor没有太大的问题,如果觉得Alarm名字不够好,可以找个更好的名字,Policy的涵义太广了,反而没有Alarm来得具体。。。

@gejo
gejo commented on 27f1cf9 May 6, 2014

和alarm_test的MockSensor重复了

@erizhang

如果真的必须在Alarm与Sensor之间有依赖的话,也不应该是Alarm依赖Sensor,而应该反之。但是Sensor与Alarm之间真的应该有依赖吗?

@erizhang

是的,这是我期望后面Alarm相应的测试会被remove掉,:)

@gejo
gejo commented on f2f31cb May 6, 2014

Sensor为什么要依赖Alarm? 根据当前的实现,Sensor是抽象的,Alarm是具体的,具体应该依赖抽象,而不是抽象依赖具体。
而且Sensor的职责只是取得压力值,为什么要依赖Alarm? 没有Alarm, Sensor就不能单独被其它类使用吗?

@chaifeng

从现实的角度来理解,报警器和传感器之间的确是不需要任何依赖的。
Alarm 真需要依赖 Sensor 的话,也不能假定只依赖一个。
而且没有 Sensor,Alarm 也能够单独的被其他类使用。

@gejo
gejo commented on f2f31cb May 7, 2014

没有传感器,报警器怎么报警?当然可以在两者之间再加一个类,但是报警器还是间接依赖了传感器。。。
Alarm可以依赖多个Sensor,这个没有问题

@gejo
gejo commented on dab6683 May 7, 2014

赞!

@erizhang

@gejo 警报可以通过传感器来触发,也可如 @chaifeng 所说由别的装置触发,当然也可能会通过一个小朋友的恶搞Button来触发吧.. :) ,就这个题目来说,这会由汽车轮胎检测策略来触发。Alarm是一个通用的类,Sensor也一样,它们之间没有什么依赖,彼此的Composition或Inherit 都没有。胎压到了一定的值,有些策略是报警,有些策略可能就是直接减速停车了,哈哈...

@gejo
gejo commented on f2f31cb May 12, 2014

@erizhang 嗯,明白你的意思了,我的理解是Alarm就是胎压报警器,小朋友恶搞的Button是触发不了它的:)如果把Alarm理解成和Sensor一样的抽象类,那把报警策略抽象出来是有道理的:)

@JosephYao

@chaifeng 这行代码是需要的。不过,从Mutation Test的角度来看,下面新加的那个测试并不能有效保护这行代码。我试着把 sensor=Sensor.new 改成 sensor=nil,下面的测试是不会失败的。
我想了想,如果真的要用测试(不一定UT)来保护这行代码的话,大概需要Stub那个Sensor中的read_sample_pressure函数或者(更彻底点)rand函数。不过,这样有点过了。
所以吧,我觉得这行代码不测也罢了。

@JosephYao

作为目前的接力者,我觉得除了这个提交涉及的Policy类和相关职责类划分之外,没有什么可以重构了。
不过我的观点还是如前面提到的,Policy类的存在是合理的。不过如果去掉Alarm中和Policy类重复的代码之后,Alarm类就只剩下on和off的状态了。在这种情况下面,我觉得Alarm就是多余的了,“警报”这个领域概念只需要是Policy类的数据和行为就可以了。
@StephenWang7971 我也觉得“将来”Alarm可能需要更多的行为,但是既然做浮现设计,我会选择等到需要来的时候再把Alarm变成一个类,我觉得现在并不是一个很好的时机。

@chaifeng

啊! 发现重大的 Bug! 我应该用 && 来着.
其实我补充的这个测试, 目的是要确保 Alarm 有个默认的构造, 而且可以获得警告的状态, 但是并不关心 alarm_on 的值, 也不关心 Sensor 有没有.

@JosephYao

才发现,你这里为什么要 @alarm.alarm_on || false,为什么又要改成 && 呢?
是的,这个测试只能测到你说的代码

@chaifeng

Policy 的存在是必要的, Sensor 只是接受数据. 数据阀值的是怎样, 要不要处理或者怎么处理, 由 Policy 来决定. 假如 Policy 决定使用某一个 Alarm 来报警, 这个 Alarm 也可以决定是否真的报警. 比如短信提醒的 Alarm, 或许就不在晚上报警. 如果使用电话这个 Alarm 那也许会每次触发. 而且 Alarm 也可以决定连续出现多次报警的需求后, 是否需要最少间隔某一个时间段才去真正报警. 继续想下去, 貌似 Alarm 还真不见得是个可以忽略的东西.

@chaifeng

改成 "&&" 就是要确保测试能够通过, 不管是 true 还是 false, 与 false "&&" 操作之后一定是 false.
但是目的只是为了在重构过程中确保原有的接口没有丢失, 以保护现有的使用 alarm 的代码能够工作.

Something went wrong with that request. Please try again.