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

Angelica & Cara - Rideshare Rails - Octos #15

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

Conversation

amcejamorales
Copy link

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 We set up Drivers and Passengers to have many Trips because each trip could only have one passenger and one driver, but each driver or passenger could have multiple trips.
Describe the role of model validations in your application We used model validations to ensure that each passenger had a name and a phone number and that each driver had a name and a vin. This was to ensure we didn't get any errors when calling on this information.
How did your team break up the work to be done? We worked together on big chunks of the main portion of the program, Trips, and each took Drivers and Passengers. We communicated over slack to divvy up styling.
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 Trips was the most important so we set it up first, but then worked on our individual components and came back together for Trips as different functionalities became relevant. We did not figure out how to include a float field in the search bar, so we didn't allow the user to search by cost like we had initially planned.
What was one thing that your team collectively gained more clarity on after completing this assignment? How the files relate to each other. When we changed something in one file we got a better sense of how this would affect files elsewhere.
What is your Trello URL? https://trello.com/b/xnIu9Mug/rideshare-rails-422018
What is the Heroku URL of your deployed application? https://rideshare-rails-ac.herokuapp.com/trips
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? One thing we discussed was that we worked well and communicated both in person and over slack. Another thing we could improve on would be checking for understanding each step of the way so both parties know why we're taking the project in a certain direction.

amcejamorales and others added 30 commits April 2, 2018 15:56
We are working on different things, and are combining our two parts.
MississippiBrenn and others added 28 commits April 5, 2018 14:01
Merging back to original after adding search bar for passengers
@kariabancroft
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 Mostly. I'd like to have seen more equitable contributions from Cara early on in the project.
Answered comprehension questions Yes
Uses named routes (like _path) Yes
RESTful routes utilized resources routes look good though there are others that are extraneous and should not be used.
Rideshare Rails Specific Content
Table relationships Looks good in the schema as well as the model files
Validation rules for Models Yes, using presence validators in the models
Business logic is in the models Yes, there is definitely some cost and search logic in the model methods. This does a nice of preventing logic from showing up in the views unnecessarily.
Database is seeded from the CSV files Yes, data on Heroku deployment looks good.
Trello board is created and utilized in project management Yes, nice job organizing by waves
Postgres database is used Yes
Heroku instance is online Yes
The app is styled to create an attractive user interface Yes - see some comments inline on the code
Overall You did a nice job hitting the major learning goals of this assignment. Overall, the biggest style issue I noticed is indentation! Since the views don't update this automatically, it is really important to do this yourself. It makes the view code a lot easier to read when it is structure appropriately.

get 'trips/index'

# For details on the DSL available within this file, see http://guides.rubyonrails.org/routing.html
root 'home#index'

Choose a reason for hiding this comment

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

For readability, we put the root route at the top of the routes file.

Rails.application.routes.draw do
get 'passengers/index'

get 'passenger/index'

Choose a reason for hiding this comment

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

Why would you have both this route and a plural version, neither of which follow the RESTFUL standard or get mapped to a controller action?

<%= submit_tag "Search" %>
<% end %>

<section class = "add-link">

Choose a reason for hiding this comment

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

When we use classes in our HTML, we do not use the same spacing conventions as we do in our Ruby code. We should not put spaces on either side of the equal, so: class="add-link"

<%= link_to driver.name, driver_path(driver) %>
</strong>
</li>
</aside>

Choose a reason for hiding this comment

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

Watch your indentation here - its not right and makes the view code much harder to read than it needs to be.

belongs_to :passenger


def convert_to_currency(n)

Choose a reason for hiding this comment

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

EEK! Watch indentation here - this is challenging to read

Driver's Average Rating: <%= @driver.average_rating %>
</p>
<p>
Driver's Trips:

Choose a reason for hiding this comment

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

I'm not sure what happened with this section, but it ends up being really difficult to read. Recommend creating an overall semantic section of some sort of encapsulate all of this trip information. As well as possibly another HTML container that would contain the details of one piece of trip info.

class DriversController < ApplicationController
def index
@drivers = Driver.all
if params[:search]

Choose a reason for hiding this comment

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

Nice job using the same route and pivoting on the params to determine which logic to execute

@@ -0,0 +1,50 @@
class PassengersController < ApplicationController
def index
@passengers = Passenger.all

Choose a reason for hiding this comment

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

By doing this first, you'll be doing an extra database query that you don't need. You only need the queries in the conditionals.

@@ -0,0 +1,61 @@
class TripsController < ApplicationController
def index
@trips = Trip.all.order(:id)

Choose a reason for hiding this comment

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

Same comment re: extra DB query

end

def create
passenger = Passenger.find_by(id: params[:passenger_id])

Choose a reason for hiding this comment

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

This would potentially be good logic to move into the Trip object so that all of the defaults would be captured there instead

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