Time - Lola - Yoyo

[ydunbar]

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?	Diagramming and working backwards helped us a lot.
What inheritance relations exist between classes?	CsvRecord was a parent to Passenger and Trip classes.
What composition relations exist between classes?	TripDispatcher connected Trips to both Passenger and Driver. CsvRecord created the file directories and read the data files to provide data for Trips, Passenger and Driver.
Describe a decision you had to make when working on this project. What options were you considering? What helped you make your final decision?	We spent the most time thinking about how to write our tests. We decided to spend more time working on our tests rather than refactoring our methods in order to meet our deadline.
Give an example of a template method that you implemented for this assignment	We used the self.from_csv method for Passenger and Driver.
Give an example of a nominal test that you wrote for this assignment	request_trip returns an instance of a trip.
Give an example of an edge case test that you wrote for this assignment	Raises ArgumentError for unavailable driver
What is a concept that you gained more clarity on as you worked on this assignment	Writing more tests using edge cases. Also being comfortable linking data between classes.

[ydunbar]

Cleaned up extra spaces and comments.

[tildeee]

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	no, all of the tests for these methods assume that the passenger has two trips.
Practices inheritence. 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	This implementation seems to connect the driver and trip twice, which is incorrect implementation! The driver ends up getting the trip added twice. Read my comments for more details.
Practices composition. In TripDispatcher#request_trip, the driver gets connected to the new trip, the passenger gets connected to the new trip	✔️, yes, but similar to above, the passenger gets connected to the trip twice.
Practices git with at least 10 small commits and meaningful commit messages	✔️

Testing Requirements

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	There's a bug in the current tests, and I would expect some test coverage around the trips array in TripDispatcher to be checked for a new trip

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

Additional Feedback

Overall, I think that this project clearly demonstrates that you all got a lot of learning out of this: your tests were clearly written, and your implementation all made sense. It's clear to me that this project is a victory :) You got through pretty a lot of the requirements.

The biggest missing feature was implementing and testing compatibility of in-progress trips to the rest of your project.

There are just some small subtle bugs, and there were enough of them, particularly around the logic of requesting a trip. Overall, your bugs were that you did everything twice on accident. Overall, I think that a good refactoring of trying to simplify logic would have saved you! I'm leaving some comments to try to lead you to thinking about that refactoring, but let me know if you have questions.

I know that you all had some edge case tests on the Driver's average rating method, but you didn't anticipate a lot of other edge case tests for other instance methods. I'd encourage you to write more tests on instance methods that may rely on empty arrays or non-typical inputs.

Based on the feedback I have this project, I would say "don't worry too much," but take this feedback as a sign to see what pieces of small logic, methods, and tests you may need to study more. Overall, the big learning goals of OOP and composition and inheritance were definitely met.

Let me know if you have any questions on some of the more subtle logic detail that you had, or about anything in general!

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	✅, the code style is very logical, organized, and consistent, especially the tests! The tests were so easy to read because of how you all organized it.

[tildeee]

This is a really good opportunity for refactoring to use an Enumerable method, like sum, instead of needing to increment rating_total in an each loop

[tildeee]

Bit of a nitpick, but the naming convention for these files should match the class that you're testing, so instead of trip_dispatch_test.rb, maybe trip_dispatcher_test.rb

[tildeee]

this test name feels vague, and i'm unsure what we're testing for. Could it have a more clear name?

[tildeee]

last_trips_driver is an ID... so it'll be 2 at this point. It won't be an instance of Driver. drivers is an array of instances of Drivers. It will never carry driver IDs by y'alls implementation. You all probably WANTED this line to be

expect(drivers.include?(driver)).must_equal false

since driver is an instance of Driver, but it didn't pass

This test has the right intention-- it's checking that the list of available drivers no longer has the newly unavailable driver. But that's not how arrays of objects work! The drivers array will always have Driver 2, because it was built with the line drivers = dispatcher.find_available_drivers BEFORE Driver 2 got assigned a trip, so it includes Driver 2. There is no line of code that says "take out Driver 2 because Driver 2's status changed to unavailable."

If I were to refactor this line, I'd probably make the array of available drivers now, AFTER the Act step. I might do

expect(dispatcher.find_available_drivers).wont_include passenger.trips.last.driver

The test line below (directly checking the status of Driver 2) should remain, too!

[tildeee]

On line 65, you're calling the add_trip method on the driver.

But in line 70, you're also calling the connect_driver method on the trip... which calls the add_trip method on the driver! You're doing it twice!

Could you refactor the logic so that you're only adding the trip to the driver once?

[tildeee]

Similarly, on line 67, you're calling the add_trip method on the passenger,

but in line 69, you call connect on trip, which also calls the add_trip methd on passenger.

How do you need to refactor this logic so that we're only adding the trip to the passenger once? Where does it make the most sense to put this logic-- on TripDispatcher, Trip, or Passenger? What information do you need?

Lastly, what tests would you all have needed to write in order to catch this bug?

[tildeee]

One last thing to think about:

A line that sticks out of this logic is setting the driver's status to unavailable. But this should ALWAYS happen every time we connect a trip and a driver. Can the logic for changing a driver's status to unavilable live in the same spot as when a trip and driver get connected, or at least get moved out of this TripDispatcher method?