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

Val & Carly - OO Ride Share - Edges #18

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
5df17ac
Dee's change to the test code.
CarlyReiter Aug 27, 2018
b39126c
modified start and end instances of Time in trip_dispatch.
CarlyReiter Aug 27, 2018
98b2ad2
Create test for Trip#initialize start time not after end time
valgidzi Aug 27, 2018
2eaa6f1
Another attempt at the start and end time test of Wave 1
CarlyReiter Aug 27, 2018
afca7c8
Finally Trip#initialize ArgumentError test raises ArgumentError
valgidzi Aug 28, 2018
7b9b72d
Add test for Trip#calculate_duration method
valgidzi Aug 28, 2018
4e1669a
Test written for net_expenditures on Wave 1.2
CarlyReiter Aug 28, 2018
ed69b8a
Add User#net_expendituresmethod
valgidzi Aug 28, 2018
d12b0b5
Update specs for User#net_expenditures and skipped irrelevant tests
valgidzi Aug 28, 2018
72fef83
Add test for User#total_time_spent method
valgidzi Aug 28, 2018
ecc9317
Wrote total_time_spent method that passed our test. Wave 1 complete.
CarlyReiter Aug 28, 2018
80efa5f
Updated Driver class specs
valgidzi Aug 28, 2018
1d526c3
Created new file for Driver class
valgidzi Aug 28, 2018
97fa9a1
Basic framework of initializing Driver class.
CarlyReiter Aug 29, 2018
e8af14f
Add lib/driver.rb to spec_helper.rb and initialize method for Driver …
valgidzi Aug 29, 2018
5808c50
Updated the conditional that raised an argument error if the ID was n…
CarlyReiter Aug 29, 2018
b99c259
Raise ArgumentError if status is invalid and all Driver#initialize me…
valgidzi Aug 29, 2018
d776175
Add driver attribute and method to Trip class
valgidzi Aug 29, 2018
ae7e9df
Added methods to TripDispatcher class to load drivers and updated cor…
valgidzi Aug 30, 2018
5c8c5b4
Added Driver methods to Driver class - total_revenue, net_expenditure…
CarlyReiter Aug 30, 2018
7310223
Modified add_driven_trip method. Removed parameter and included logi…
CarlyReiter Aug 30, 2018
c090425
Added requried test file to TripDispatcher spec
valgidzi Aug 31, 2018
4a75404
Fixed Driver#methods to pass tests
valgidzi Aug 31, 2018
48b1a91
Create TripDispatcher#request_trip method and make adjustments to Tri…
valgidzi Aug 31, 2018
9f6676c
Updated tests for driver net_expenditure and total_revenue specs unti…
CarlyReiter Aug 31, 2018
240cbe6
Worked more on TripDispatcher::request_trip and affected methods in T…
valgidzi Aug 31, 2018
fdeb16c
Change driver status to :UNAVAILABLE in TripDispatcher#request_trip m…
valgidzi Aug 31, 2018
05bf3a9
Added more specs to TripDispatcher tests
valgidzi Aug 31, 2018
9caae1d
Update load_drivers method in TripDispatcher class
valgidzi Aug 31, 2018
2d296da
Final edits for submission
valgidzi Sep 1, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 85 additions & 0 deletions lib/driver.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
require 'csv'
require 'time'
require_relative "user"

module RideShare

class Driver < User
attr_reader :vin, :driven_trips
attr_accessor :status

def initialize(input)
super(input)

@vin = input[:vin].to_s

if @vin.nil? || @vin == 0
raise ArgumentError, 'ID cannot be blank or less than zero.'
end


if input[:vin].length != 17 || input[:vin].length == 0
raise ArgumentError, 'vin numbers must be 17 characters in length'
end

@status = input[:status]

unless @status == :AVAILABLE
@status == :UNAVAILABLE
Copy link

Choose a reason for hiding this comment

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

This line is not doing what you probably intended it to do.

Please review this code and figure out what should be changed. Once you know what should be changed -- stop! Before you actually make the change, try to write a test first!

Figure out what kind of test would be necessary to verify that line 28 is working, then write that test. Running your tests should then show that the new one is failing. After that you can implement your fix, and run the tests again to verify that it worked.

If you have any trouble following the above suggestions, let me know!

Copy link

Choose a reason for hiding this comment

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

Less important comment on this unless statement: This code is probably a bit "too fragile", meaning it's coded with a very strict expectation of what data values might be in the @status variable.

As written, we're expecting that the only valid values for @status are :AVAILABLE and :UNVAILABLE. This is fine for now as the project requirements only involve those two options, but we could definitely imagine additional status options (like :LOCKED for a driver who has been banned or otherwise prevented from using the system).

In that case it might be simpler to write the conditional this way:

if @status.nil?
   # set @status to :UNAVAILABLE
end

