Space-Hala Haddad and Becca Roach by halahaddad1 · Pull Request #12 · Ada-C13/oo-ride-share

halahaddad1 · 2020-02-28T22:17:28Z

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 took a whole day to go through the database, we did not move forward with the waves before we had a working understanding of the database and tests
What inheritance relations exist between classes?	CsvRecord is the parent class of classes : Passenger, Trip and Driver
What composition relations exist between classes?	TripDispatcher has Passengers, Trips 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?	One was deciding how to calculate the total revenue of a driver, the design document had mentioned extracting $1.65 from a trip, but we decided to make a decision to only extract that amount if the trip cost was over $1.65. we considered doubling that number, or at least having a minimum threshold before subtracting that amount, but what helped in our final decision was knowing that we don't have all the information needed to make that business logic!
Give an example of a template method that you implemented for this assignment	The load_all method inherits .from_csv method and that is the way all the classes accessed data from the CSV files
Give an example of a nominal test that you wrote for this assignment	We wrote a test in trip_dispatcher that made sure the driver picked was the most eligible driver according to "wave 4" requirements. Another example is the total_time_spent for a passenger's trip was accurate
Give an example of an edge case test that you wrote for this assignment	We wrote a few tests that checked if input was less than 1 or nil to verify id numbers or passenger id numbers
What is a concept that you gained more clarity on as you worked on this assignment	Tdd and writing tests in general, also debugging!

Merge branch 'master' of https://github.com/halahaddad1/oo-ride-share

kaidamasaki · 2020-03-10T22:59:06Z

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 edge case tests.
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	✔️ (Though the status is switched in `pick_best_driver` which is non-obvious at first. I'd recommend against mixing a query with a command like this. We're talking about this tomorrow.)
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)	No.
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?
Perfect Indentation	✅
Elegant/Clever
Descriptive/Readable
Concise
Logical/Organized	✅

kaidamasaki

Good job. Here are some tips to clean things up. :)

kaidamasaki · 2020-03-06T18:16:13Z

+      unless super(id) > 0 
+        raise ArgumentError
+      end


I'm pretty sure super doesn't do what you think it does. For initialize doesn't return a value it just initializes the CsvRecord and raises an ArgumentError if there's a bad id so checking here is unnecessary.

Suggested change

unless super(id) > 0

raise ArgumentError

end

kaidamasaki · 2020-03-06T18:22:30Z

+      @vin = vin
+
+      unless vin.length == 17
+        raise ArgumentError


Including a message that contains the bad argument is helpful for debugging and your users.

Suggested change

raise ArgumentError

raise ArgumentError, "Bad vin: #{vin}"

kaidamasaki · 2020-03-09T17:20:11Z


 module RideShare
  class Trip < CsvRecord
-    attr_reader :id, :passenger, :passenger_id, :start_time, :end_time, :cost, :rating


I'm not clear on why you removed :id from this list.

kaidamasaki · 2020-03-09T17:21:51Z

+      unless passenger_id == nil
+        if passenger_id < 1
+          raise ArgumentError
+        end
+      end


Can be simplified as:

Suggested change

unless passenger_id == nil

if passenger_id < 1

raise ArgumentError

end

end

raise ArgumentError, "Bad passenger_id: #{passenger_id}" unless passenger_id.nil? || passenger_id >= 1

kaidamasaki · 2020-03-09T17:25:55Z

+    def available_drivers
+      available_drivers_list = []
+      @drivers.each do |driver|
+        if driver.status == :AVAILABLE && (driver.trips.each { |trip| trip.end_time != nil })


You almost certainly want all? here not each. (each always returns the original array, all? returns a boolean.)

Suggested change

if driver.status == :AVAILABLE && (driver.trips.each { |trip| trip.end_time != nil })

if driver.status == :AVAILABLE && (driver.trips.all? { |trip| trip.end_time != nil })

kaidamasaki · 2020-03-09T17:31:23Z

+      best_driver = nil
+      available_drivers_list.each do |driver|
+      # Pick first driver in array of available drivers that has nil or no trips
+        if driver.trips == nil || driver.trips == []


Can be made a little clearer with:

Suggested change

