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

Katricia/Karis - Edges - RideShareRails #18

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

Conversation

kimj42
Copy link

@kimj42 kimj42 commented Oct 6, 2018

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 Driver has zero or many Trips and Passenger has 0 or many Trips. Trips has foreign keys for driver_id and passenger_id.
Describe the role of model validations in your application For Driver, it requires the presence of name and vin. Vin has to be unique in 17 characters. For Trip, it requires the name and the phone number to exist for the new form to be submitted.
How did your team break up the work to be done? We broke it up by passenger and driver. We decided to work on setting up the initial baseline together. We also worked on heroku as well as css together.
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 decided to prioritize the user stories for passenger and driver. We set aside the css since that was the last requirement.
What was one thing that your team collectively gained more clarity on after completing this assignment? We both gained clarity on how to use branches and merge. We both took advantage of merging and branching.
What is your Trello board URL? https://trello.com/b/EwrDMSzw/katricia-karis
What is the Heroku URL of your deployed application?
https://katricia-karis-rideshare.herokuapp.com/
What are two discussion points that you and your pair discussed when giving/receiving feedback from each other that you would be willing to share? The division of project task was nicely done and fluid. We both appreciated that we listened to our ideas.

kimj42 and others added 30 commits October 1, 2018 16:14
… doesnt show the driver or new passenger id yet)
Merge katriciabranch and karis branch
krsmith7 and others added 28 commits October 5, 2018 14:27
Add form partial for error handling
Correct driver total_earnings method
@droberts-sea
Copy link

Rideshare Rails

What We're Looking For

Feature Feedback
Baseline
Appropriate git usage with no extraneous files checked in and both partners contributing yes
Answered comprehension questions yes
Uses named routes (like _path) yes
RESTful routes utilized some extra routes defined
Project Requirements
Table relationships yes
Validation rules for Models yes
Business logic is in the models mostly - see inline
Database is seeded from the CSV files yes
Trello board is created and utilized in project management yes
Heroku instance is online yes
The app is styled to create an attractive user interface yes
Overall Good work overall! This site's code is well-written and clear. You make good use of validations and relations, and most of your business logic is in the model - it's clear to me that the learning goals for this week were met. As with any project of this size I've left a number of nitpicks, but in general I am happy with what I see here. Keep up the hard work!


nav li :visited {
color: #d08b2536;
}

Choose a reason for hiding this comment

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

This doesn't style unvisited links (if the user is visiting your site for the first time), which are really hard to read.


def average_rating
return trips.sum(&:rating).to_f/ trips.length
end

Choose a reason for hiding this comment

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

What happens if there are no trips associated with a driver? Then total would be 0.0 and trip_count is 0. Ruby evaluates 0.0 / 0 as NaN or "Not A Number." When it gets to the driver/show.html.erb view, NaN.to_s evaluates to just "NaN" so it shows up on the driver page as "NaN" on the page.

<section class="passenger-trips">
<h3> Passenger Trips </h3>
<table id="driver-trips-table">
<tr>

Choose a reason for hiding this comment

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

You have almost exactly the same code to render the table of trips on the passenger show page and the driver show page. Could you DRY this up with a partial somehow?


driver = (Driver.all.select { |d| d.availability == true }).sample

@trip = passenger.trips.new(driver_id: driver.id, passenger_id: passenger.id, date: Date.today, rating: 0, cost: 0)

Choose a reason for hiding this comment

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

All of the work done in this method could be moved to a model method. An instance method on Passenger would probably work well, something like Passenger#request_trip.

def update
@driver = Driver.find(params[:id])

if @driver.update(driver_params)

Choose a reason for hiding this comment

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

It's interesting to me that in create you store the strong params in an intermediate local variable, but in update you pass it directly into the model. Using the local variable is fine if it makes the code clearer to you, but you should follow the same pattern in both places.

resources :drivers

resources :trips

Choose a reason for hiding this comment

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

Do you need all 7 restful routes for :trips? I think that both :new and un-nested :create are not used.

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.

3 participants