With that above code we have logic that makes :UNAVAILABLE the default status, but we aren't preventing other values from being provided.


end


@driven_trips = input[:trips].nil? ? [] : input[:trips]
end

def add_driven_trip(driven_trip)
@driven_trips << driven_trip

unless driven_trip.is_a? Trip
raise ArgumentError
Copy link

Choose a reason for hiding this comment

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

This check should happen as the first thing within this method.

Traditionally, raising an error like we're doing on this line means that the application stops right then. However, it is possible to use rescue blocks to catch those errors and allow the program to continue running.

In that situation, we will have added an invalid trip to the @driven_trips array, and the rest of the program may function incorrectly and result in more bugs or more incorrect results for users.

Finally, as with my previous comment, I suggest fixing this method by moving the check and raise lines to the beginning of the method. However, first y'all should write a test will fail until the method is fixed.

end

end


def average_rating
total_rating = 0

@driven_trips.each do |trip|
total_rating += trip.rating
end

if @driven_trips.length == 0
average = 0
Copy link

Choose a reason for hiding this comment

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

Minor comment: This check could be implemented as an "early exit" as well:

def average_rating
  return 0 if @driven_trips.empty?

  # ... rest of the method

I also think it's worth considering whether zero is the correct return value when a driver has not yet driven any trips.

else
average = total_rating.to_f/@driven_trips.length
end
return average
end


def total_revenue
total_revenue = 0

@driven_trips.each do |trip|
total_revenue += trip.cost
end

if @driven_trips.length == 0
total = 0
Copy link

Choose a reason for hiding this comment

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

I don't think this check is necessary for the total_revenue. We have the same check in average_rating, but it's necessary there to avoid potentially dividing by zero on line 56.

In this case we have no division, so it should be fine to run all of the code when @driven_trips is empty. However, this check does correct for one bug (but only in one case): without this check the method would return a negative value for drivers without any trips.

Clearly that would be an invalid result, so it's good that we're avoiding it. However, we're still possibly running into that bug when @driven_trips is not empty. If there was a driver who had one trip but the cost for that trip was less than $1.65, then total_revenue will still return a negative value.

So, my suggestion would be to remove the check about @driven_trips being empty, and instead use the same calculation in all cases. Then, have the method return a minimum value of zero: return [0, total].max.

else
total = (total_revenue.to_f - 1.65)*0.8
end
return total
end


def net_expenditures
return super - total_revenue.round(2)

end


end
end
18 changes: 16 additions & 2 deletions lib/trip.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
require 'csv'
require 'time'

module RideShare
class Trip
attr_reader :id, :passenger, :start_time, :end_time, :cost, :rating
attr_reader :id, :passenger, :start_time, :end_time, :cost, :rating, :driver

def initialize(input)
@id = input[:id]
Expand All @@ -11,10 +12,23 @@ def initialize(input)
@end_time = input[:end_time]
@cost = input[:cost]
@rating = input[:rating]
@driver = input[:driver]

if @rating > 5 || @rating < 1
valid_ratings = [*1..5, nil]
unless valid_ratings.include? (rating)
raise ArgumentError.new("Invalid rating #{@rating}")
end

if end_time != nil
if @end_time < @start_time
raise ArgumentError.new
end
end
end

def calculate_duration
duration = @end_time - @start_time
return duration
Copy link

Choose a reason for hiding this comment

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

Minor comment: I think that a better name for this method would be simply duration.

The reason for this is due to the implication behind the phrasing "calculate duration". In that case we're explicitly saying that the duration will be calculated by this method (using the equation on line 30). This is in contrast to a method that simply returns the value of a previously saved/assigned instance variable.

Naming a method in a way that indicates how the method works is considered to be a more fragile approach than avoiding those details. Because if we end up needing to change the method's code, the name may no longer be accurate.

For this reason the Ruby convention for method names like this is to stick with a noun rather than a verb+object combination phrase. Examples:

Non-Ruby style Ruby style
Trip#calculate_duration Trip#duration
Person#find_house Person#house
Invoice#sum_line_items Invoice#price

end

def inspect
Expand Down
126 changes: 96 additions & 30 deletions lib/trip_dispatcher.rb
Original file line number Diff line number Diff line change
@@ -1,16 +1,20 @@
require 'csv'
require 'time'
require 'pry'

require_relative 'user'
require_relative 'trip'
require_relative 'driver'

module RideShare
class TripDispatcher
attr_reader :drivers, :passengers, :trips

def initialize(user_file = 'support/users.csv',
trip_file = 'support/trips.csv')
trip_file = 'support/trips.csv',
driver_file = 'support/drivers.csv')
@passengers = load_users(user_file)
@drivers = load_drivers(driver_file)
@trips = load_trips(trip_file)
end

