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

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

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

Версия репы: [`799eacd8676fe4566a82085b5b00ba21f28c8c49`](https://github.com/sidorovvadim33/WeatherServiceApi/tree/799eacd8676fe4566a82085b5b00ba21f28c8c49)

Допы:

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

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

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

## Замечания

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

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

1. В куче мест, где ошибка — обычное дело (в частности, при походе по HTTP, запросе в БД), она обрабатывается "фатал еррором" (крэшем сервера). Это совершенно недопустимо

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

1. [Вот тут](https://github.com/sidorovvadim33/WeatherServiceApi/blob/799eacd8676fe4566a82085b5b00ba21f28c8c49/internal/api/weatherClient/weatherClient.go#L33-L55) мы запускаем множество HTTP-запросов во внешний сервис условно одновременно. Так нельзя, нужен рейт-лимит. При 20 городах всё работает, но если городов в базе хотя бы 1117... результат тебе уже известен 🌝

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

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

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

1. Формат тел ответов везде JSON, что прекрасно, но в некоторых случаях (напр. `POST /api/userfavs/...`) ответ приходит просто в виде текста "`1`". Так не годится: если везде JSON, то везде JSON, пусть даже просто `{ok: true}`
2. Система авторизации, в которой с каждым запросом нужно слать пароль — это а) небезопасно и б) колхозно. Раз уж заморочался, мог бы и bearer-токены прикрутить

### Архитектура

1. Что за термины `service` и `storage`? В частности, нахрена нужен `service`, если весь код всё равно в `storage`? Где ты это взял? *(просто у нас в коде ровно то же самое, и я всё это время был уверен, что это эндемическое заболевание)*

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

1. Вот это вот:

    for _, city := range cities {
		city := city
		go func() {
			// Использование city
		}()
	}

покрасивше ж вот так:

    for _, city := range cities {
		go func(city cityClient.CityData) {
			// Использование city
		}(city)
	}

2. `w.WriteHeader(http.StatusOK)` — это no-op
3. А зачем использовать `net.Listen` вообще? HTTP-сервер из `net/http` сам это умеет...
4. Куча мест ([например](https://github.com/sidorovvadim33/WeatherServiceApi/blob/799eacd8676fe4566a82085b5b00ba21f28c8c49/internal/api/weatherClient/weatherClient.go#L36)), где переменная объявляется в самом начале функции, хотя используется только через 20–30 строк. Это чё, C89?