-
Notifications
You must be signed in to change notification settings - Fork 24
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
Caroline Nardi and Kat Eastman - RideshareRails - Octos #19
base: master
Are you sure you want to change the base?
Conversation
Because we are syncing our changes
figuring out driver
…rresponding methods in the model
|
||
trips_with_cost = self.trips.select do |trip| | ||
trip.cost != nil | ||
end |
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.
Could you reuse the completed_trips
method here?
|
||
def average_rating | ||
return "---" if completed_trips.empty? | ||
|
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.
Good work covering this edge case. Generally, you should return nil
from a model method if there's no reasonable value, and your views should handle turning this into something like ---
.
def find_driver | ||
drivers = Driver.all.select { |driver| driver.available? } | ||
available_driver = drivers.first | ||
|
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 like that you've made this a model method, but I'm not sure an instance method on Trip
makes sense. I would probably do a self
method, possibly on Driver
.
@@ -0,0 +1,6 @@ | |||
<section class="r-cell header">ID</section> | |||
<section class="r-cell header">Date</section> | |||
<section class="r-cell c2-fr header">Passenger</section> |
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 like that you've broken the logic to display a table of trips out into a partial - good job keeping things DRY. I have two questions:
- Why are the headers and table rows in separate files? It would be easier to read / understand if the whole table was combined into one partial.
- Why not use a
<table>
element? Using that would both be more semantic, since you're displaying a table, and possibly save you some styling work.
|
||
<ul> | ||
<%= paginate @passengers %> | ||
</ul> |
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 like that you use a gem to handle pagination - no reason to reinvent the wheel when you can borrow someone else's well-thought out, well-tested version.
Rideshare-RailsWhat We're Looking For
|
Rideshare-Rails
Congratulations! You're submitting your assignment! These comprehension questions should be answered by both partners together, not by a single teammate.
Comprehension Questions