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

Queues - Anna Barklund - ride-share-two #44

Open
wants to merge 33 commits into
base: master
Choose a base branch
from
Open

Conversation

amb54
Copy link

@amb54 amb54 commented Mar 13, 2017

Ride Share

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Describe a design decision you had to make when working on this project. What options were you considering? What helped you make your final decision? There were three classes to create that needed to communicate with each other. The csv file for the trip class showed information of id related to all three classes. My decision was to have the Trip class as a connector for the three classes.
Describe a concept that you gained more clarity on as you worked on this assignment. The idea how the classes are communicating with each other.
Describe a nominal test that you wrote for this assignment. For the trip class method self.all I tested that all 600 lines in the csv file created instances.
Describe an edge case test that you wrote for this assignment. Testing of the accepted phone numbers in the Rider class initialize method. They must be in the format NPA-NXX-xxxx. N cannot be 0 or 1. XX cannot be 11
How do you feel you did in writing pseudocode first, then writing the tests and then the code? I like it. It makes it more clear. I get a simple overview of the project, everything is not spinning around in my head. I get more focused. I really like the tests : )

amb54 added 30 commits March 6, 2017 16:21
…ated two test csv-files for trip trip_spec_true.csv and trip_spec_false.csv.
…aned up comments for describe and it. Updated spec_helper.rb
…port files with headers. Trip#self.all_trips_for_driver tested and passed.
…e sections should be commented out when running the tests. Changed Driver#self.find to have a begin/rescue for cases when an id is not found. Subsequently changed the driver_spec for this method. Cleaned up trip_spec with respect to let()
… the rider_id isnot found. Subsequently updated the related test. Updated Rider#previous_trips tofunction correctly with uniq. Added tests for #previous_trips.
…ed test cases for the methods initialize and self.all in class Driver with subsequent updating these methods topass the tests.
…any ArgumentErrors for the Trip methods initialize and self.all. Updated the Trip code to pass the tests. Updated support/trip_spec_false.csv with 20 trips and made 6 of them invalid. Updated the tests for the Trip method self.all
…od initialize. changed the code to pass the tests. Added a private method self.invalide_phone_num? to check if a phonenumber is valid according to North American Numbering Plan.
…escribe #self.find_all_trips_for_driver do in trip_spec.rb
…scribe #self.find_all_trips_for_rider do in trip_spec.rb
@droberts-sea
Copy link

Ride Share

What We're Looking For

Feature Feedback
Baseline
Used Git Regularly yes - good work!
Answer comprehension questions yes
Driver
Uses the all method in the find method yes
Has appropriate edge-case tests for each method in the class yes
Created a method that uses a method from the Trip object to retrieve the list of trips yes
Created a method that uses the internal trips list to calculate the average rating yes
Rider
Uses the all method in the find method yes
Has appropriate edge-case tests for each method in the class yes
Created a method that uses a method from the Trip object to retrieve the list of trips yes
Created a method that uses the internal trips method to retrieve the associated drivers yes
Trip
Reads the CSV file in the all method yes
Has appropriate edge-case tests for each method in the class yes
Created a method that uses a method from the Driver to retrieve the associated driver instance yes
Created a method that uses a method from the Rider to retrieve the associated rider instance yes
Created a method to retrieve all trips by driver id yes
Created a method to retrieve all trips by rider id yes
Overall

Great work overall! This code is clean and readable, your tests cover everything I'm looking for and your git habits appear strong. Keep up the good work.

end

it "Test of calculation of the average of the rating with number of rides == 0 " do
driver100 = RideSharing::Driver.find(100)

Choose a reason for hiding this comment

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

Because you defined driver100 in the let block above, you don't need to re-find it here.

found_driver = self.all.select { |driver| driver.id == driver_id}
begin
raise ArgumentError.new("Id number #{driver_id} does not exist") if found_driver == []
rescue ArgumentError => exception

Choose a reason for hiding this comment

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

Typically you don't rescue an exception that you yourself raise. Instead, raise-ing an exception is used to communicate to whoever called this method that there was a problem, and rescue is used if some method raises an exception you know how to handle.

Choose a reason for hiding this comment

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

In fact, you don't really even need to puts anything here - returning nil is sufficient to let the caller know that there was no Driver with that ID.

return list_of_trips.map { |trip| trip.rating}.sum.to_f/ list_of_trips.length
else
puts "#{@name} with id##{@id} has not yet taken any trips.\nHence the average rating cannot be calcualted"
return Float::NAN

Choose a reason for hiding this comment

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

As above, you don't need to puts here - returning NAN is sufficient.

end

def previous_drivers
drivers = list_of_trips.map { |trip| trip.find_driver}.delete_if {|driver| driver == nil}.uniq { |driver| driver.id}

Choose a reason for hiding this comment

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

Good use of enumerables here!

All of these are clear in what they do, but it still works out to a pretty long line. Something like

drivers = list_of_trips.map { |trip| trip.find_driver}
drivers.delete_if! {|driver| driver == nil}
drivers.uniq! { |driver| driver.id}

might be a little easier to read.

return drivers
end

private

Choose a reason for hiding this comment

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

Nice use of private to hide methods that aren't part of your API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants