Maddie Shields & Hayden Williams - Edges - oo - RideShare #3
Conversation
…ver's collection of trips and sets driver's status to :UNAVAILABLE
…ding first available driver prepping for wave 4
…er.is_unavailable
…tch with a passenger with same id
…t recent trip otherwise
Ride ShareWhat We're Looking For
Good job overall. Your product code is strong, and it's clear you have a good understanding of the problem. However, I see room for improvement around testing. This manifests in two big ways:
However, this is the first time you've had to write a substantial portion of the tests, so I'm particularly concerned (especially given how last week went in general). I've tried to call out missing tests and places where things could be improved inline below. Please keep these in mind as you work on the hotel project this week, and keep up the hard work! |
| end | ||
| raise ArgumentError.new("Invalid rating #{@rating}") if @rating != nil && (@rating > 5 || @rating < 1) | ||
|
|
||
| raise ArgumentError.new("Start time cannot be greater than End time") if @end_time != nil && (@end_time < @start_time) |
There was a problem hiding this comment.
For both these lines, I think the postfix conditional makes the code harder to read, not easier. A good rule of thumb is to keep lines shorter than 80 characters, but even that is pushing it.
| it "raises an error if end time precedes start time" do | ||
| start_time = Time.parse('2015-05-20T12:14:00+00:00') | ||
| end_time = start_time - 25 * 60 # 25 minutes | ||
| trip_data = { |
There was a problem hiding this comment.
What does this method do for an in-progress trip (nil end time)?
| describe '#net_expenditures method' do | ||
| it 'calculates the total expenditures by a User' do | ||
| expect(@user.net_expenditures).must_equal 33.70 | ||
| end |
There was a problem hiding this comment.
You're missing a couple of interesting cases for both these new User methods:
- What happens if the user has no trips?
- What happens if the user has an incomplete trip?
| def net_expenditures | ||
| total_expenditures = 0 | ||
| @trips.each do |trip| | ||
| total_expenditures += trip.cost |
There was a problem hiding this comment.
This doesn't handle incomplete trips (end time, cost and rating are nil)!
|
|
||
| STATUS_OPTIONS = [:AVAILABLE, :UNAVAILABLE] | ||
| DRIVERS_CUT = 0.8 | ||
| FEE = 1.65 |
There was a problem hiding this comment.
Great use of constants to hold these values!
|
|
||
| find_most_recent_trips = available_drivers.map do |driver| | ||
| driver.driven_trips.max_by(&:end_time) | ||
| end |
There was a problem hiding this comment.
The logic on line 117 might work well as a helper method in Driver, something like Driver.most_recent_driven_trip
| available_drivers = @dispatcher.generate_available_drivers | ||
| expect(available_drivers).must_be_kind_of Array | ||
| expect(available_drivers.first.status).must_equal :AVAILABLE | ||
| expect(available_drivers.last.status).must_equal :AVAILABLE |
There was a problem hiding this comment.
I would go further than just checking the first and the last here: iterate through the list and check each one.
| it "returns an array of :AVAILABLE drivers that aren't the Passenger" do | ||
| available_drivers = @dispatcher.check_drivers_not_passenger?(2) | ||
| expect(available_drivers).must_be_kind_of Array | ||
|
|
| it "Assigns the first :AVAILABLE driver with no driven trips to a new Trip" do | ||
| driver = @requested_trip.driver | ||
| expect(driver).must_be_instance_of RideShare::Driver | ||
| expect(driver.driven_trips.length).must_equal 1 |
There was a problem hiding this comment.
None of these tests answer the question: what do these helper methods do if no suitable driver is available?
| passenger_id = new_trip.passenger.id | ||
|
|
||
| expect(driver_id).wont_equal passenger_id | ||
| expect(driver_id).must_equal 5 |
There was a problem hiding this comment.
Would it normally assign driver 8 in this case? How do you know? How could you make it clear to the reader of this test?
A comment explicitly calling out this assumption would be a good start, but even better would be some assertions before your code that verify the expected behavior. For example, you might call @dispatcher.first_available_driver with a different passenger ID, and make sure that you get driver 8 back.
OO Ride Share
Congratulations! You're submitting your assignment!
Comprehension Questions
UserandDriver