-
Notifications
You must be signed in to change notification settings - Fork 48
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
Ports - Kasey #47
base: master
Are you sure you want to change the base?
Ports - Kasey #47
Conversation
Ride ShareWhat We're Looking For
This code certainly solves the problem at hand, and is for the most part well-organized. However, there are definitely some pieces that could be cleaned up, as I've tried to call out in the inline comments below. Action items for you before your next submission:
Beyond that, I'm not too concerned. Keep up the hard work! |
rideinfo = { | ||
DR0004: [{date: Time.new(2016, 2, 3, 0, 0, 0), | ||
cost: 5, | ||
rider_id: "RD0022", |
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 not a huge deal, but at Ada, our recommendation is that each key/value pair get a separate line, and that each level of a data structure gets a separate level of indentation. Applying those rules, this data structure would look like:
rideinfo = {
DR0004: [
{
date: Time.new(2016, 2, 3, 0, 0, 0),
cost: 5,
rider_id: "RD0022",
rating: 5
},
{
...
}
],
DR001: [
...
],
...
}
rating: 5}, | ||
{date: Time.new(2016, 2, 4, 0, 0, 0), | ||
cost: 10, | ||
rider_id: "RD0022", |
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 use of the Time
class here.
|
||
rideinfo.each do |k, v| | ||
puts v.length | ||
end |
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 just prints out the number of rides, not who was driving. This is especially confusing since your drivers are out of order (driver 4 comes first).
rideinfo.each do |k, v| | ||
summed = v.sum { |x| x[:cost] } | ||
puts "driver #{k} made $#{summed}" | ||
end |
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.
k
, v
and x
are not great variable names. As a newcomer to this code, I have no idea what these variables are supposed to contain. driver_id
, trips
, and trip
might be better choices.
As an engineer, a huge portion of your job is communicating your ideas to other engineers, and variable names are part of that. Remember, your code will be read many more times than it is written.
puts "driver #{highest} has the highest rating with #{holder[highest].round(2)}" | ||
end | ||
|
||
puts highestRating(rideinfo) |
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.
Your highestRating
method finishes with a puts
(line 81), which means it implicitly returns nil
. That means that line 84 will print out nothing but an empty line. The same thing happens with mostMoney
.
In general, we recommend you return values from methods, and puts
from whatever code calls that method.
def mostMoney(rideinfo) | ||
holder = rideinfo.map { |k, v| | ||
[k, v.sum { |x| x[:cost] }] | ||
}.to_h |
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 like that you've broken this work out into separate methods - good organization!
def highestRating(rideinfo) | ||
holder = rideinfo.map { |k, v| | ||
[k, (v.sum { |x| x[:rating] } / v.length.to_f)] | ||
}.to_h |
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 like that you're using the enumerable methods we learned about in class here. However, the fact that you're having to make this complicated array structure and then call .to_h
on the result indicates to me that the map pattern doesn't quite fit this problem.
Unfortunately Ruby doesn't have a builtin method to map a hash to a hash (which is what you're really looking for), but you might find this article interesting.
Test Grade! Hope this did the normal grade things! |
ride share
Congratulations! You're submitting your assignment.
Comprehension Questions
.map
? If so, when? If not, why, or when would be a good opportunity to use it?