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 1 Denis Pavlov #124

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

Task 1 Denis Pavlov #124

wants to merge 4 commits into from

Conversation

di8905
Copy link

@di8905 di8905 commented Mar 10, 2023

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.

В целом ок, хорошо бы было заполнить плейсхолдеры и написать за сколько получилось уложиться в результате.

Но понравился минимализм, всего лишь два изменённых файла. Некоторые коммитят по 150к строк

Я решил исправить эту проблему, оптимизировав эту программу.

## Формирование метрики
Для того, чтобы понимать, дают ли мои изменения положительный эффект на быстродействие программы я придумал использовать такую метрику: *тут ваша метрика*
Copy link
Collaborator

Choose a reason for hiding this comment

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

*тут был плейсхолдер*

Программа поставлялась с тестом. Выполнение этого теста в фидбек-лупе позволяет не допустить изменения логики программы при оптимизации.

## Feedback-Loop
Для того, чтобы иметь возможность быстро проверять гипотезы я выстроил эффективный `feedback-loop`, который позволил мне получать обратную связь по эффективности сделанных изменений за *время, которое у вас получилось*
Copy link
Collaborator

Choose a reason for hiding this comment

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

*тут тоже*

- какой отчёт показал главную точку роста
По отчету Stackprof увидел большие затраты на Date.parse при сборе дат сессий пользователей, 34 процента общего времени
- как вы решили её оптимизировать
Использовать явный формат Date.strptime так как даты в файле в одном формате
Copy link
Collaborator

Choose a reason for hiding this comment

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

Дата на самом деле сразу в нужном формате, можно с ней ничего не делать


*Какими ещё результами можете поделиться*
Было открытием для меня что один из способов собрать обработать уникальные данные - пройтись по массиву и убрать эти данные в множество Set,
которое априори уникально, что это очень медленно. Это типичный паттерн в пайтоне насколько знаю. Оказывается собрать уникальные браузеры через хэщ быстрее на порядок.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Очень странно, насколько я знаю Set внутри работает примерно как и Hash. И многие ребята используют Set в этом задании. Бывает пишут, что чуть медленнее Hash, но не на порядок, а чуть-чуть

if br_hash[browser]
else
uniqueBrowsers << browser
br_hash[browser] = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

двойная работа осталась


{
'sessionsCount' => user.sessions.count,
'totalTime' => "#{sessions_timings.inject(:+)} min.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

inject(:+) -> sum

'alwaysUsedChrome' => !user_browsers.any? { |b| b !~ /CHROME/ },
'dates' => user.sessions.map { |s| s['date'] }
.map { |d| Date.strptime(d, '%Y-%m-%d') }
.sort { |a, b| b <=> a }
Copy link
Collaborator

Choose a reason for hiding this comment

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

reverse

'dates' => user.sessions.map { |s| s['date'] }
.map { |d| Date.strptime(d, '%Y-%m-%d') }
.sort { |a, b| b <=> a }
.map(&:iso8601)
Copy link
Collaborator

Choose a reason for hiding this comment

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

не надо

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