Reholly / de-csu

0 stars 0 forks source link

ревью #3

Open crutchM opened 1 month ago

crutchM commented 1 month ago

ПРЕЖДЕ ЧЕМ ПИСАТЬ МНЕ И СПРАШИВАТЬ/ОТВЕЧАТЬ ПОЧИТАЙ ДО КОНЦА Я ГДЕ-ТО ТУТ ПЕРЕОБУЛСЯ

душную духоту про арху и это все опустим, скрипт для заливки данных, пофек

https://github.com/Reholly/de-csu/blob/a1c09a91db98ebc9a0495f2ce1abdcec442c33f9/candles/cmd/main.go#L175

в константу

https://github.com/Reholly/de-csu/blob/a1c09a91db98ebc9a0495f2ce1abdcec442c33f9/candles/cmd/main.go#L184

есть envconfig что умеет собирать в структуру конфини из .env'a имхо так удобнее, но на вкус и цвет)

https://github.com/Reholly/de-csu/blob/a1c09a91db98ebc9a0495f2ce1abdcec442c33f9/candles/cmd/main.go#L178

Зачем? zero value и так пустая строка

https://github.com/Reholly/de-csu/blob/a1c09a91db98ebc9a0495f2ce1abdcec442c33f9/candles/cmd/main.go#L189

харам

https://github.com/Reholly/de-csu/blob/a1c09a91db98ebc9a0495f2ce1abdcec442c33f9/candles/cmd/main.go#L191

если прям в край душиться, то пул бы ограничить ручками

https://github.com/Reholly/de-csu/blob/a1c09a91db98ebc9a0495f2ce1abdcec442c33f9/candles/cmd/main.go#L234

✨magic✨

https://github.com/Reholly/de-csu/blob/a1c09a91db98ebc9a0495f2ce1abdcec442c33f9/candles/cmd/main.go#L238

странные штуки происходят, я внутрь еще не смотрел, но все же обычно примитивами синхронизации мы рулим по месту объявления, если внутри wg нужна, то и заводи отдельно, а во внешке по классике

go func(){
    defer wg.done()
    foo()
}()

https://github.com/Reholly/de-csu/blob/a1c09a91db98ebc9a0495f2ce1abdcec442c33f9/candles/cmd/main.go#L244

а ты это вщ запускал?) wg объявляется и забивается, wg.Wait в мейне нет, мб оно живет чисто на магии time.Sleep ЭТО

https://github.com/Reholly/de-csu/blob/a1c09a91db98ebc9a0495f2ce1abdcec442c33f9/candles/cmd/main.go#L254 по-хорошему бы сделать allCandles := make([][]models.Candle, 0, len(instrument.Instruments)) чтоб не получить заполненный zero-value слайс на входе, ну либо вместо аппенда жоска по индексу ставить сабслайсы

https://github.com/Reholly/de-csu/blob/a1c09a91db98ebc9a0495f2ce1abdcec442c33f9/candles/cmd/main.go#L259

✨magic✨

https://github.com/Reholly/de-csu/blob/a1c09a91db98ebc9a0495f2ce1abdcec442c33f9/candles/cmd/main.go#L270

то же самое что и с верхним слайсом(хотя тут уже по индексу заполнение, но мне лень стирать)

https://github.com/Reholly/de-csu/blob/a1c09a91db98ebc9a0495f2ce1abdcec442c33f9/candles/models/candle.go#L18

из меня сукульщик никакущий, но имхо генерить на стороне гошки генерить юиды поприкольнее, т.к. можно организовать генерацию UUIDv7, который по рассказам волшебных сеньоров гарантирует тебе упорядоченность, т.е. поприкольнее индексам на стороне самой постгри работается на почитать

https://github.com/Reholly/de-csu/blob/a1c09a91db98ebc9a0495f2ce1abdcec442c33f9/candles/storage/init.sql#L20

в постгре вроде есть енамы, зачем плодить тогда таблички?

https://github.com/Reholly/de-csu/blob/a1c09a91db98ebc9a0495f2ce1abdcec442c33f9/candles/storage/storage.go#L19

зачем тянуть указатель на структуру в которой лежит еще один указатель? на распаковке больше потеряешь чем на копирование структуры с одним указателем

https://github.com/Reholly/de-csu/blob/a1c09a91db98ebc9a0495f2ce1abdcec442c33f9/candles/storage/storage.go#L68

ладна, видимо типы динамичные зря быканул

https://github.com/Reholly/de-csu/blob/a1c09a91db98ebc9a0495f2ce1abdcec442c33f9/candles/cmd/main.go#L238

вернулся, перечитал, можно не гонять вола, сделать нормальные вейтгруппы и не долбиться к синглтону логгера из горутин, а просто errgroup юзать. это

https://github.com/Reholly/de-csu/blob/a1c09a91db98ebc9a0495f2ce1abdcec442c33f9/candles/cmd/main.go#L202 ладно я не переобулся ты статично закидываешь типы, так почему не енам пгшный?

evabronskayaa commented 1 month ago

@crutchM @Reholly

Если таблица подразумевает обновление и дополнение новыми данными, то лучше оставить таблицу, выглядит она точно нормально

Если обновлений следующих не будет и будет условно всегда 3-4 значения, то можно TYPE ENUM

Обновление и добавление новых данных в ENUM достаточно стремное, потому что надо делать DDL. А некоторые СУБД при изменении структуры данных становятся недоступными на время изменений

В рамках данного проекта не так важно что это будет: таблица или ENUM НО если задумываться о практической ценности, то обычно другие участники команды не знают про ENUM и им проще что-то более примитивное, но это на вкус и цвет

https://github.com/Reholly/de-csu/blob/a1c09a91db98ebc9a0495f2ce1abdcec442c33f9/candles/storage/init.sql#L20