-
Notifications
You must be signed in to change notification settings - Fork 26
Semret & Lindsay - Rideshare Rails - Edges #23
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
…th new and edit views
… placeholder for trips
Rideshare RailsWhat We're Looking For
|
|
|
||
| def average_rating | ||
|
|
||
| return 0 if trips.length == 0 |
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 use of a guard clause
| def create | ||
| if rating_completed? | ||
| if params[:passenger_id] | ||
| passenger = Passenger.find_by(id: params[:passenger_id]) |
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.
All of the work done in this method could be moved to a model method. An instance method on Passenger would probably work well, something like Passenger#request_trip.
Note that you can call methods of one model from another model, so Passenger#request_trip could call Driver.available_driver.
| def self.available_driver | ||
| available_drivers = Driver.where(available: true) | ||
| driver = available_drivers.sample | ||
| return driver |
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 love that you made this a separate model method - good instincts!
Regarding naming, we don't typically include the name of the class in the name of a method for that class. Something like Driver.first_available or .select_available might work well.
| end | ||
| end | ||
|
|
||
| average_rating = total_rating / trips.length |
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 ignore trips with a nil rating when computing the sum, but still include them when dividing for the average here. That means that an un-rated trip will drop the driver's average rating the same as a rating of 0 would!
|
|
||
| resources :passengers do | ||
| resources :trips, only: [:create, :show] | ||
| 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.
I don't think you need the :show route for trips nested under passengers. That would create a route like passengers/:passenger_id/trips/:id.
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