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

Naheed & Dionisia's Ride Share Edges #16

Open
wants to merge 35 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
495c712
Wave 1 steps 1-3 complete
larachan15 Aug 27, 2018
b8718c2
fixed tests for wave 1 1-3
arangn Aug 28, 2018
3393aef
fixed trip.spec for start time < end time
arangn Aug 28, 2018
17ec25d
fixing indentation
larachan15 Aug 28, 2018
b088120
merging changes
larachan15 Aug 28, 2018
3d2cfac
wave 1.2 all tests pass
larachan15 Aug 28, 2018
3ea0722
start wave 2
arangn Aug 28, 2018
1416994
Wave 2 Drivers Class Complete
larachan15 Aug 28, 2018
c5ccfe8
Wave 2 Loading Drivers in progress
larachan15 Aug 28, 2018
ab8b9d8
minor changes to find_driver method
arangn Aug 29, 2018
0439271
fixed load_drivers
arangn Aug 29, 2018
f978aa3
actually fixed load_drivers
arangn Aug 29, 2018
ad732d7
removed unrelated tests
arangn Aug 29, 2018
1823c1d
Fixed fails, still have 6 errors to resolve for Wave 2
larachan15 Aug 29, 2018
efff9d6
debugging
arangn Aug 29, 2018
9c6f5cd
Some indentation cleanup
larachan15 Aug 29, 2018
2c6f4e7
code indentation cleaned up
larachan15 Aug 29, 2018
318e44d
merged changes
arangn Aug 29, 2018
25277b2
updated driver.spec
arangn Aug 29, 2018
080d205
most errors fixed
larachan15 Aug 29, 2018
afc89f9
fixed total_revenue spec
arangn Aug 29, 2018
9513989
fixed driver_spec
arangn Aug 29, 2018
bbd9fa0
debugging errors for driver_spec
arangn Aug 29, 2018
92b96c0
fixed total_revenue method in driver.rb
arangn Aug 29, 2018
fc3e0cd
wave 2 complete
larachan15 Aug 29, 2018
bded3e7
Update README.md
arangn Aug 29, 2018
abafafb
Update README.md
arangn Aug 29, 2018
7657914
implement wave 3, need tests
arangn Aug 30, 2018
3ec0d20
Merge branch 'master' of https://github.com/arangn/oo-ride-share
arangn Aug 30, 2018
04c7c54
Wave 3 first test fixed
larachan15 Aug 30, 2018
ff02f25
add test for assign_driver argument error
arangn Aug 30, 2018
a03ea70
Wave 3 complete with tests passing
larachan15 Aug 30, 2018
e4f838a
Attempt to do Wave 4. Test with error
larachan15 Sep 1, 2018
bbe2c4d
first driver selected method working
larachan15 Sep 6, 2018
09e67bf
Wave 4 complete with passing tests. Also fixed minor errors
larachan15 Sep 6, 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
6 changes: 3 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ We will do this by creating a `Driver` class which inherits from `User`. A `Dri

**Attribute**|**Description**
-----|-----
vehicle_id|The driver's Vehicle Identification Number (VIN Number), Each vehicle identification number should be a specific length of 17 to ensure it is a valid vehicle identification number
|The driver's Vehicle Identification Number (VIN Number), Each vehicle identification number should be a specific length of 17 to ensure it is a valid vehicle identification number
driven_trips | A list of trips the user has acted as a driver for.
status|Indicating availability, a driver's availability should be either `:AVAILABLE` or `:UNAVAILABLE`

Expand Down Expand Up @@ -208,7 +208,7 @@ net_expenditures|This method will **override** the cooresponding method in `User

**All the new methods above should have tests**

<!-- # Wave 3
# Wave 3

Our program needs a way to make new trips and appropriately assign a driver and user.

Expand Down Expand Up @@ -246,7 +246,7 @@ Your code from waves 1 & 2 should _ignore_ any in-progress trips. That is to say

You should also **add explicit tests** for this new situation. For example, what happens if you attempt to calculate the total money spent for a `User` with an in-progress trip, or the average hourly revenue of a `Driver` with an in-progress trip? -->

<!-- # Wave 4
# Wave 4

We want to evolve `TripDispatcher` so it assigns drivers in more intelligent ways. Every time we make a new trip, we want to pick drivers who haven't completed a trip in a long time, or who have never been assigned a trip.

