-
Notifications
You must be signed in to change notification settings - Fork 0
start hw12 #12
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
base: master
Are you sure you want to change the base?
start hw12 #12
Conversation
sshaplygin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Здравствуйте. Спасибо за выполненное задание. Немного не хватает, чтобы получить необходимо количество баллов для зачета. У вас обязательно получится 😉 осталось чуть-чуть
Итого: 6/10
[x] Понятность и чистота кода (включая факт, что проект разбит
на пакеты по определенной логике) - 1/2 баллов
[x] Реализовано хранилище:
[x] in-memory - 0/1 балл
[x] sql + миграции - 1/2 балла
[x] Присутствуют юнит-тесты - 0/1 балл так как существую баги, которые тесты не отлавливают
|
|
||
| var ErrEventNotFound = errors.New("event not found") | ||
|
|
||
| func (s *Storage) Connect(ctx context.Context) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
Зачем вы реализуете те методы, которые не присущи данному типу хранилища?
| s.mu.Lock() | ||
| event.ID = s.LastID | ||
| s.LastID++ | ||
| s.Events[event.ID] = &event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
У вас все равно будет гонка по памяти, так как указатель, который вы записываете и указатель, который вы возвращаете разный, но он ссылается на один и тот же участок памяти. нужно делать копию данных
| } | ||
| s.mu.RUnlock() | ||
|
|
||
| s.Events[event.ID] = &event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
Изменение данных тоже нужно закрывать мьютексом, но не на чтение, а брать полную блокировку, так как мы можем конкуретно изменять один и тот же участок памяти
No description provided.