Fire - Tram & Earth - Jasmine #7
Conversation
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 | ✔️ WONDERFUL! |
| There are reasonable tests to test the validation requirements for all models | ✔️ |
| 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 | ✔️ |
| 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
FANTASTIC work on this project, Jasmine and Tram!! Your project submission looks GREAT-- the code overall looks great, the functionality is all there, and it has a lot of polish.
I want to specifically compliment the user experience of the whole app. Requesting a trip was so nice-- the button text that changes if it's unrated! The red validation error texts! So nice.
Another thing I want to say is that the code style demonstrated in the driver.rb file and in the tests (driver_test.rb) was really clean and enjoyable to read. I like that your tests had a lot of explicit Trip.create() lines in each test to describe the changing circumstances, which made the tests really clear and easy to understand.
Keep up the great work!
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 | ✅ |
| def average_rating | ||
| all_ratings = self.trips.map { |trip| trip.rating} | ||
| return nil if all_ratings.empty? | ||
|
|
||
| number_ratings = all_ratings.filter { |rating| rating unless rating.nil?} | ||
| average = number_ratings.sum / number_ratings.length.to_f | ||
| return average / 10 == 0 ? average : average.round(1) | ||
| end |
There was a problem hiding this comment.
This is such lovely and clean code! Great implementation. Well done!! I'm inspired!!!
| def set_available | ||
| self.update(available: true) | ||
| end | ||
|
|
||
| def set_unavailable | ||
| self.update(available: false) | ||
| end |
There was a problem hiding this comment.
I love these helper methods! They're wonderful!
|
|
||
|
|
||
| resources :drivers | ||
| # don't we need trips :index and :new for the nested routes? |
There was a problem hiding this comment.
To answer this question: You might! But in this specific rubric, I'm not looking for it right now ;) But definitely worth considering.
| # Your code here | ||
| end | ||
| it "can change availability to false from true" do | ||
| new_driver.save |
There was a problem hiding this comment.
Veeerryyyy small note: I think I might understand why you all included this line-- to make it clear that available is true. However, from my point of view, if you all had something like the set_available test, I'd think that was totally reasonable.
| new_driver.save | |
| new_driver.available = true |
An even weirder thing you could do is make your "Arrange" step an assert that checks that available isn't false, in case you wanted to assume a default value of true
| new_driver.save | |
| expect(new_driver.available).must_equal true |
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