Expand Down
82 changes: 82 additions & 0 deletions lib/driver.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
module RideShare
class Driver < User
attr_reader :vin
attr_accessor :driven_trips, :status

def initialize(input)
super(input)
if input[:vin].length != 17
raise ArgumentError, 'ID must be 17 characters'
end

# valid_staus = [:AVAILABLE, :UNAVAILABLE]
# if valid_staus.include?(@status)
# raise ArgumentError, 'Not a valid status'
# end

@vin = input[:vin]
@driven_trips = input[:driven_trips].nil? ? [] : input[:driven_trips]
@status = input[:status] == nil ? :AVAILABLE : input[:status]
end

def average_rating
total_ratings = 0
@driven_trips.each do |trip|
total_ratings += trip.rating

Choose a reason for hiding this comment

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

All these driver methods will fail for incomplete trips!

end

if @driven_trips.length == 0
average = 0
else
average = (total_ratings.to_f) / @driven_trips.length
end

return average
end


# def average_rating
# ratings = @driven_trips.find { |trip| trip.rating } #FIX ME PLEASE
# # binding.pry
# average = ratings.sum / @driven_trips.length
# total_ratings = 0
# @trips.each do |trip|
# total_ratings += trip.rating
# end
#
# if trips.length == 0
# average = 0
# else
# average = (total_ratings.to_f) / trips.length
# end
# return average
# end

def add_driven_trip(trip)
if trip.class != Trip
raise ArgumentError.new("Can only add trip instance to driven_trips array")
end
@driven_trips << trip
return @driven_trips
end

def total_revenue
revenue = 0.0
income = 0.0
@driven_trips.each do |trip|
if trip.cost == nil
raise ArgumentError, "Trip still in progress, no revenue"
else
revenue = (trip.cost - 1.65) * 0.8
income += revenue

Choose a reason for hiding this comment

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

This code would be a little more readable if the two magic numbers (0.8 and 1.65) were stored in constants, maybe something like DRIVER_CUT and FEE.

end
end
return income.round(2)
end

def net_expenditures
return super - total_revenue
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
26 changes: 22 additions & 4 deletions lib/trip.rb
Original file line number Diff line number Diff line change
@@ -1,20 +1,38 @@
require 'csv'
require 'pry'


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]
@passenger = input[:passenger]
@start_time = input[:start_time]
@end_time = input[:end_time]
@cost = input[:cost]
@rating = input[:rating]
@cost = input[:cost].to_f
@rating = input[:rating].nil? ? nil : input[:rating]
@driver = input[:driver]

if @rating > 5 || @rating < 1
if @rating != nil && (@rating > 5 || @rating < 1)
raise ArgumentError.new("Invalid rating #{@rating}")
end

if @end_time != nil && (@start_time > @end_time)
raise ArgumentError, "Invalid end time"
end

end

# def retrieve_driver(driver)
# driver = Driver.find(line[0])
# end

def trip_duration
duration = @end_time - @start_time
duration_in_seconds = duration.to_i
return duration_in_seconds
end

def inspect
Expand Down
152 changes: 118 additions & 34 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 @@ -25,52 +29,132 @@ def load_users(filename)

users << User.new(input_data)
end

return users
end

def load_drivers(filename)
driver_data = CSV.open(filename, 'r', headers: true, header_converters: :symbol)

return driver_data.map do |each_driver|
user = find_passenger(each_driver[:id].to_i)
# binding.pry
driver = {
id: each_driver[:id].to_i,
trips: user.trips,
vin: each_driver[:vin],
status: each_driver[:status].to_sym,
name: user.name,
phone: user.phone_number
}

Driver.new(driver)
end

end

def load_trips(filename)
trips = []
trip_data = CSV.open(filename, 'r', headers: true,
header_converters: :symbol)

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
}
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)
raw_trip[:start_time] = Time.parse(raw_trip[:start_time])
raw_trip[:end_time] = Time.parse(raw_trip[:end_time])
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,
driver: driver
}

trip = Trip.new(parsed_trip)
passenger.add_trip(trip)
driver.add_driven_trip(trip)
trips << trip

end
return trips
end

trip = Trip.new(parsed_trip)
passenger.add_trip(trip)
trips << trip
def find_passenger(id)
check_id(id)
return @passengers.find { |passenger| passenger.id == id }
end

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

