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

Alison's Ride Share #12

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

Alison's Ride Share #12

wants to merge 18 commits into from

Conversation

AlisonZ
Copy link

@AlisonZ AlisonZ commented Mar 11, 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? A major design decision was how to have the pieces talk to each other and where to handle the "heavy lifting"(??) I chose for the driver and rider class to mostly call other methods in the Trip class and the Trip class did most of the work.
Describe a concept that you gained more clarity on as you worked on this assignment. Class methods vs. instance methods!!!!!!!!
Describe a nominal test that you wrote for this assignment. In testing the find all trips method I tested that driver 16 returned all 6 trips that were listed for that driver in the data file.
Describe an edge case test that you wrote for this assignment. I tested a driver that didn't exist in the data file returned an argument error warning of invalid driver.
How do you feel you did in writing pseudocode first, then writing the tests and then the code? I love pseudocode and don't understand how anyone codes without doing it. And I am becoming a huge fan of writing tests first then code. I find my code is so much clearer by the time I get to writing it.

Copy link

@CheezItMan CheezItMan left a comment

Choose a reason for hiding this comment

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

Just a few comments




it "All elements in the Driver instance match the data in the CSV file" do

Choose a reason for hiding this comment

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

Since you're not testing all elements here, I would either make a loop that tests them all or at least change the title here.

end

it "Raises an Error for an incorrect " do
proc { RideShare::Driver.find(116) }.must_raise ArgumentError

Choose a reason for hiding this comment

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

I would suggest that here, you should just return nil if they try to find a Driver that doesn't exist.

def self.all
riders = []

CSV.open("support/riders.csv").each do |line|

Choose a reason for hiding this comment

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

Should be tabbed over!

@CheezItMan
Copy link

Ride Share

What We're Looking For

Feature Feedback
Baseline
Used Git Regularly I think you should be breaking up things into more "atomic" commits. You've got a lot in the commits I saw.
Answer comprehension questions Nice thinking with the Trip class. It's good to hear your growing confidence with pseudo-code & testing!
Driver
Uses the all method in the find method Check, but could you use the .find enumerable method instead of .each
Has appropriate edge-case tests for each method in the class Well done
Created a method that uses a method from the Trip object to retrieve the list of trips Check
Created a method that uses the internal trips list to calculate the average rating Check
Rider
Uses the all method in the find method Check, although you could use a different than enumerable to find a Rider.
Has appropriate edge-case tests for each method in the class Check
Created a method that uses a method from the Trip object to retrieve the list of trips Check
Created a method that uses the internal trips method to retrieve the associated drivers Check
Trip
Reads the CSV file in the all method Well done
Has appropriate edge-case tests for each method in the class What about tests for Trip.find?
Created a method that uses a method from the Driver to retrieve the associated driver instance Check
Created a method that uses a method from the Rider to retrieve the associated rider instance Check
Created a method to retrieve all trips by driver id Check
Created a method to retrieve all trips by rider id Check

Summary

Nicely done, it does seem like you missed a test for Trip.find, but you've shown a lot of growth here. Another question, are there requirements on IDs? Do they have to be positive Integers?

it "Every item in the array is an instance of driver" do
@previous_drivers.each do |driver|
driver.must_be_instance_of RideShare::Driver
end

Choose a reason for hiding this comment

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

You should also make sure it's the correct drivers.

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