-
Notifications
You must be signed in to change notification settings - Fork 5
lesson 3.1: added go example #25
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: main
Are you sure you want to change the base?
lesson 3.1: added go example #25
Conversation
rekby
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.
Привет, спасибо за PR.
Отличная работа - код хорошо читается и прокомментирован.
Сделал несколько комментариев для улучшения познавательной ценности примера для студентов.
| // чтобы избежать дублирования при следующем выполнении функции-ретраера | ||
| // и вернуть ретраеру ошибку | ||
| if err != nil { | ||
| clear(result) |
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.
тут и далее про clear
- Такую очистку лучше проводить в начале функции - оно одно и очевидное. Ошибок может быть много и тогда пришлось бы повторять это код рядом с каждой ошибкой.
Цена - лишняя очистка пустого результата на старте функции.
Ещё вариант - сделать результат именованной ошибкой и очистку выполнять в defer если возвращем ошибку.
- Использование clear тут тоже создаёт ошибку. result - это слайс, clear для слайсов - зануляет значения всех элементов слайса, но не обнуляет его длину. Тут же - важно обнулить длину слайса, иначе при ретраях получится результат где первые значения нулевые из-за ретраев, а правильный результат начинается с неизвестного индекса в хвосте result.
| // чтобы избежать дублирования при следующем выполнении функции-ретраера | ||
| // и вернуть ретраеру ошибку | ||
| if err != nil { | ||
| clear(result) |
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.
... про clear
dev-1/lesson-3.1/go/query_helper.go
Outdated
| // Это улучшает тестируемость - можно передать mock-объект для тестов | ||
| // 2) Универсальная логика выполнения запросов и чтения результатов | ||
| // 3) В методах принимает контекст, который может управлять исполнением запросов (например, через таймаут) | ||
| type QueryHelper struct { |
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.
Вот это я бы убрал - хочется в коде примеров видеть именно код работы с драйвером. В прод-варианте обёртки/хелперы это хорошо, но в учебном курсе лучше оставить это явным: чтобы проще было понять как это работает и как пользоваться самим SDK.
dev-1/lesson-3.1/go/query_helper.go
Outdated
| return err | ||
| } | ||
|
|
||
| materializeFunc(resultSet, ctx) |
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.
Такая штука скорее вредная - её сложно понять, к тому же в YDB уже есть свой такой материализатор, если выполнять запрос на query-клиенте.
В качестве примера для очистки результатов при ретрае нужно привести пример интерактивной транзакции, это ровно то место, где такой код требуется в реальности.
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.
а точно ли стоит пихать сюда интерактивные транзакции?
это все-таки урок про базовую работу с таблицами, про транзакции там ничего не было
| // Репозиторий для работы с тикетами в базе данных YDB | ||
| // Реализует операции добавления и чтения тикетов | ||
| type IssueRepository struct { | ||
| query *QueryHelper |
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.
Тут нужно избавиться от хелпера
| // Репозиторий для управления схемой базы данных YDB | ||
| // Отвечает за создание и удаление таблиц | ||
| type SchemaRepository struct { | ||
| query *QueryHelper |
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.