def find_passenger(id)
check_id(id)
return @passengers.find { |passenger| passenger.id == id }
end
def inspect
return "#<#{self.class.name}:0x#{self.object_id.to_s(16)} \
#{trips.count} trips, \
#{drivers.count} drivers, \
#{passengers.count} passengers>"
end

def inspect
return "#<#{self.class.name}:0x#{self.object_id.to_s(16)} \
#{trips.count} trips, \
#{drivers.count} drivers, \
#{passengers.count} passengers>"
end
def check_id(id)
raise ArgumentError, "ID cannot be blank or less than zero. (got #{id})" if id.nil? || id <= 0
end

def request_trip(user_id)
chosen_driver = assign_driver(user_id)
passenger = find_passenger(user_id)

private
valid_trip_id = [1..1000].select
@trips.each do |trip|
if trip.id == valid_trip_id
raise ArgumentError, "ID already exists"

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 code will do what you want. [1..1000].select returns a thing called an enumerator, which is basically a loop with no block. Aside from not being a reasonable value for an ID, this will never be equal to your integer trip ID, so the argument error will never be raised.

Moreover, even if you had said .sample instead of .select, raising an error because you happened to generate a bad random number is not a great behavior. Instead, you should retry selecting a number, or even better choose the number without randomness:

highest_current_id = @trips.map { |t| t.id }.max
new_trip_id = highest_current_id + 1

end
end

def check_id(id)
raise ArgumentError, "ID cannot be blank or less than zero. (got #{id})" if id.nil? || id <= 0
trip = RideShare::Trip.new(id: valid_trip_id, driver: chosen_driver, passenger: passenger, start_time: Time.now, end_time: nil, cost: nil, rating: nil)

chosen_driver.add_driven_trip(trip)
passenger.add_trip(trip)
@trips << trip

Choose a reason for hiding this comment

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

You don't mark the driver as unavailable!

return trip
end

def assign_driver(passenger_id)
# iterates through the drivers to select and return available drivers and drivers who are not driving themselves
available_drivers = @drivers.select do |driver|
if driver.status == :AVAILABLE && driver.id != passenger_id

Choose a reason for hiding this comment

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

I like that you broke this logic out as a separate method! Good instincts.

driver
end
end

# iterates through the available_drivers to see if there is a driver that has not given any trips.
chosen_driver = available_drivers.find do |driver|
driver.driven_trips.empty?
end

Choose a reason for hiding this comment

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

Good use of enumerables throughout this method!


# if there hasn't been a driver that hasn't given any trips, then this iterates through available drivers and selects the driver who's trip ended the longest time ago.
if chosen_driver.nil?
furthest_date = Time.now
available_drivers.each do |driver|
# This assumes that driven_trips are in chronological order
end_time = driver.driven_trips.last.end_time
if end_time < furthest_date

Choose a reason for hiding this comment

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

That might be a big assumption.

end_time = furthest_date # sets new end time
chosen_driver = driver # assigns that driver as chosen driver
end
end
end

# if there are no available drivers
if chosen_driver.nil?
raise ArgumentError, "No drivers available"
end

# if there is an available driver, changes their status
chosen_driver.status = :UNAVAILABLE
return chosen_driver
end

Choose a reason for hiding this comment

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

Aha, here's where you set the status. I would probably not do this yet - what if something goes wrong in trying to create the trip? Then the driver would be unavailable but not actually have an incomplete trip.

end
end
end
25 changes: 25 additions & 0 deletions lib/user.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
require 'pry'
module RideShare
class User
attr_reader :id, :name, :phone_number, :trips
Expand All @@ -16,5 +17,29 @@ def initialize(input)
def add_trip(trip)
@trips << trip
end

def net_expenditures
cost_array = []
@trips.each do |trip|
if trip.cost == nil
raise ArgumentError, "Trip is in progress, no cost"

Choose a reason for hiding this comment

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

I don't know that raising an ArgumentError is the right course of action here - you could imagine a user wanting to look up their net expenditures while on a trip, for example. Instead I would probably omit incomplete trips:

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

end
cost = trip.cost
cost_array << cost
end
return cost_array.sum
end

def total_time_spent
time_array = []
@trips.each do |trip|
if trip.end_time == nil
raise ArgumentError, "Trip is in progress, no end time"
end
time_per_trip = trip.end_time.to_i - trip.start_time.to_i
time_array << time_per_trip
end
return time_array.sum
end
end
end
Loading