Earth - Denise & Ringo#2
Conversation
…ed assign_new_trip
dHelmgren
left a comment
There was a problem hiding this comment.
OO Ride Share
Major Learning Goals/Code Review
| Criteria | yes/no, and optionally any details/lines of code to reference |
|---|---|
The code demonstrates individual learning about Time and the responsibility of Trip.from_csv, and uses Time.parse in Trip.from_csv |
✔️ |
The code demonstrates breaking out complex logic in helper methods, such as making a helper method in Trip to calculate duration |
✔️ |
There are tests for the nominal cases for the Passenger#net_expenditures and Passenger#total_time_spent |
✔️ |
There is at least one edge case test for either Passenger#net_expenditures or Passenger#total_time_spent testing if the passenger has no trips |
✔️ |
Practices inheritance. Driver inherits from CsvRecord, and implements from_csv |
✔️ |
Employs problem-solving and implements Driver#average_rating and Driver#total_revenue |
✔️ |
Implements the TripDispatcher#request_trip, which creates an instance of Trip with a driver and passenger, adds the new trip to @trips, and changes the status of the driver |
✔️ |
Practices composition. In TripDispatcher#request_trip, the driver gets connected to the new trip, the passenger gets connected to the new trip |
✔️ |
| Practices git with at least 10 small commits and meaningful commit messages | ✔️ |
Testing Requirements
a
| Testing Requirement | yes/no |
|---|---|
| There is reasonable test coverage for wave 1, and all wave 1 tests pass | ✔️ |
| There is reasonable test coverage for wave 2, and all wave 2 tests pass | ✔️ |
| Wave 3: Tests in wave 1 and wave 2 explicitly test that only completed trips should be calculated (and ignore in-progress trips) | ✔️ |
There is reasonable test coverage for TripDispatcher#request_trip, and all tests pass |
✔️ |
Overall Feedback
| Overall Feedback | Criteria | yes/no |
|---|---|---|
| Green (Meets/Exceeds Standards) | 8+ in Code Review && 3+ in Functional Requirements | ✔️ |
| Yellow (Approaches Standards) | 6+ in Code Review && 2+ in Functional Requirements | |
| Red (Not at Standard) | 0-5 in Code Review or 0,1 in Functional Reqs, or assignment is breaking/doesn’t run with less than 5 minutes of debugging |
Code Style Bonus Awards
Was the code particularly impressive in code style for any of these reasons (or more...?)
| Quality | Yes? |
|---|---|
| Elegant/Clever | ✅ |
| Descriptive/Readable | ✅ |
| @@ -0,0 +1,71 @@ | |||
| require 'csv' | |||
There was a problem hiding this comment.
since CsvRecord already includes csv, you don't need to include it here. :)
| raise ArgumentError.new("no trips found for this passenger") if @trips.empty? | ||
| complete_trips = @trips.find_all { |trip| trip.end_time != nil } | ||
| return complete_trips.sum { |trip| trip.cost } | ||
| end | ||
|
|
||
| def total_time_spent | ||
| raise ArgumentError.new("no trips found for this passenger") if @trips.empty? | ||
| complete_trips = @trips.find_all { |trip| trip.end_time != nil } | ||
| return complete_trips.sum { |trip| trip.duration } | ||
| end |
There was a problem hiding this comment.
so I wouldn't actually call this a good situation for an exception! things like ArgumentError tend to indicate that something broken happened, or a bad state was reached. It's totally normal for a passenger to have never used a service before! Instead, the thing to do would to be to return 0 in those situations.
| it "returns zero if driver has no trips" do | ||
| expect(@inactive_driver.total_revenue).must_equal 0 | ||
| end |
There was a problem hiding this comment.
Why does this return zero and Passenger raise an error? disagreement? It's important for readability that things like this are consistent!
Assignment Submission: OO Ride Share
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection