Skip to content

Time - Sharon and Jocelyn#1

Open
wangjoc wants to merge 20 commits into
Ada-C13:masterfrom
wangjoc:master
Open

Time - Sharon and Jocelyn#1
wangjoc wants to merge 20 commits into
Ada-C13:masterfrom
wangjoc:master

Conversation

@wangjoc
Copy link
Copy Markdown

@wangjoc wangjoc commented Feb 27, 2020

Assignment Submission: OO Ride Share

Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.

Reflection

Question Answer
How did getting up to speed on the existing codebase go? If it went well, what worked? If it didn't go well, what will you do differently next time? For this project, we read through the code and discussed what we thought each part was doing. We then started to just modify the code and test as we proceeded through the waves. In the future, it would likely have to do a lot less rework if we drew a graph before we started working directly on the code, since it would give us a better picture of how everything is linked.
What inheritance relations exist between classes? CsvRecord is the parent class for Trip, Passenger, and driver so they all have access to the attributes and methods that CsvRecord has. More specifically, classes that inherited from CsvRecord used it’s method to load in data from the CSV and parse it out.
What composition relations exist between classes? TripDispatcher has composition relation with drivers, passengers, and trips. On the other hand, Passenger and Driver to Trips have a one to many relationship. Each Trip will have one Passenger and one Driver per each, although there is no limit to how many Passengers, Drivers, and Trips there are in total.
Describe a decision you had to make when working on this project. What options were you considering? What helped you make your final decision? We had to determine what to do to track in-progress trips. We at first considered creating a new class for In-progress Trips that would inherit most attributes from the existing Trip class. However, we ultimately steered away from that since it would add in a lot more complexity due to the increase of managing a new class, as well as converting it from an In-progress Trip to a Completed Trip later on. We then tried to create a new attribute for the Trip class that was for @completed_trip, which would simply save true or false. When we first created it, we set the default instantiation value to be false and set up logic that would flip it to true if end_time, cost, and rating were all present. While this allowed us to pass the tests, we were triggering a lot of warning messages from the older tests. Rather than going back and actually modifying the old test data, we removed the attribute from being one of the arguments accepted in the initialize method. This allowed us to retain attributes for our test purposes, but get rid of the warning messages.
Give an example of a template method that you implemented for this assignment One of the places we used a template method was for the request trip method in Wave 3. We found it easier to break out the problem statement into individual components and tackle them one at a time. As we worked on each one, we would develop tests for it first and keep tinkering with the code until it worked. Since we had already segregated the code into different components, we kept it that way when combining the parts together to request a trip. This also made it easier to identify which part of the code was actually causing the issue.
Give an example of a nominal test that you wrote for this assignment We have “add trips to driver” test. It will test that the driver returned from the first_available_driver method is a driver that is :AVAILABLE at that time, and that their status will change from :AVAILABLE to :UNAVAILABLE. It will also check that the number of total trips by that driver increased by 1.
Give an example of an edge case test that you wrote for this assignment We wrote a test to raise an ArgumentError if we don’t have enough drivers. We did this by purposefully using up the other two available drivers first so that it would raise an error the third time a trip was requested.
What is a concept that you gained more clarity on as you worked on this assignment The project definitely helped us better understand Inheritance relations and Composition since we were actually working on code involving those concepts. We also learned the value of sketching out all the relationships before charging into the details.

@beccaelenzil
Copy link
Copy Markdown

OO Ride Share

Major Learning Goals/Code Review

Criteria yes/no, and optionally any details/lines of code to reference
The code demonstrates individual learning about Time and the responsibility of Trip.from_csv, and uses Time.parse in Trip.from_csv ✔️
The code demonstrates breaking out complex logic in helper methods, such as making a helper method in Trip to calculate duration ✔️
There are tests for the nominal cases for the Passenger#net_expenditures and Passenger#total_time_spent ✔️these should be in separate it blocks, even though they are quite similar.
There is at least one edge case test for either Passenger#net_expenditures or Passenger#total_time_spent testing if the passenger has no trips ✔️
Practices inheritence. Driver inherits from CsvRecord, and implements from_csv ✔️
Employs problem-solving and implements Driver#average_rating and Driver#total_revenue ✔️
Implements the TripDispatcher#request_trip, which creates an instance of Trip with a driver and passenger, adds the new trip to @trips, and changes the status of the driver ✔️
Practices composition. In TripDispatcher#request_trip, the driver gets connected to the new trip, the passenger gets connected to the new trip ✔️
Practices git with at least 10 small commits and meaningful commit messages ✔️

Testing Requirements

Testing Requirement yes/no
There is reasonable test coverage for wave 1, and all wave 1 tests pass ✔️
There is reasonable test coverage for wave 2, and all wave 2 tests pass ✔️
Wave 3: Tests in wave 1 and wave 2 explicitly test that only completed trips should be calculated (and ignore in-progress trips) methods such as average_rating should no include a test for drivers with incomplete trips
There is reasonable test coverage for TripDispatcher#request_trip, and all tests pass ✔️

Overall Feedback

Overall Feedback Criteria yes/no
Green (Meets/Exceeds Standards) 8+ in Code Review && 3+ in Functional Requirements ✔️
Yellow (Approaches Standards) 6+ in Code Review && 2+ in Functional Requirements
Red (Not at Standard) 0-5 in Code Review or 0,1 in Functional Reqs, or assignment is breaking/doesn’t run with less than 5 minutes of debugging

Code Style Bonus Awards

Was the code particularly impressive in code style for any of these reasons (or more...?)

Quality Yes?
Elegant/Clever
Descriptive/Readable
Concise
Logical/Organized

Copy link
Copy Markdown

@beccaelenzil beccaelenzil left a comment

Choose a reason for hiding this comment

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

Great work on this assignment. I've left a few inline comment for you to review, mostly giving you kudos for your use of enumerable methods :). It is clear that the learning goals around TDD, object composition, and inheritance were met. Keep up the hard work!

Comment thread lib/passenger.rb

def total_time_spent
return 0 if @trips == []
@trips.map {|trip| trip.duration() }.inject(:+)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice use of enumerable methods.

Comment thread test/passenger_test.rb

describe "net_expenditures" do
# You add tests for the net_expenditures method
describe "net_expenditures and total time spent" do
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I know these methods are quite similar, but they still should be tested in different it blocks.

Comment thread lib/driver.rb
@trips << trip
end

def average_rating
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice use of enumerable methods, again :)

Comment thread lib/trip_dispatcher.rb
end


def request_trip(passenger_id)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great use of helper methods.

Comment thread lib/trip_dispatcher.rb
counter = nil
max_time = Time.now()
self.drivers.each_with_index do |driver, index|
if driver.status == :AVAILABLE
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It seems that you implemented some of the optionals for wave 3 (nice work!), but I don't see tests corresponding to these rules. In addition, the mix of a post-fix conditional and a standard conditional, nested in another conditional, makes this a little tricky to read. Consider breaking out into helper methods to increase readability.

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