if driver.trips == nil || driver.trips == []

if driver.trips.nil? || driver.trips.empty?

kaidamasaki · 2020-03-09T20:09:53Z

+      driver2.add_trip(trip)
+      driver2.add_trip(trip2)
+
+      expect(driver2.total_revenue).must_equal ((43 + 56) - (1.65 * 2)) * 0.8


You should use must_be_close_to when comparing Floats because they are imprecise.

kaidamasaki · 2020-03-09T20:48:52Z

+          @dispatcher2.available_drivers.each do |driver|
+            if driver.status == :UNAVAILABLE
+              return false
+            else 
+              return true
+            end
+          end


This method doesn't actually check all statuses. It only checks the first status since it returns on the first time through the each loop.

You probably wanted to use .all? instead of .each:

Suggested change

@dispatcher2.available_drivers.each do |driver|

if driver.status == :UNAVAILABLE

return false

else

return true

end

end

@dispatcher2.available_drivers.all? do

driver.status != :UNAVAILABLE

end

halahaddad1 and others added 27 commits February 25, 2020 16:58

wave 1

bb4851f

Becca comments Wave 1

ae558f5

Made a Driver Class

1d8702a

trying to pull

6fee71f

trying to pull

d1de1f0

Driver Class working to instantiate without errors from inheritance

43fbb0d

Merge branch 'master' of https://github.com/halahaddad1/oo-ride-share

3c7e5fd

updated find driver

5e7854f

Updating Trip_dispatcher class

8cd69ae

"Working on wave 2"

aa18f1a

Merge branch 'master' of https://github.com/halahaddad1/oo-ride-share

connect driver added to dispatcher

8e3bdf1

Progress on trip dispatcher.

6622fa8

"progress on trip dispatcher"

a808d33

Merge branch 'master' of https://github.com/halahaddad1/oo-ride-share

Tests included update

36fa3cb

average rating method added

bea6025

added total revenue

01ee5ca

decided how to handle total revenue being less than 1.65

dc5d735

Added tests for Trip Request method.

fd259c9

made edits to trip_request

3128a55

fixed trip dispatcher testing errors

8fbbe8b

wave 4

809dc38

Wave 3 progress

b081cf0

Working on wave 3

2a09b28

Merge branch 'master' of https://github.com/halahaddad1/oo-ride-share

Implemented tests for wave 4, all passing.

b504bc7

Cleaned indents, spacing and added comments for tests

2539b00

Refactored tests for trip_dispatcher_test.rb

3e7a615

Final cleaning and refactoring of code and tests.

ccc7df1

kaidamasaki reviewed Mar 10, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Space-Hala Haddad and Becca Roach#12

Space-Hala Haddad and Becca Roach#12
halahaddad1 wants to merge 27 commits into
Ada-C13:masterfrom
halahaddad1:master

halahaddad1 commented Feb 28, 2020

Uh oh!

kaidamasaki commented Mar 10, 2020

Uh oh!

kaidamasaki left a comment

Uh oh!

kaidamasaki Mar 6, 2020

Uh oh!

kaidamasaki Mar 6, 2020

Uh oh!

kaidamasaki Mar 9, 2020

Uh oh!

kaidamasaki Mar 9, 2020

Uh oh!

kaidamasaki Mar 9, 2020

Uh oh!

kaidamasaki Mar 9, 2020

Uh oh!

kaidamasaki Mar 9, 2020

Uh oh!

kaidamasaki Mar 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	if driver.status == :AVAILABLE && (driver.trips.each { \|trip\| trip.end_time != nil })
	if driver.status == :AVAILABLE && (driver.trips.all? { \|trip\| trip.end_time != nil })

	if driver.trips == nil \|\| driver.trips == []
	if driver.trips.nil? \|\| driver.trips.empty?

Uh oh!

Conversation

halahaddad1 commented Feb 28, 2020

Assignment Submission: OO Ride Share

Reflection

Uh oh!

kaidamasaki commented Mar 10, 2020

OO Ride Share

Major Learning Goals/Code Review

Testing Requirements

Overall Feedback

Code Style Bonus Awards

Uh oh!

kaidamasaki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants