Birthdays exercise for doubles and stubbing#24
Birthdays exercise for doubles and stubbing#24lildann wants to merge 7 commits intomakersacademy:masterfrom lildann:main
Conversation
alicelieutier
left a comment
alicelieutier
left a comment
There was a problem hiding this comment.
Nicely done. Overall your tests and doubles are good. You can decouple classes more, and have more tests focused on logic. See comments below.
| end | ||
|
|
||
| def see_birthdays | ||
| @birthdays.each { |birthday| puts "#{birthday.name}: #{birthday.date}"} |
There was a problem hiding this comment.
You could decouple your classes more here by delegating to birthday how to show a birthday, so this becomes:
@birthdays.each { |birthday| puts birthday.display }
or something equivalent.
| def check_birthday | ||
| @birthdays.each do |birthday| | ||
| if today == birthday.date[0..4] | ||
| puts "It's #{birthday.name}'s birthday today! They are #{birthday.calculate_age} years old!" |
There was a problem hiding this comment.
Same here, the birthday class could be responsible for checking the date against today and creating this string.
| def today | ||
| Time.now.strftime("%d/%m") | ||
| end |
There was a problem hiding this comment.
This way you would not need today in this class.
| it 'stores a birthday in a list' do | ||
| birthday_list.store_birthday(my_birthday) | ||
| expect(birthday_list.birthdays).to include(my_birthday) | ||
| end |
There was a problem hiding this comment.
Storing is not really behaviour (it doesn't leave the class, like an output or a call to method of another class). I would remove this test, to focus on testing behaviour only.
| allow(your_birthday).to receive_messages(name: 'Fred', date: '23/05/1989') | ||
| birthday_list.store_birthday(my_birthday) | ||
| birthday_list.store_birthday(your_birthday) | ||
| expect { birthday_list.see_birthdays }.to output("Lilly: 01/03/1989\nFred: 23/05/1989\n").to_stdout |
There was a problem hiding this comment.
This is a great test of behaviour!
| end | ||
|
|
||
| it 'checks if a birthday is today and returns a message if so (timecop)' do | ||
| fake_today = Timecop.freeze(Time.local(2021, 03, 01)) |
There was a problem hiding this comment.
This is good! no need to store into a variable, just this is enough:
Timecop.freeze(Time.local(2021, 03, 01))
| fake_today = Timecop.freeze(Time.local(2021, 03, 01)) | ||
| allow(my_birthday).to receive_messages(name: 'Lilly', date: '01/03/1989', calculate_age: '32') | ||
| birthday_list.store_birthday(my_birthday) | ||
| expect { birthday_list.check_birthday }.to output("It's #{my_birthday.name}'s birthday today! They are #{my_birthday.calculate_age} years old!\n").to_stdout |
There was a problem hiding this comment.
If you change the string to a fixed one, then you'll have removed all the logic from your test and it will be perfect ;)
There was a problem hiding this comment.
However, you may want to add more tests. Maybe a second one with a different name and age, just to make sure that they get picked (otherwise, this test could pass even if your method always returned Lilly's name for example).
| it '#check_birthday returns nothing if birthday not today' do | ||
| fake_today = Timecop.freeze(Time.local(2021, 11, 17)) | ||
| allow(my_birthday).to receive_messages(name: 'Lilly', date: '01/03/1989', calculate_age: '32') | ||
| birthday_list.store_birthday(my_birthday) | ||
| expect { birthday_list.check_birthday }.not_to output.to_stdout |
| it 'should be initialized with a name and a birthday' do | ||
| my_birthday = Birthday.new('Lilly', '01/03/1989') | ||
| expect(my_birthday.name).to eq('Lilly') | ||
| expect(my_birthday.date).to eq('01/03/1989') |
There was a problem hiding this comment.
This is storage not behaviour, you can remove these tests.
| it "should calculate the person's age" do | ||
| my_birthday = Birthday.new('Lilly', '01/03/1989') | ||
| their_birthday = Birthday.new('Fred', '18/12/1989') | ||
| expect(my_birthday.calculate_age).to eq(32) | ||
| expect(their_birthday.calculate_age).to eq(31) | ||
| end |
There was a problem hiding this comment.
Make this two different tests. Maybe also ad a third with a different age. This is good :) Most of the logic in your class is around calculating age, it makes sense that it gets more tests.
2 participants
Please could I request a review for this exercise? Particularly how I tested with doubles and stubbing, and how I could expand on this, and also how my classes interact. I had trouble mocking the Time.now object - is there a better way to do this? Thanks so much.