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

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

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

Версия репы: [`db75d096d0e648358de981368e4cb9903f076c8f`](https://github.com/VitJRBOG/weather-forecast-api/tree/db75d096d0e648358de981368e4cb9903f076c8f)

Допы:

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

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

*Не обнаружены*

## Замечания

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

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

1. Сервис вообще не отвечает на запросы, пока не завершится загрузка данных из внешнего API, при том что длительность этой операции велика (практически не ограничена), а успех — не гарантирован. Можно было бы сразу запустить HTTP-сервер, а в фоне уже че-то там обновлять: если это не первый запуск, то данные в базе уже есть, пусть и необновленные; мы их могли бы начать отдавать сразу же, как к ней подключились!
2. `sendError` пишет HTTP-статусы (типа `http.StatusBadRequest`) только в тело запроса. Их туда, конечно, класть можно, но в первую очередь они предназначены в качестве кода HTTP-ответа (он проставляется методом `w.WriteHeader`)

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

Этот код полностью полагается на быструю и безотказную работу внешнего API. Так делать нельзя — каким бы железобетонным не был внешний сервис, стандартная практика все равно предполагает ряд мер на случай, если он начнет капризничать:

1. Явно указываемый таймаут на каждый запрос и хотя бы парочка ретраев на случай редко всплывающей ошибки
2. Рейт-лимит на клиентской стороне. Здесь мы шлем **N запросов условно одновременно**. С этим API и с N=20 всё работает, но в общем случае ни один сервис так использовать не следует: вот я записал в базу 1107 городов вместо 20, и мне за такой запуск ключ забанили, а он был даже не мой, а из репы Сидорова (true story).

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

1. В `saveDataToDB`: ошибка из `db.SelectByCityAndDateFromForecast` обрабатывается наихудшим возможным способом – _завершением сервиса_. В реальной жизни это будет рестарт, но как раз на старте у нас а) слабо ограниченный по времени даунтайм и б) потенциально снова эта же ошибка. *Ситуации, в которых нет другого выбора, кроме как помереть, бывают, но это точно не тот случай*
2. В том же `saveDataToDB`: ошибка из `db.InsertIntoForecast` обрабатывается вторым наихудшим способом – _совершенно никак_. Точнее, она логируется, но внутри самой вызываемой функции, прямо перед `return err`. *Ошибку либо обрабатывают на месте, либо возвращают, передавая весь геморрой в вызывающий код. Совмещение этих двух действий — неправильное использование ошибок*
3. Ну и вообще, мы как во внешний сервис шлем одновременно N→∞ запросов, так же и в базу — так тоже делать нельзя, по тем же причинам *(но с базой нагляднее — если в таблицу `cities` вставить 1107 городов вместо 20, сервис реально падает по превышению лимита подключений в п.1)*

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

1. Много функций, между которыми таскается один и тот же `*sql.DB` — это намек, что неплохо было бы их сделать методами одной структуры *(или хотя бы в глобальную переменную эту `DB` записать, прости г-ди)*
2. В `collectingData`: неоправданное дробление работы с `sync.WaitGroup` между функциями: пихаем эту вейтгруппу в вызов `fetchAndSave`, просто чтобы внутри сделать `wg.Done()`. Можно и нужно было вызовы `fetchAndSave` и `wg.Done` делать в одной inline-функции в `collectingData`, сразу после слова `go`. *Это на этом проекте кажется, что мелочь, но пара месяцев такой разработки — и сам увидишь, как эта вейтгруппа по всему коду гастролирует вместе с `*sql.DB`*
3. Вместо того чтобы извлекать и валидировать значения GET-параметров прямо в HTTP-хэндлере, эта работа передается вглубь дерева вызовов. То, что `url.Values` протекает в код, которому ни про какое HTTP знать не нужно и нежелательно — это еще ладно. Главное, что из-за этого по коду хэндлера невозможно понять, какие он ожидает параметры

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

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

### "Юзабилити"

- Можно было хотя бы чуть-чуть логировать, че щас происходит ("коннектимся к БД...", "пошли в апишку за данными...") — тогда, если на чем-то из этого сервис молча помирает или просто долго ворочается, сразу примерно понятно, на чем

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

- В `root.go` ряд функций не возвращает ошибку, хотя источников ошибок в них навалом; при этом в остальной программе с этим проблем нет или почти нет
- В `if` перед условием можно впихнуть стейтмент (напр., объявление переменной) — эта прекрасная фича вообще не используется, хотя для этого куча мест
- В нескольких местах видно, что код ужимался по ширине до 80 колонок. Ограничивать ширину — это хорошо, но 80 колонок — вредный архаизм, в 2022 году забота о читателе кода предполагает 120

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

- Для GPS-координат (`lat`+`lon`) не нужно и не следует использовать десятичный тип (`numeric` = `decimal`) — это как раз тот случай, когда `double` подходит идеально
- После цикла `for rows.Next() { ... }` нужно **обязательно** проверять `rows.Err()`! Наличие там ошибки означает, что ответ считался не полностью (не все строки)

### Docker

- Указывать `CMD` как-то принято в самом докерфайле, а не в `docker-compose.yml`...