-
Notifications
You must be signed in to change notification settings - Fork 28
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
base: master
Are you sure you want to change the base?
Changes from all commits
5df17ac
b39126c
98b2ad2
2eaa6f1
afca7c8
7b9b72d
4e1669a
ed69b8a
d12b0b5
72fef83
ecc9317
80efa5f
1d526c3
97fa9a1
e8af14f
5808c50
b99c259
d776175
ae7e9df
5c8c5b4
7310223
c090425
4a75404
48b1a91
9f6676c
240cbe6
fdeb16c
05bf3a9
9caae1d
2d296da
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 In that situation, we will have added an invalid trip to the 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this check is necessary for the In this case we have no division, so it should be fine to run all of the code when 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 So, my suggestion would be to remove the check about |
||
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 |
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] | ||||||||||
|
@@ -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 | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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:
|
||||||||||
end | ||||||||||
|
||||||||||
def inspect | ||||||||||
|
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 | ||
|
||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Minor note: Because the list of trips is provided as a parameter rather than assumed to be the 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} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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:
With that above code we have logic that makes
:UNAVAILABLE
the default status, but we aren't preventing other values from being provided.