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

Task 3 solution #71

Closed
wants to merge 1 commit into from
Closed

Task 3 solution #71

wants to merge 1 commit into from

Conversation

d1ma-angel
Copy link

No description provided.

Copy link
Collaborator

@spajic spajic left a comment

Choose a reason for hiding this comment

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

@@ -2,15 +2,7 @@
<%= "Автобусы #{@from.name}#{@to.name}" %>
</h1>
<h2>
<%= "В расписании #{@trips.count} рейсов" %>
<%= "В расписании #{@trips.size} рейсов" %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Лучше @trips.load.size, чтобы явно написать что происходит

</ul>
<%= render "delimiter" %>
<% end %>
<%= render partial: 'trip', collection: @trips, as: :trip %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Можно ещё delimiter задать параметром: https://guides.rubyonrails.org/layouts_and_rendering.html#spacer-templates

Для того, чтобы понимать, дают ли мои изменения положительный эффект на быстродействие программы я придумал использовать такую метрику: запуск rake-задачи с функцией `time`

```bash
time be rake reload_json\[fixtures/small.json\]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Минус такого подхода - вы заодно меряете время на загрузку рельсового окружения.
Получатся 1) дольше 2) сложнее понять что именно с вашим куском кода происходит


## Гарантия корректности работы оптимизированной программы

Для корректности работы был предоставлен пример результатов, которые должны получиться и с ними происходила сверка.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

class AddIndexesToTables < ActiveRecord::Migration[5.2]
def change
add_index :buses, :number, unique: true
add_index :buses_services, %i[bus_id service_id], unique: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

хорошо ещё добавлять с algorithm: :concurrently
в данном случае конечно не важно, а вот на проде лучше не забывать

task :reload_json, [:file_name] => :environment do |_task, args|
require 'activerecord-import/base'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Внутренности этой rake-таски лучше вынести в отдельный класс.
Так получается удобнее.
rake-задачи хорошо держать тоже "тонкими", просто брать аргументы командной строки, если есть, и вызывать класс, который уже делает реальную работу

trip['bus']['services'].each do |service|
s = Service.find_or_create_by(name: service)
services << s
cities << trip['from']
Copy link
Collaborator

Choose a reason for hiding this comment

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

С Set очень удобно тут 👍

let(:task) { Rake::Task['reload_json'] }

it 'works creates Buses' do
expect { task.invoke('fixtures/example.json') }.to change(Bus, :count).from(0).to(1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

let(:subject) { task.invoke('fixtures/example.json') }
expect { subject }.to change(Bus, :count).from(0).to(1)

@d1ma-angel d1ma-angel closed this by deleting the head repository Feb 24, 2023
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.

None yet

2 participants