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

Add Vagons #2

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

Add Vagons #2

wants to merge 24 commits into from

Conversation

mazovladimir
Copy link
Owner

No description provided.

private

def set_number
self.number = Random.rand(0...10)

Choose a reason for hiding this comment

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

Если у тебя все номера будут от 0 до 10, то номера могут повторяться, а этого по условиям задачи быть не должно. Надо делать по другому: брать максимальное значение номера вагона у поезда и прибавлять к нему 1. В этом поможет метод maximum

@vkurennov
Copy link

Исправь замечания и я не увидел возможности указания времени прибытия и отправления на станцию

private

def set_number
self.number = Vagon.maximum('number') + 1
Copy link

@vkurennov vkurennov Jun 25, 2016

Choose a reason for hiding this comment

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

Опять же ты берешь максимальное значение номера вагона глобально. А надо в рамках поезда, т.е. у 2-х разных поездов могут быть вагоны с одинаковыми номерами. А у тебя получается, что если у одного поезда есть вагон с номером 1, то у другого, нумерация будет уже с 2.

@vkurennov
Copy link

С 15-м в целом ок, но исправь замечания


scope :count_order, -> { order(:count) }

def self.show_trains

Choose a reason for hiding this comment

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

В чем смысл этого метода? Он перебирает названия у станций, но возвращает массив маршрутов, т.к. each возвращает оригинальный массив. И в чем тогда смысл?

@vkurennov
Copy link

Исправь замечания

route1 = RailwayStation.find(start_station).routes.all
route1.each do |route1|
route1.trains.to_a.each do |train|
return train.title

Choose a reason for hiding this comment

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

Здесь у тебя будет возвращено название только первого поезда, т.к. после return происходит выход из всего метода.

route1 = RailwayStation.find(start_station).routes.all
route2 = RailwayStation.find(end_station).routes.all
routes = route1 & route2
routes.each do |myroutes|

Choose a reason for hiding this comment

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

Я же тебе написал как найти все поезда одним запросом без перебора: Train.where(route: routes).all

@@ -0,0 +1,28 @@
<%= form_for(@ticket) do |f| %>

Choose a reason for hiding this comment

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

А как мы передаем от куда и до куда у нас билет?

@vkurennov
Copy link

С поиском худо-бедно разобрались (хотя я написал как надо сделать), но это еще далеко не конец. Исправь остальные замечания.

@@ -9,13 +9,16 @@ def show

def new
@ticket = Ticket.new
@ticket.first_station = Ticket.result_start(params[:first_station])

Choose a reason for hiding this comment

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

Зачем тебе методы result_start и result_end, когда в параметрах приходят id станции и ее можно загрузить по RailwayStation.find?

@vkurennov
Copy link

Ладно, будем считать, что сделано. Принимаю.

def edit
end

def create

Choose a reason for hiding this comment

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

В задании не сказано, что админ может создавать билеты

@vkurennov
Copy link

Исправь добавление имени при регситрации

@@ -2,4 +2,25 @@ class ApplicationController < ActionController::Base
# Prevent CSRF attacks by raising an exception.
# For APIs, you may want to use :null_session instead.
protect_from_forgery with: :exception
before_action :configure_sign_up_params, if: :devise_controller?
before_action :configure_sign_in_params, if: :devise_controller?

Choose a reason for hiding this comment

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

Зачем одно и то же дважды?

@vkurennov
Copy link

Ок, принимаю, без усложненного.

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.

2 participants