Feedback PR only#27
Conversation
Local merge
…e-share into CRUD_stations_laszlo
Merge CRUD_stations_laszlo branch into CRUD_stations branch
Itertaion6 wolff
Local merge of iteration6-laszlo with latest version of iteration6
Iteration6 shipley ok to be merged into `iteration6`
adds photos
Local merge with master update with images and iteration6-laszlo branch.
Iteration6 merge into master for new baseline
Local merge to ensure using new baseline.
Laszlo error handling refactor into MASTER
Laszlo error handling refactor into Master
Lastday shipley
Most and Fewest Days
There was a problem hiding this comment.
@wlffann @Laszlo-JFLMTCO @Sh1pley @MarisaMBurton good work on this project. i left ya some comments - let me know if you have questions on any of my comments or anything else 🎉
also - i LOVE all the commits. woooo 👍
keep up the hard work!
| require 'will_paginate/active_record' | ||
|
|
||
| class BikeShareApp < Sinatra::Base | ||
| include WillPaginate::Sinatra::Helpers |
|
|
||
| get '/stations/:id' do | ||
| @station = Station.find(params[:id]) | ||
| erb :"stations/show" |
There was a problem hiding this comment.
good work nesting your files with your views
|
|
||
| put '/stations/:id' do | ||
| station = Station.find(params[:id]) | ||
| station.write_update(station, params[:stations]) |
There was a problem hiding this comment.
since write_update is an instance method, do we need to pass in the specific station since we already have access to it? you can access all the attributes within a specific instance of Station within an instance method 👍
|
|
||
| get '/trips/:id/edit' do | ||
| @trip = Trip.find(params[:id]) | ||
| @subscription_type = Subscription.find(@trip.subscription_id) |
There was a problem hiding this comment.
if trip has an association with subscription, we should be able to just call @trip.subscription rather than calling find
|
|
||
| class Condition < ActiveRecord::Base | ||
| include DateFormat | ||
| has_many :trips, class_name: "Trip", primary_key: "date", foreign_key: "start_date" |
| @@ -0,0 +1,167 @@ | |||
| require_relative '../models/subscription' | |||
| require_relative '../models/date_format' | |||
There was a problem hiding this comment.
you shouldn't have to manually require these models. they should all be required for you in config/environment.rb
| </tr> | ||
| <tr> | ||
| <td>41-50</td> | ||
| <td><%= Condition.average_number_of_rides_in_range_max_temp(41..50) %></td> |
There was a problem hiding this comment.
we talked about this in your eval, but i'll reiterate it here. currently, we're overstepping the controller's responsibilities. the controller should grab this data from your models and the view should simply display the data. as this view template stands, there's too much logic here because it's actually querying your database by accessing a model directly.
|
|
||
| <tr> | ||
| <td><h3> Total Station Count</td></h3> | ||
| <td><h3> <%= Station.count %> </td></h3> |
There was a problem hiding this comment.
same here - we want our controller to get the data and our view to simply display it 👍
| <div class="thumbnail"> | ||
| <div class="caption"> | ||
| <h3>Single Date with Most Trips</h3> | ||
| <p>Date: <%= Trip.date_with_highest_number_of_trips_with_count_of_those_trips[:date_with_most_trips] %></p> |
| end | ||
|
|
||
| it "they press the delete button from the show file" do | ||
| Station.write(name: "TestStation1", |
There was a problem hiding this comment.
you can DRY this up a bit by using a before filter or let blocks in RSPEC 👍
3 participants
do not merge