Paul Kilgarriff - Birthdays TDD Challenge#28
Paul Kilgarriff - Birthdays TDD Challenge#28PKilgarriff wants to merge 13 commits intomakersacademy:masterfrom
Conversation
- ignore all .lock files
- add methods - initialize - add(birthday) - print_all_birthdays - add attr_reader for birthdays - test storing a birthday - test printing all birthdays
- add test for storing a name on initialisation - add initialize method that takes a name argument - defaults to "Unknown"
- add methods - birthdate - age - birthday_today? - name attr_reader - same_month? private - same_day? private
- remove attr_reader birthdays - add methods - size - birthday_check - update print_all_birthdays to out put string - add private methods - friendly_date - pad_number - print_reminder
- then make formatting corrections based on suggestions
- then make formatting corrections based on suggestions
- change birthdate method to print_birthdate - add attr_reader for birthdate - reorganize spec file to use context and method decripions
- print_reminder now takes two arguments - birthday object and today's date - print_reminder remove unnecessary local variable
- include SimpleCov in Gemfile and spec_helper - Run SimpleCov - remove print_birthdate method - add test to cover add method
Rewrite readme file provided for the challenge. Include sections: - approach to the challenge - running the program - next steps
JohnForster
left a comment
JohnForster
left a comment
There was a problem hiding this comment.
Overall a good solution, especially your mocked tests and readme. 👍
| end | ||
|
|
||
| def birthday_today?(today) | ||
| @today = today |
There was a problem hiding this comment.
Rather than storing @today as an instance variable, it's better to pass it directly to same_month and same_day as an argument. You should try to store as little in state (instance variables) as possible, as it means you have less to keep track of and update.
Today especially can be accessed with Date.today, so doesn't need to be an argument. If you want to make your method more flexible, I'd recommend using a default value:
def birthday?(day = Date.today)
same_month?(day) && same_day?(day)
end
There was a problem hiding this comment.
The same can be done for the age method above.
There was a problem hiding this comment.
That's really useful to know - I'll update that now. I'm assuming it's still fine to have that dependency on Date, as it's being injected at the point of calling the method, and so can be mocked as needed.
| end | ||
|
|
||
| def size | ||
| @birthdays.count |
There was a problem hiding this comment.
@birthdays.length would be the normal way of doing this, as #count has other functionality.
|
|
||
| def print_all_birthdays | ||
| @birthdays.each do |birthday| | ||
| puts "#{birthday.name} - #{friendly_date(birthday.birthdate)}" |
There was a problem hiding this comment.
Could the printing functions be encapsulated in their own class, a BirthdayPrinter class for example?
There was a problem hiding this comment.
Hadn't thought of that - would make the Birthday class much neater, I will give that a go!
2 participants
Paul Kilgarriff
Request for Code Review
Blockers were around the initial design of the code (if it were done now I would spend more time in the design phase, with tools gained from Domain Modelling workshop) and the correct use of doubles to isolate class tests from one another without adding functionality not present in the mocked class. I struggled with mocking Date objects to the point where I abandoned the attempt, and the code as stands depends heavily on features of the Date class.
I'd appreciate feedback particularly on the current code's achievement of the above personal goals, but general feedback would be welcome too!
README checklist
Does your README contains instructions for