Skip to content

Time - Alicia & Vicki#5

Open
aecombs wants to merge 18 commits intoAda-C13:masterfrom
aecombs:master
Open

Time - Alicia & Vicki#5
aecombs wants to merge 18 commits intoAda-C13:masterfrom
aecombs:master

Conversation

@aecombs
Copy link

@aecombs aecombs commented Feb 28, 2020

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? It was difficult and took a long time, but we got there. It went okay, we probably needed more time to digest it.
What inheritance relations exist between classes? The Trip, Driver, and Passenger classes were children of the CsvRecord class.
What composition relations exist between classes? TripDispatcher was composed of Drivers, Passengers, and Trips. Trips were composed of Passengers and Drivers.
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 had to decide how to handle what would happen if the cost of a ride was less than $1.65. We decided to wave the fee and still give the driver 80% profit from that ride.
Give an example of a template method that you implemented for this assignment In the Driver class, we added the .from_csv method.
Give an example of a nominal test that you wrote for this assignment We had nominal tests for the trip class to test the duration method to make sure that the calculated result is what we expected.
Give an example of an edge case test that you wrote for this assignment We wrote and edge case test for the trip end_time method to make sure that a trip was given a valid end time.
What is a concept that you gained more clarity on as you worked on this assignment We gained more clarity on inheritance and how to practice TDD.

vwhwang and others added 18 commits February 24, 2020 14:55
…assenger,trip,tripdispatch. uncommented driver test.
…d_driver and add_trip method in driver class. updated connect method in trip class. test for add_trip for driver passed
Copy link

@beccaelenzil beccaelenzil left a comment

Choose a reason for hiding this comment

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

Good work overall. You've met the learning goals around working with a large existing coding base and working with classes using composition and inheritance. I've left a few comments for you to review. Your tests could be a bit more thorough accounting for edge cases for instances where there are no trips or trips in progress. Keep up the hard work!

@passenger.add_trip(trip2)
end

it "will calculate the total time spent on their trips" do

Choose a reason for hiding this comment

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

You should also test the edge case of a passenger having no trips.

@trips << trip
end

def average_rating

Choose a reason for hiding this comment

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

Driver and Passenger methods should handle incomplete trips.
Both these methods will hit an error if you try to run them on a user with an incomplete trip. There are a couple of workarounds for this.

You could explicitly ignore trips with a nil cost:

trips.each do |trip|
  if trip.cost.nil?
    next
  end
  # ... add to the total ...
end

Or even better, you could write a helper method that returns a list of complete trips:

def completed_trips
  return @trips.reject { |t| t.end_time.nil? }
end

def net_expenditures
  completed_trips.each do |trip|
    # ... same logic as before ...
  end
end

@beccaelenzil
Copy link

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 see in line comment
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 ✔️
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

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) I don't see a place that this is explicitly tested. Take a look and correct me if I'm wrong!
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?
Descriptive/Readable
Concise
Logical/Organized

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.

3 participants