-
Notifications
You must be signed in to change notification settings - Fork 29
Space - Lak & Kate #2
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
base: master
Are you sure you want to change the base?
Conversation
OO Ride ShareMajor Learning Goals/Code Review
Testing Requirements
Overall Feedback
Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
kaidamasaki
left a comment
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.
Good job! Here are a few small things you can do to clean up your code and make it more concise.
| { | ||
| // Use IntelliSense to learn about possible attributes. | ||
| // Hover to view descriptions of existing attributes. | ||
| // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387 | ||
| "version": "0.2.0", | ||
| "configurations": [ | ||
| { | ||
| "name": "Debug Local File", | ||
| "type": "Ruby", | ||
| "request": "launch", | ||
| "program": "${file}" | ||
| } | ||
| ] | ||
| } No newline at end of file |
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.
You should probably globally ignore your .vscode folder.
| source 'http://rubygems.org' | ||
|
|
||
| ruby '2.5.5' | ||
| ruby '2.7.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.
We're using Ruby 2.6.5 for this current cohort:
| ruby '2.7.0' | |
| ruby '2.7.0' |
| def initialize( | ||
| id:, | ||
| name:, | ||
| vin:, | ||
| status: :AVAILABLE, | ||
| trips: nil | ||
| ) |
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.
If you indent your argument list it makes it easier to distinguish from the method body.
| def initialize( | |
| id:, | |
| name:, | |
| vin:, | |
| status: :AVAILABLE, | |
| trips: nil | |
| ) | |
| def initialize( | |
| id:, | |
| name:, | |
| vin:, | |
| status: :AVAILABLE, | |
| trips: nil | |
| ) |
|
|
||
| #Raise argument for vin | ||
| if @vin.length != 17 | ||
| raise ArgumentError.new("Invalid VIN number, needs to be 17 in length.") |
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.
It's helpful to include the bad argument in the message for your ArgumentError:
| raise ArgumentError.new("Invalid VIN number, needs to be 17 in length.") | |
| raise ArgumentError.new("Invalid VIN number: #{@vin}, needs to be 17 in length.") |
| #Wave 2: Adding trips to the driver | ||
| def add_trip(trip) | ||
| @trips << trip | ||
| #self.trip << trip |
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.
It's considered best practice to remove commented out code:
| #self.trip << trip |
| end | ||
|
|
||
| #Wave 3: Find driver_by_status | ||
| def find_driver_status(status) |
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.
The name find_driver_status is misleading since it sounds like you are looking for the status of a single driver.
| def find_driver_status(status) | |
| def find_driver_by_status(status) |
| #Wave 3: request_trip method | ||
| #Wave 3: added new instance of trip | ||
| def request_trip(passenger_id) | ||
| available_driver = @drivers.select {|driver| driver.status == :AVAILABLE}.first |
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.
Using find here is a little clearer (since it's easy to miss the .first at the end of the line):
| available_driver = @drivers.select {|driver| driver.status == :AVAILABLE}.first | |
| available_driver = @drivers.find {|driver| driver.status == :AVAILABLE} |
| @trip_data[:start_time] = Time.parse("2018-12-27 02:00:00 -0800") | ||
| @trip_data[:end_time] = Time.parse("2018-12-27 01:00:00 -0800") |
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.
In your tests I'd recommend directly constructing objects instead of parsing strings. Also, when using a Time or Date there are some helpful factory methods: Time.now and Date.today.
| @trip_data[:start_time] = Time.parse("2018-12-27 02:00:00 -0800") | |
| @trip_data[:end_time] = Time.parse("2018-12-27 01:00:00 -0800") | |
| @trip_data[:start_time] = Time.new(2018, 12, 27, 2) | |
| @trip_data[:end_time] = Time.new(2018, 12, 27, 1) |
Assignment Submission: OO Ride Share
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection