-
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?
Conversation
…responding spec files
…s, add_driven_trips, average_rating
… that mmade sense :)
…p#initialize for TripDispatcher#request_trip method to work
…l they passed. On to Wave 3.
…ethod and added test for this
Ride ShareWhat We're Looking For
|
|
||
def calculate_duration | ||
duration = @end_time - @start_time | ||
return duration |
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.
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 |
@status = input[:status] | ||
|
||
unless @status == :AVAILABLE | ||
@status == :UNAVAILABLE |
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:
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.
@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 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 | ||
|
||
if @driven_trips.length == 0 | ||
average = 0 |
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.
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.
end | ||
|
||
if @driven_trips.length == 0 | ||
total = 0 |
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.
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
.
expect(@driver.id).must_be_kind_of Integer | ||
expect(@driver.name).must_be_kind_of String | ||
expect(@driver.vin).must_be_kind_of String | ||
expect(@driver.status).must_be_kind_of Symbol |
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.
Very minor: This test could be further dried up:
it "is set up for specific attributes and data types" do
{id: Integer, name: String, vin: String, status: Symbol, driven_trips: Array}.each do |prop, type|
expect(@driver).must_respond_to prop
expect(@driver.send(prop)).must_be_kind_of type
end
end
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 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.
|
||
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 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.
end | ||
|
||
it "throws an argument error for a bad ID" do | ||
expect { @dispatcher.find_driver(0) }.must_raise ArgumentError |
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.
Minor comment: I think the correct behavior here would be for TripDispatcher#find_driver
to return nil
when no driver was found with the given ID.
The reasoning for this is that technically it's not an invalid argument -- the ID was actually an integer, it just didn't happen to match anything in our data set. Another reason for preferring to return nil
is because Rails (which the all
and find
methods in these projects are based upon) also works that way.
|
||
it "selects first :AVAILABLE driver" do | ||
trip = @dispatcher.request_trip(1) | ||
expect(trip.driver.id).must_equal 5 |
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 is a solid test! We could expand on it by requesting a second trip after the first one, and then verifying that we got the next available driver (and not driver #5 a second time).
OO Ride Share
Congratulations! You're submitting your assignment!
Comprehension Questions
User
andDriver