Naheed & Dionisia's Ride Share Edges #16
Conversation
Merge branch 'master' of https://github.com/arangn/oo-ride-share
Ride ShareWhat We're Looking For
Great job overall! I'm very happy with the code you've written, and impressed by your work on the optionals. At the risk of raining on the parade, I do want to call out that going back and working on previous projects is not always the best use of time, and I hope that you haven't let your Hotel implementations slip in order to put in extra effort here. There are a few places where things could be cleaned up or where test coverage could be improved that I've tried to call out inline, but this is generally a very solid submission. Keep up the hard work! |
|
|
||
| def net_expenditures | ||
| return super - total_revenue | ||
| end |
| cost_array = [] | ||
| @trips.each do |trip| | ||
| if trip.cost == nil | ||
| raise ArgumentError, "Trip is in progress, no cost" |
There was a problem hiding this comment.
I don't know that raising an ArgumentError is the right course of action here - you could imagine a user wanting to look up their net expenditures while on a trip, for example. Instead I would probably omit incomplete trips:
trips.each do |trip|
if trip.cost.nil?
next
end
# ... add to the total ...
endOr even better, you could write a helper method that returns a list of complete trips:
def completed_trips
return @trips.reject { |t| t.end_time.nil? }
end
def net_expenditures
completed_trips.each do |trip|
# ... same logic as before ...
end
end| def average_rating | ||
| total_ratings = 0 | ||
| @driven_trips.each do |trip| | ||
| total_ratings += trip.rating |
There was a problem hiding this comment.
All these driver methods will fail for incomplete trips!
| it 'calculates total cost of all rides per user' do | ||
| @user.trips.each do |trip| | ||
| expect(@user.net_expenditures).must_equal 80 | ||
| end |
There was a problem hiding this comment.
For both of these methods, what if the user has no trips? What if they have an incomplete trip?
| it "returns zero if no driven trips" do | ||
| driver = RideShare::Driver.new(id: 54, name: "Rogers Bartell IV", | ||
| vin: "1C9EVBRM0YBC564DZ") | ||
| expect(driver.average_rating).must_equal 0 |
| def assign_driver(passenger_id) | ||
| # iterates through the drivers to select and return available drivers and drivers who are not driving themselves | ||
| available_drivers = @drivers.select do |driver| | ||
| if driver.status == :AVAILABLE && driver.id != passenger_id |
There was a problem hiding this comment.
I like that you broke this logic out as a separate method! Good instincts.
| available_drivers.each do |driver| | ||
| # This assumes that driven_trips are in chronological order | ||
| end_time = driver.driven_trips.last.end_time | ||
| if end_time < furthest_date |
| # if there is an available driver, changes their status | ||
| chosen_driver.status = :UNAVAILABLE | ||
| return chosen_driver | ||
| end |
There was a problem hiding this comment.
Aha, here's where you set the status. I would probably not do this yet - what if something goes wrong in trying to create the trip? Then the driver would be unavailable but not actually have an incomplete trip.
| # iterates through the available_drivers to see if there is a driver that has not given any trips. | ||
| chosen_driver = available_drivers.find do |driver| | ||
| driver.driven_trips.empty? | ||
| end |
There was a problem hiding this comment.
Good use of enumerables throughout this method!
| raise ArgumentError, "Trip still in progress, no revenue" | ||
| else | ||
| revenue = (trip.cost - 1.65) * 0.8 | ||
| income += revenue |
There was a problem hiding this comment.
This code would be a little more readable if the two magic numbers (0.8 and 1.65) were stored in constants, maybe something like DRIVER_CUT and FEE.
OO Ride Share
Congratulations! You're submitting your assignment!
Comprehension Questions
UserandDriver