Expand All @@ -33,44 +37,106 @@ def load_users(filename)
def load_trips(filename)
trips = []
trip_data = CSV.open(filename, 'r', headers: true,
header_converters: :symbol)
header_converters: :symbol)

trip_data.each do |raw_trip|
passenger = find_passenger(raw_trip[:passenger_id].to_i)
driver = find_driver(raw_trip[:driver_id].to_i)

parsed_trip = {
id: raw_trip[:id].to_i,
passenger: passenger,
#2018-06-07 04:18:47 -0700
start_time: Time.parse(raw_trip[:start_time]),
#,2018-06-07 04:19:25 -0700
end_time: Time.parse(raw_trip[:end_time]),
cost: raw_trip[:cost].to_f,
rating: raw_trip[:rating].to_i,
driver: driver
}

create_trip(parsed_trip, driver, passenger, trips)
end

return trips
end

trip_data.each do |raw_trip|
passenger = find_passenger(raw_trip[:passenger_id].to_i)

parsed_trip = {
id: raw_trip[:id].to_i,
passenger: passenger,
start_time: raw_trip[:start_time],
end_time: raw_trip[:end_time],
cost: raw_trip[:cost].to_f,
rating: raw_trip[:rating].to_i
}
def load_drivers(filename)
drivers = []
driver_data = CSV.open(filename, 'r', headers: true, header_converters: :symbol)

driver_data.each do |raw_driver|
user_with_same_id = find_passenger(raw_driver[0].to_i)

parsed_driver = {
id: raw_driver[0].to_i,
vin: raw_driver[1],
name: user_with_same_id.name,
phone_number: user_with_same_id.phone_number,
status: raw_driver[2].to_sym
}

driver = Driver.new(parsed_driver)
drivers << driver
end

return drivers
end


def find_passenger(id)
check_id(id)
return @passengers.find { |passenger| passenger.id == id }
end


def find_driver(id)
check_id(id)
return @drivers.find { |driver| driver.id == id }
end

trip = Trip.new(parsed_trip)
def create_trip(trip_data, driver, passenger, trip_list)
Copy link

Choose a reason for hiding this comment

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

I really like this method! When we have constraints/requirements that are somewhat "implicit", factory methods like this can help ensure that we don't write code that accidentally breaks those constraints.

In this case the Trip class has an implicit constraint that the associated Passenger and Driver must also include the trip instance in their own arrays. Because we have this factory method then we can more easily enforce that the only way to create a new Trip instance in this project is to call the create_trip method.

Minor note: Because the list of trips is provided as a parameter rather than assumed to be the @trips instance variable, this method could actually be a class method. That is usually the convention for factory methods, actually.

I would also consider that providing the trip list, and having code to push the new trip into that list, is probably a violation of "separation of concerns". It's possible that we could want to create a trip and not put it into an array, which is not supported by the current version of this method.

trip = Trip.new(trip_data)
passenger.add_trip(trip)
trips << trip
driver.add_driven_trip(trip)
trip_list << trip
return trip
end

return trips
end

def find_passenger(id)
check_id(id)
return @passengers.find { |passenger| passenger.id == id }
end
def request_trip(user_id)
available_drivers = @drivers.select { |driver| driver.status == :AVAILABLE}
Copy link

Choose a reason for hiding this comment

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

This line would be a good candidate for being moved into its own "helper" method. Doing so would be in keeping with the "separation of concerns" concept discussed in POODR.


def inspect
return "#<#{self.class.name}:0x#{self.object_id.to_s(16)} \
#{trips.count} trips, \
#{drivers.count} drivers, \
#{passengers.count} passengers>"
end
passenger = find_passenger(user_id)
driver = available_drivers.first

trip_data = {
start_time: Time.now,
end_time: nil,
passenger: passenger,
driver: driver,
cost: nil,
rating: nil
}

private
trip_data[:driver].status = :UNAVAILABLE
new_trip = create_trip(trip_data, driver, passenger, @trips)

def check_id(id)
raise ArgumentError, "ID cannot be blank or less than zero. (got #{id})" if id.nil? || id <= 0
return new_trip
end

def inspect
return "#<#{self.class.name}:0x#{self.object_id.to_s(16)} \
#{trips.count} trips, \
#{drivers.count} drivers, \
#{passengers.count} passengers>"
end

private

def check_id(id)
raise ArgumentError, "ID cannot be blank or less than zero. (got #{id})" if id.nil? || id <= 0
end
end
end
end
19 changes: 19 additions & 0 deletions lib/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,24 @@ def initialize(input)
def add_trip(trip)
@trips << trip
end

def net_expenditures
# all_trips = @user.trips
array = []
@trips.each do |trip|
array << trip.cost
end
return array.sum

end

def total_time_spent
array = []
@trips.each do |trip|
array << trip.calculate_duration
end
return array.sum
end

end
end
Loading