-
Notifications
You must be signed in to change notification settings - Fork 26
Amber Lynn & Dionisia - Edges | Rideshare-Rails #21
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
…w driver in application.html.erb
… all trips for given passenger next
merging branches
footer additions and corrections
Rideshare RailsWhat We're Looking For
Hey all! Good work on this project. I think it looks pretty great! I think that everything looks pretty good! I have a couple of comments on how to improvement, but with only one urgent comment: When seeding your database, you all run into issues about a lot of the Passenger and Trip data not being loaded correctly. The trips aren't loading from the seed file because it can't find the correct passenger... The Passengers aren't loading because you all have a validation on the passenger data. It says that a lot of the passenger data from the seed files don't fit your validation, so they don't get saved to the database. That being said, I'll put the rest of the comments on Github, and overall this is pretty good! Good work |
| rating += trip.rating | ||
| counter += 1 | ||
| end | ||
| return (rating.to_f/counter).round(2) |
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.
This is pretty good -- a more programmer way of looking at this problem is:
- Instead of
counterand incrementing it, just usingself.trips.sizeorself.trips.countto get you the size of the array - Instead of using a
.each, using the Enumerablessummethod https://ruby-doc.org/core-2.5.1/Enumerable.html#method-i-sum
This method could be as slim as:
def average_rating
return ( self.trips.sum / self.trips.size ).round(2)
end| end | ||
| end | ||
| return trips | ||
| 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.
This method returns an array of all trips associated with this Driver.
Why couldn't we just use the built in .trips method? The built in .trips method returns an array of all trips associated with this instance of Driver.
| self.trips.each do |trip| | ||
| total += (trip.cost/100.to_f) | ||
| end | ||
| return total |
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 might want to look into the Enumerables sum method.
You could add them all up and then divide by 100 after.
| end | ||
| end | ||
| return trips | ||
| 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.
Same as above: Why do we need to build a new list_trips method when Passenger has_many Trips?
| @@ -0,0 +1,17 @@ | |||
| <div class="driver-edit-container"> | |||
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.
Why is this class driver-edit-container in the new view?
| @@ -0,0 +1,19 @@ | |||
| <div class="container"> | |||
| <div class="div3"> | |||
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.
In general, class names like div3 aren't so great-- I have no idea what they represent or what they'll hold! When I add styles to this in the CSS file, how do I know what this is supposed to look like by the name?
| @@ -0,0 +1,19 @@ | |||
| <div class="container"> | |||
| <div class="div3"> | |||
| <h3 id="pass_list" class="passenger_list">List of Passengers</h3> | |||
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.
Just a small comment-- it happens to be CSS/HTML naming convention that class names are hyphen separated. I know! So many different naming conventions.
| <div class="div3"> | ||
| <h3 id="pass_list" class="passenger_list">List of Passengers</h3> | ||
| <ol class="passenger_list"> | ||
| <% i = 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.
Why was i = 0 declared?
| root 'trips#index' | ||
|
|
||
| resources :trips do | ||
| resources :passengers, include: [:index, :show] |
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.
Why do we nest passenger routes in trips?
Nesting passenger routes in trips gives us the routes like
trip_passengers GET /trips/:trip_id/passengers ... What does that get us?
Also, is the syntax isn't include:, it's only:
|
|
||
| # Drivers routes | ||
| resources :drivers do | ||
| resources :trips, include: [:new, :index, :show] |
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.
Same as above: Why do we need to nest trips routes into drivers?
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