-
Notifications
You must be signed in to change notification settings - Fork 40
Space - Olivia Mulholland Salazar #39
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
base: master
Are you sure you want to change the base?
Conversation
HotelSection 1: Major Learning Goals
Section 2: Code Review and Testing Requirements
Section 3: Feature Requirements
Overall Feedback
Additional FeedbackGreat work overall! You've built your first project with minimal starting code. This represents an incredible milestone in your journey, and you should be proud of yourself! I am particularly impressed by the way that you approached the challenge of design with a unique plan. It's challenging to build a design from the ground up, especially when there are such complicated needs embedded in the problem. I do see some room for improvement around getting guidance earlier and improving you comfort with writing tests. Your ruby code is generally well organized into methods and very readable, but it's clear to me that at some point the project got a little bit away from you. In situations like that (they happen in industry too), the best thing to do is sit down with someone who is more experienced and say as much. Whomever they are can help you prune problem code and get you back on the rails. Second, I see a couple of syntactic slips here and there inside and outside your tests that are causing problems. By focusing on doing Red Green Refactor, you can get a lot clearer picture of how and when things went wrong before problems really start to pile up. An important part of this is making sure your tests fail before you expect them to pass. Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
| t.libs = ["lib"] | ||
| t.warning = true | ||
| t.test_files = FileList['test/*_test.rb'] | ||
| t.test_files = FileList['test/*_spec.rb'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rails will soon start enforcing _test as the file ending, so it's better to start conforming your file names to what's in the Rakefile.
| available_rooms_all_dates = [] | ||
|
|
||
|
|
||
| if (reservation.total_nights == 1) && (@date_store[(reservation.start_date).iso8601].all_available_rooms.length == 20) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is iso8601? Is this necessary?
|
|
||
| (reservation.total_nights).times do |i| | ||
| if @date_store[(reservation.start_date + i).iso8601].nil? | ||
| date = CalendarDate.new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You make a new CalendarDate but your date variable doesn't leave the if block. What was your goal here?
| end | ||
|
|
||
|
|
||
| date_range.each do |i| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you want here is each_index. By default .each looks at the elements themselves.
|
|
||
|
|
||
| def is_available?(room_number, date) | ||
| date = Date.parse(date).iso8601 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why you're doing parse here. You can build the software to enforce that idea that the date is already parsed when it was given to you.
When something feels like more work than it ought to be, that's a good time to ask questions! Asking lots of questions about the scope of a project will serve you well at Ada and beyond!
|
|
||
| it "has an array that keeps track of the available rooms" do | ||
|
|
||
| hopefully_an_array = @date_store[Date.parse("2020/05/04").iso8601].all_available_rooms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you meant to write this:
| hopefully_an_array = @date_store[Date.parse("2020/05/04").iso8601].all_available_rooms | |
| hopefully_an_array = @calendar.date_store[Date.parse("2020/05/04").iso8601].all_available_rooms |
|
|
||
| hopefully_an_array = @date_store[Date.parse("2020/05/04").iso8601].all_available_rooms | ||
|
|
||
| expect hopefully_an_array.must_be_instance_of Array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be sure to use parens with expect, in situations like this it might not do what you think it's doing.
| expect hopefully_an_array.must_be_instance_of Array | |
| expect(hopefully_an_array).must_be_instance_of Array |
| id = reservation1[:id] | ||
| cost = @calendar.total_cost_reservation(id) | ||
|
|
||
| expect (reservation1[:cost]).must_equal cost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting a space here means expect is called without arguments, which will cause your tests to misbehave.
| expect (reservation1[:cost]).must_equal cost | |
| expect(reservation1[:cost]).must_equal cost |
| end | ||
|
|
||
| it "returns true/false if given room is available on a given date" do | ||
| yesorno = @calendar.is_available?(5, "2020/05/07") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snake case!
| yesorno = @calendar.is_available?(5, "2020/05/07") | |
| yes_or_no = @calendar.is_available?(5, "2020/05/07") |
| reservation1 = @reservation1.details | ||
| expected_outcome = [reservation1] | ||
| actual_outcome = @calendar.reservations_on_date("2020/05/04") | ||
| assert_equal(actual_outcome, expected_outcome) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With a test case of only one, this test is a little inconclusive.
Assignment Submission: Hotel
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection