Fire - Ida Goitom and Earth - Christina Minh #10
Conversation
… index, new, and show actions for PassengersController, and partial view of form
…n'). Passenger#total_charged with tests. Changed Passenger's show.html.erb to show total charged and list trips(date).
… that users must enter provide a name and vin
…STful routes. Added corresponding views
…troller#index and #new.
…refactor. Changed TripsController#new accordingly.
…enu when editing an existing trip
There was a problem hiding this comment.
Rideshare Rails
Major Learning Goals/Code Review
| Criteria | yes/no, and optionally any details/lines of code to reference |
|---|---|
| Practices collaborating with git, and all team members contribute git commits and best git practices | ✔️ |
| Demonstrates understanding of relationships by giving accurate answers to the reflection questions and appropriate code in the models | ✔️ |
| Logic to calculate a driver's average rating and total earnings is located in the model, and it has unit tests | This pains me to take away this checkmark, since your model tests otherwise are really well-written and thorough... But technically y'all didn't test toggle_status, and truthfully I would expect additional tests for the custom methods to test edge cases, such as what if the driver doesn't have trips. |
| There are reasonable tests to test the validation requirements for all models | ✔️, your trip validation tests are particularly amazing! |
| There are reasonable tests to test the relationship requirements for all models | ✔️ |
| There are reasonable tests to test the controller actions for all controllers | ✔️ |
| The app has an attractive and usable user interface | ✔️ |
| The app uses/is compatible with database seeds | ✔️ |
Router code is clean: uses resources and RESTful routes |
✔️ |
Functional Requirements
| Functional Requirement | yes/no |
|---|---|
| On the passenger's details page, I want to be able to see total charged, list of trips, a link to edit, and delete | ✔️ |
| When adding a new passenger, I want to see errors and validations that show that a passenger must be provided a name and a phone number, so that I cannot make a passenger without name or phone number | ✔️ |
| On the passenger's details page, I can create a new trip for this passenger, with an assigned driver and no rating | Doesn't automatically assign a driver, but brings user to a form with a drop down. The drop down only lists available drivers! |
| On the driver's details page, I want to be able to see total earnings, average rating, list of trips, a link to edit, and delete | ✔️ |
| When adding a new driver, I want to see errors and validations that show that a driver must be provided a name and VIN, so that I cannot make a driver without name or VIN | ✔️ |
| On the trip's detail page, I want to be able to view details, assign a rating, navigate to the trip's passenger, driver, a link to edit, and delete | ✔️ |
Overall Feedback
| Overall Feedback | Criteria | yes/no |
|---|---|---|
| Green (Meets/Exceeds Standards) | 7+ in Code Review && 5+ in Functional Requirements | ✔️ |
| Yellow (Approaches Standards) | 5+ in Code Review && 4+ in Functional Requirements, or the instructor judges that this project needs special attention | |
| Red (Not at Standard) | 0-4 in Code Review or 0-3 in Functional Reqs, or assignment is breaking/doesn’t run with less than 5 minutes of debugging, or the instructor judges that this project needs special attention |
Additional Feedback
Great work on this project, Ida and Christina!
Overall, your code looks great. I'm seriously really impressed and happy with how your model tests are written and how they read.
Overall, I'm adding a few comments to hopefully help code style-- a common theme I saw was having things (variables, methods) that probably could be refactored out. It's good to have things be very clear and organized, but when we have too many things, it'll end up being confusing for your teammates-- your teammates will ask, "do we need this file? Do we use this method?" and the communicate gets harder :(
One other pattern I'm going to point out is-- for a better user experience, now that we know the power of redirects, it may be be better to redirect, instead of head :not_found! This applies to your code in your Drivers, Trips, and Passengers controllers.
Other than that, your project is great-- great work on all of the functionality you two built!
Code Style Bonus Awards
Was the code particularly impressive in code style for any of these reasons (or more...?)
| Quality | Yes? |
|---|---|
| Perfect Indentation | |
| Elegant/Clever | |
| Descriptive/Readable | |
| Concise | |
| Logical/Organized |
| @driver = Driver.find_by(id: driver_id) | ||
| # | ||
| if @driver.nil? | ||
| head :not_found |
There was a problem hiding this comment.
Now that we know the power of redirects, in these cases, it might be best to redirect to a page like the homepage, in case the driver isn't found.
| def show | ||
| driver_id = params[:id].to_i | ||
| @driver = Driver.find_by(id: driver_id) | ||
| # |
There was a problem hiding this comment.
Nitpicking: This is an empty line with a comment, but no comment :) feel free to delete this before submission
| else | ||
| render :new | ||
| end |
|
|
||
| def edit | ||
| @driver = Driver.find_by(id: params[:id]) | ||
| if @driver.nil? |
There was a problem hiding this comment.
Similar to above, now that we know the power of redirects, in these cases, it might be best to redirect to a page like the homepage, in case the driver isn't found.
| all_trips = self.trips | ||
|
|
||
| all_trips.each do |trip| |
There was a problem hiding this comment.
Here, the value of all_trips is self.trips, and it's used once... Personally, I love having variables in order to give context, but I personally think that self.trips IS self-explanatory/self-documenting enough, that the code still reads well if we use self.trips directly. Consider using self.trips instead!
Your code might look like this, instead (deleting line 8 as well)
| all_trips = self.trips | |
| all_trips.each do |trip| | |
| self.trips.each do |trip| |
| total = 0 | ||
| all_trips.each do |trip| | ||
| total += trip.rating.to_f | ||
| end |
There was a problem hiding this comment.
Here, you're summing all of the trip's rating attributes. Consider using something like sum!
| def total_driver_trips | ||
| all_trips = self.trips | ||
| total_trips = [] | ||
| all_trips.each do |trip| | ||
| total_trips << trip | ||
| end | ||
| return total_trips | ||
| end |
There was a problem hiding this comment.
I am a little unsure about how this method differs from the original self.trips array. It seems to shovel every trip from self.trips into a new array... Could we get rid of this method, and in the view, use @driver.trips?
| def self.find_available_driver | ||
| available_driver = self.find_by(available: true) | ||
| return available_driver | ||
| end | ||
|
|
||
| def toggle_status | ||
| self.available = !self.available | ||
|
|
||
| return self.save | ||
| end |
| Rails.application.routes.draw do | ||
| # For details on the DSL available within this file, see https://guides.rubyonrails.org/routing.html | ||
| end |
There was a problem hiding this comment.
Nitpicking... I see this file named routes 2.rb. This seems like a mistake? If it is a mistake, you all should feel free to delete that file before submission ;)
| resources :drivers do | ||
| resources :trips, only: [:index] | ||
| end | ||
| resources :homepages |
There was a problem hiding this comment.
y'all probably didn't need all of resources :homepages since there's truly one route that you care about; it'll be best to be selective here!
Assignment Submission: Rideshare Rails
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions. These questions should be answered by all team members together, not by a single teammate.
Reflection