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

Iter9 #9

Open
wants to merge 29 commits into
base: main
Choose a base branch
from
Open

Iter9 #9

wants to merge 29 commits into from

Conversation

shilin-anton
Copy link
Owner

No description provided.

@shilin-anton shilin-anton self-assigned this Mar 27, 2024
@shilin-anton shilin-anton requested a review from Bazys March 28, 2024 10:07
Copy link
Collaborator

@Bazys Bazys left a comment

Choose a reason for hiding this comment

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

Сам код написан хорошо, но он написан в нектором структурно-функциональном стиле.
Сейчас ты хранишь только в памяти, дальше надо будет сохранять на диск, а еще дальше - будем работать с БД.
Все время ифчиками это не обложить.
Посмотри в сторону Dependency Injection и чистой архитектуры.
Вот ссылка для старта процесса изучения. Но это всего лишь старт, не окончательная версия.
Можно еще тут посмотреть, здесь показано как DI облегчит тестирование в дальнейшем.
Сейчас принимаю, но начинай задумываться над рефакторингом в сторону чистой архитектуры.

}

// добавляем реализацию http.ResponseWriter
loggingResponseWriter struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

не увидел использования этой структуры

Copy link
Owner Author

Choose a reason for hiding this comment

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

На предыдущих итерациях эти структурки были нужны, просто забил их выпилить. Спасибо за наблюдение


type (
// Структура для хранения сведений об ответе
responseData struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

так же не увидел использования этой структуры

Copy link
Owner Author

Choose a reason for hiding this comment

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

здесь та же история, что и с предыдущей структурой)
спасибо

Remove storage import from file-manager,
Change logic in main,
Change code style in files,
Remove server package,
Fix tests
Separate filemanager layer from handlers,
Improve filemanager handling in storage
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