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

Pipes - Kate Evans-Spitzer, Iuliia Chikulaeva, Anders Chen - Ride Share #6

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

Conversation

Guribot
Copy link

@Guribot Guribot commented Oct 2, 2017

Rideshare-Rails

Congratulations! You're submitting your assignment! These comprehension questions should be answered by both partners together, not by a single teammate.

Comprehension Questions

Question Answer
Describe the types of entity relationships you set up in your project and why you set up the relationships that way A Passenger has many trips, and a Driver has many trips. A trip has exactly one Passenger and one Driver. We felt that Passenger and Driver had no relationship outside of any given trip.
Describe the role of model validations in your application For Passenger and Driver, we validate the presence of Name, VIN, and phone number. For Trip, since our website automatically assigns the highest-rated available driver, we validate that the driver is not already booked on that day. Since math is performed on both the cost and the ratings, we validate numericality of both of those. We also validate the presence of all input.
How did your team break up the work to be done? Initially we primarily broke work out by class, but came together to work on concepts that were new to this project. Later on, we switched between divvying up features to work on individually, and coming together to work through anything we were having trouble with.
What features did you choose to prioritize in your project, and what features, if any, did you have to set aside to meet the deadline? We were able to implement all of the required features, as well as a few that we came up with on our own. However, we received a suggestion from Dan to prevent a user from creating a new ride until they had rated all their previous rides, that we were interested in but ran out of time to implement.
What was one thing that your team collectively gained more clarity on after completing this assignment? Lots of things - trello/kanban, working with multiple models, working within the model file, nested routes, view helpers, git branching, etc
What is your Trello URL? https://trello.com/b/R4e5PbtV/ride-share
What is the Heroku URL of your deployed application? http://cool-team-ride-share.herokuapp.com/

@Hamled
Copy link

Hamled commented Oct 14, 2017

Rideshare-Rails

What We're Looking For

Feature Feedback
Baseline
Appropriate Git Usage with no extraneous files checked in and both partners contributing
Answered comprehension questions
Uses named routes (like _path)
RESTful routes utilized
Rideshare Rails Specific Content
Table relationships
Validation rules for Models
Business logic is in the models
Database is seeded from the CSV files
Trello board is created and utilized in project management ?? I cannot check this yet, as I do not have permission to access the Trello board.
Postgres database is used
Heroku instance is online
The app is styled to create an attractive user interface
Overall Looks good! I think y'all have done a good job of keeping complex logic within your Model classes. I especially like the use of a custom validation method on the Trip model.


root 'main#index'

get '/drivers/by_rating', to: 'drivers#by_rating', as: 'drivers_by_rating'
Copy link

Choose a reason for hiding this comment

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

This route could be nested within the resources :drivers directive:

resources :drivers do
  get 'by_rating', on: :collection
end

The on: :collection part tells Rails that this is a route which applies to the whole collection of Drivers, rather than a single member of that collection (which would generate a route that included :driver_id). See the Rails Routing Guide for details.

resources :passengers do
resources :trips, only: [:new]
end
resources :trips, except: [:index]
Copy link

Choose a reason for hiding this comment

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

If we have the new action for TripsController nested under resources :passengers then we should probably include it in the except on this line, so that we don't end up having two routes which go to Trips#new.

get '/drivers/by_rating', to: 'drivers#by_rating', as: 'drivers_by_rating'
resources :drivers
resources :passengers do
resources :trips, only: [:new]
Copy link

Choose a reason for hiding this comment

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

If we add the create and index actions to this only: list (which is reasonable since new and create are generally paired, and index makes sense for listing all of the trips for a given passenger), then we can actually simplify this a bit by using shallow nesting.

@@ -0,0 +1,4 @@
class AddDateToTrips < ActiveRecord::Migration[5.1]
def change
end
Copy link

Choose a reason for hiding this comment

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

This migration is empty, so it's not getting us anything and we could probably remove it?

Based on the name it seems like it was probably intended to add a date column to the trips table, however the original migration to create that table already includes a date column. As a reminder, we should not go back and modify our existing migrations if we've already committed them into Git (and certainly not after deploying them to Heroku).

belongs_to :passenger
# driver_id, passenger_id, date, rating, cost
validates :driver_id, presence: {message: "Please select a driver"}
validates :passenger_id, presence: {message: "Please select a passenger"}
Copy link

Choose a reason for hiding this comment

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

As it turns out, since Rails 5.0, when we establish a belongs_to association on our model, as we're doing here for driver and passenger, Rails will automatically create this presence validation.

numericality: {only_integer: true, message: "Ride cost should be in cents"}
validate :driver_available_on_date

def driver_available_on_date
Copy link

Choose a reason for hiding this comment

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

Great use of a custom validation method!

validates :name, presence: {message: "Please enter a name"}
validates :vin, presence: {message: "Please enter a VIN"}

def average_rating
Copy link

Choose a reason for hiding this comment

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

There are a few ways that we can use Enumerable methods, or even better ActiveRecord calculation methods, to simplify the implementation of this method. Here are two options:

Enumerable style:

def average_rating
  trips.map(&:rating).compact.sum / trips.length
end

ActiveRecord style:

def average_rating
  trips.average(:rating)
end

The second option is my preferred style because it is both simpler code and more efficient. This version actually moves the calculation of the average into SQL code, so the Postgres database ends up doing the average calculation for us. In that case it is significantly faster when we have a very large amount because it doesn't need to be sent to Rails first.

return avg / count
end

def total_earnings
Copy link

Choose a reason for hiding this comment

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

Similar to Driver#average_rating we could use ActiveRecord calculation methods to simplify this method:

def total_earnings
  trips.sum(:cost) * 0.85
end

end

def trips_by_date
return trips.order(:date).reverse
Copy link

Choose a reason for hiding this comment

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

ActiveRecord actually gives us a handy option for telling Postgres to use reverse sorting order when we call the order method. The above code is equivalent to:

return trips.order(date: :asc).reverse

which can be simplified to:

return trips.order(date: :desc)

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.

4 participants