Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Michelle and Katrina - Edges - OO-Ride-Share #23

Open
wants to merge 42 commits into
base: master
Choose a base branch
from

Conversation

kangazoom
Copy link

@kangazoom kangazoom commented Sep 1, 2018

OO Ride Share

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer Instructor Feedback
Describe a design decision you had to make when working on this project. What options were you considering? What helped you make your final decision? In TripDispatcher#request_trip, we decided to find the maximum trip ID and add to it instead of merely finding the length so that if the IDs are not in sequential order, then the code will still run correctly. We also refactored our code to use enumerables to enhance readability in case we needed to edit it in the future.
Describe any examples of composition that you encountered in this project, if any In TripDispatcher, we created a method called request_trip and inside that method, we called methods in other classes. For example, we created a new instance of Driver and called new_driver.status as well as added the new trip to the Driver like new_driver.add_diven_trip(trip). We also did the same by creating a new passenger and calling a method from the passenger/user class like new_passenger.add_trip(trip). Your answer gets at this, but I want to call out this vocab explicitly. By composition we mean a has-a or has-many relationship. Examples in this project include "a TripDispatcher has-many Drivers, Passengers and Trips", "a Driver has-many Trips", "a Trip has-a Driver".
Describe the relationship between User and Driver Driver inherits from User, meaning that all of User's methods are available to the Driver without needing to create them. Further, the Driver is a type of User.
Describe a nominal test that you wrote for this assignment. For request_status in TripDispatch_spec, we wrote a test called "Connects passenger with trip in progress" in which we checked to make sure the Passenger instance stored in the in-progress Trip instance matched the Passenger instance that was added to the Trip.
Describe an edge case test that you wrote for this assignment For net_expenditures in Driver_spec, we created a test called "returns a float with correct money format" in which we checked what would happen if we got a money value return that went beyond two decimal places in order to ensure it only returned those two decimal places.
Describe a concept that you/your pair gained more clarity on as you worked on this assignment We learned a lot about taking input as a parameter and what is actually necessary to instantiate a new class instance. We also learned more about debugging errors in the code we wrote versus the tests we wrote.
What are two discussion points that you and your pair discussed when giving/receiving feedback from each other that you would be willing to share? (1) We discussed telling each other right away before we went down a long path of code whether or not we thought it would work and why. (2) We discussed giving each other space to think through and figure out code/issues before diving in.

@kangazoom
Copy link
Author

should we resolve conflicts??

specs/driver_spec.rb
specs/test_data/trips_test.csv

@droberts-sea
Copy link

Ride Share

What We're Looking For

Feature Feedback
Baseline
Used Git Regularly yes
Answer comprehension questions yes - see feedback above
Wave 1
Appropriate use of Ruby's Date yes
Trip has a helper method to calculate duration yes
User (passenger) has a method to calculate total cost of all trips yes
Tests for wave 1 missing some edge cases - see inline
Wave 2
Driver inherits from User yes
Driver has add_driven_trip method yes
Driver has method to calculate average rating yes
Driver has method to calculate net expenditures and it uses super yes
Driver has a method to calculate total revenue yes
Tests for wave 2 mostly
Wave 3
TripDispatcher has a new method to create trips yes
creating a trip in TripDispatcher relies on methods in Driver and User (passenger) to modify their own attributes no (sets driver status directly)
Complex logic was correctly implemented yes
Tests for request_trip mostly
Methods from wave 1 and 2 handle incomplete trips yes
Tests for wave 1 and 2 methods with incomplete trips no
Overall Great job overall! This is a strong submission. There are a few places where things could be cleaned up or test coverage expanded, but in general I'm pretty happy with what I see. Keep up the hard work!

def net_expenditures()
valid_trips = @trips.reject { |trip| trip.cost.nil? }
return valid_trips.sum { |trip| trip.cost} if valid_trips.length > 0
end

Choose a reason for hiding this comment

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

Great use of enumerables through here!

describe "user#net_expenditures" do
before do
@user = RideShare::User.new(id: 9, name: "Merl Glover III",
phone: "1-602-620-2330 x3723", trips: [])

Choose a reason for hiding this comment

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

I think you're missing some edge cases that are common to both these User methods:

  • What happens if the user has no trips?
  • What happens if the user has an incomplete trip?


if !(super.nil? && total_driver_revenue.nil?)
return ("%.2f" % (super - total_driver_revenue)).to_f
end

Choose a reason for hiding this comment

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

Good use of super here

end
end

describe "average_rating method" do
describe "net_expenditures" do
before do
@driver = RideShare::Driver.new(id: 54, name: "Rogers Bartell IV",

Choose a reason for hiding this comment

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

You're missing some interesting cases here. What if a driver has

  • No driven trips
  • No trips as a passenger
  • An incomplete trip as either a driver or passenger
  • Made more money than they've spent

def request_trip(user_id)
current_passenger = find_passenger(user_id)
available_driver = @drivers.find { |driver| driver.status == :AVAILABLE }

Choose a reason for hiding this comment

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

You're not screening for getting the same person who requested this trip


describe 'Request Trip Method ' do
before do
@dispatcher = RideShare::TripDispatcher.new(USER_TEST_FILE, TRIP_TEST_FILE, DRIVER_TEST_FILE)

Choose a reason for hiding this comment

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

What happens if there are no drivers available?

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