Skip to content

Earth - Denise & Ringo#2

Open
ringolingo wants to merge 20 commits intoAda-C14:masterfrom
ringolingo:master
Open

Earth - Denise & Ringo#2
ringolingo wants to merge 20 commits intoAda-C14:masterfrom
ringolingo:master

Conversation

@ringolingo
Copy link
Copy Markdown

Assignment Submission: OO Ride Share

Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.

Reflection

Question Answer
How did getting up to speed on the existing codebase go? If it went well, what worked? If it didn't go well, what will you do differently next time? Having to read and understand so much existing code was a new challenge for both of us but we managed to get up to speed pretty well. Our approach was to skim over the whole codebase overall and then get into testing to help us see how the individual pieces worked in more depth. We found this worked well for this size of project and where most of the pieces were kinds of files or methods we'd seen before. In a different project, we might need a different approach.
What inheritance relations exist between classes? Driver, Trip, and Passenger all inherit from CsvRecord. They all are subclasses from CsvRecord and used the parent class version of the load_all method. Each of them had their own version of the from_csv method. Each of them used the parent class id variable in the constructor, but then had their own versions of the rest of the instance methods.
What composition relations exist between classes? TripDispatcher has a one-to-many composition relationship with the other classes because it contains many instances of each of them inside one instance of itself. Trip has a many-to-many composition relationship with both Driver and Passenger, because one driver and one passenger can have many trips in their @trips array.
Describe a decision you had to make when working on this project. What options were you considering? What helped you make your final decision? One decision that we had to make was how to handle the total_revenue method in cases where a drive cost less than the $1.65 fee. We decided that our app would not charge the fee in those cases, so the driver would take their percentage out of the total. We considered taking the fee anyway or taking a partial fee but decided not to because our app believes in paying our drivers fairly.
Give an example of a template method that you implemented for this assignment The .from_csv method was a template method that we implemented. The parent class had a method that established the signature and then each child class had its own code to run when that method was called.
Give an example of a nominal test that you wrote for this assignment One nominal test that we wrote for the net_expenditures method was making up a passenger with a few rides and checking that their total expenditures would add up to the costs of the rides that we made up.
Give an example of an edge case test that you wrote for this assignment One edge case test that we wrote for the total_time_spent method was to check that if the passenger trip array was empty it would raise an ArgumentError.
What is a concept that you gained more clarity on as you worked on this assignment We have gained a better understanding of inheritance and how a child class can behave while working with the parent's instance methods. This also helped us see how a child class can modify or overwrite a parent's methods and why that can be useful. Also we both gained more clarity on how to use Minitest.

Copy link
Copy Markdown

@dHelmgren dHelmgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread lib/driver.rb
@@ -0,0 +1,71 @@
require 'csv'
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since CsvRecord already includes csv, you don't need to include it here. :)

Comment thread lib/passenger.rb
Comment on lines +20 to +29
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
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread test/driver_test.rb
Comment on lines +197 to +199
it "returns zero if driver has no trips" do
expect(@inactive_driver.total_revenue).must_equal 0
end
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this return zero and Passenger raise an error? disagreement? It's important for readability that things like this are consistent!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants