# Заметки по ревью кода Острякова В.

*(тестовое задание в контент WB)*

Ревьюер: [Гульков И.С.](https://t.me/isgulkov)

Версия репы: [`d575b38a5852034104de0cfe3cf036ea9bdd9ca9`](https://github.com/Lnx117/L3WB/tree/d575b38a5852034104de0cfe3cf036ea9bdd9ca9)

Допы:

- [x] 1\. Асинхронное обновление данных
- [x] 2\. Распараллеливание процесса
- [ ] 3\. Docker
- [ ] 4\. «Для самых смелых»
- [x] 5\. Swagger, тесты, логи

## Несоответствия требованиям

1. Данные обновляются не раз в минуту, а раз в "минуту + время самого обновления"

## Замечания

*Эти пункты в той или иной мере повлияли на оценку*

### API самого сервиса

1. Сервис вообще не отвечает на запросы, пока не завершится загрузка данных из внешнего API, при том что длительность этой операции велика (практически не ограничена), а успех — не гарантирован.

Можно было бы сразу запустить HTTP-сервер, а в фоне уже че-то там обновлять: если это не первый запуск, то данные в базе уже есть, пусть и необновленные; мы их могли бы начать отдавать сразу же, как к ней подключились!

### Поддерживаемость и кодстайл

1. [Передаем вейт-группу в другую функцию](https://github.com/Lnx117/L3WB/blob/d575b38a5852034104de0cfe3cf036ea9bdd9ca9/pkg/service/geocodingService.go#L75), просто чтобы вызвать там внутри `wg.Done()`. Ну, можно же вот так делать:

   go func() {
       syncCitiesOpenweathermapData.GetCityOpenweathermapData(v)
       wg.Done()
   }()

*Когда синхронизационный примитив можно не выпускать за пределы одной функции, следует так и поступать*

### Обработка ошибок

1. [Логируем ошибку и идем дальше](https://github.com/Lnx117/L3WB/blob/d575b38a5852034104de0cfe3cf036ea9bdd9ca9/pkg/repository/postgresQueries.go#L96), вставлять в БД заведомо невалидные данные; [вот то же самое](https://github.com/Lnx117/L3WB/blob/d575b38a5852034104de0cfe3cf036ea9bdd9ca9/pkg/repository/postgresQueries.go#L129): залогировали ошибку и пошли ковырять заведомо невалидный `rows`...
2. [Неоправданный фатал еррор](https://github.com/Lnx117/L3WB/blob/d575b38a5852034104de0cfe3cf036ea9bdd9ca9/pkg/repository/postgresQueries.go#L35) (крэш сервера) из-за всего лишь невалидного состояния данных в БД
3. Очень многие функции, где ошибки возможны и вероятны (особенно связанные с походом во внешнее API), не возвращают их, что позволило бы, например, сделать ретрай, зато сразу, на месте, логируют

### Работа с БД

- Подставлять параметры в запрос текстом — недопустимо, т.к. приводит к SQL-инъекциям (в твоем коде одна такая даже есть). Для подстановки данных в запрос следует использовать `$`-параметры

### Работа с внешним API

- [Вот здесь](https://github.com/Lnx117/L3WB/blob/d575b38a5852034104de0cfe3cf036ea9bdd9ca9/pkg/service/geocodingService.go#L75) мы условно одновременно отправляем N запросов, где N — количество городов. С 20 городами именно этот сервис такое позволяет, а если их в БД будет 1000? *(тогда тебя забанят по API-ключу, я проверял уже)*

## Придирки по-мелочи

*Эти пункты __не__ повлияли на оценку*

### Поддерживаемость и кодстайл

- [Локальная переменная называется с большой буквы](https://github.com/Lnx117/L3WB/blob/d575b38a5852034104de0cfe3cf036ea9bdd9ca9/pkg/repository/postgresQueries.go#L60)

### Работа с БД

- Держать имена таблиц в константах и потом их подставлять под `%s` — это какая-то дичь...