Antonboom
commented
2 years ago Здравствуйте! Отвечу для истории.
нет документации как собрать проект без этих посторонних средств
Согласен, не думал, что строчку go run ./cmd/trading-robot нужно выносить в документацию. Поддержка Windows – тут каюсь, последнее, что пришло бы в голову 🙂.
не go-way обработка ошибок
Здесь, видимо, проверяющему не понравилось, что я чаще использую %v вместо %w. Пусть учтёт, что %w без надобности ведёт к утечке абстракции между слоями. Там, где нужно, он используется вместе с sentinel errors, errors.Is и т.д. Где здесь не go-way – вопрос.
паники по которым не понятно что пошло не так, если не идти смотреть в код
Паники только на старте приложения, имеющие сообщение и стектрейс (на всякий случай). В основном возникают при невалидном конфиге, видимо, на это и наткнулся проверяющий. Пример паник:
Key: 'Config.Clients.TinkoffInvest.Address' Error:Field validation for 'Address' failed on the 'required' tag
goroutine 1 [running]:
log.Panic({0x14000593d98?, 0x100c10788?, 0x140001a4000?})
/usr/local/go/src/log/log.go:385 +0x68
main.mustNil(...)
/Users/anthony/tinkoff-invest-robot-contest-2022/cmd/trading-robot/main.go:164
main.main()
/Users/anthony/tinkoff-invest-robot-contest-2022/cmd/trading-robot/main.go:45 +0x224panic: unauthenticated: not real clients.tinkfoff_invest.token
goroutine 1 [running]:
log.Panic({0x140000a3d98?, 0x140001aa050?, 0x102a4e3c8?})
/usr/local/go/src/log/log.go:385 +0x68
main.main()
/Users/anthony/tinkoff-invest-robot-contest-2022/cmd/trading-robot/main.go:81 +0x9fcЧто здесь можно не понять из текста, мне неизвестно.
есть хардкод и опции которые нельзя никак сконфигурировать/поменять без перекомпиляции
Это верно, пример опций, о которых говорит проверяющий:
const (
applyingTimeout = 3 * time.Second // Один раз подстроишь и больше никогда не трогаешь.
lotsInTrade = 1 // Ограничение стратегии в первых реализациях,
) // можно было вынести в конфиг.использование божественных классов
Без конкретики тоже неясно. Типов в боте не так много:
tinkoffinvest.Clientservices/portfoliowatcher.Watcherservices/toolscache.Cachestrategies/bullsbearmon.Strategystrategies/spreadparasite.Strategy
Стратегии просты как пробка "слушай изменения и применяй к ним логику стратегий", сервисы тоже. Скорее всего речь идёт о клиенте к Invest API, который я не стал дробить на N типов, т.к. не увидел в этом необходимости – физическое разделение происходит с помощью файлов / конкретных интерфейсов:
internal/clients/tinkoffinvest
├── adapters.go
├── adapters_test.go
├── api_balance.go
├── api_instruments.go
├── api_order_book.go
├── api_order_cancel.go
├── api_order_place.go
├── api_order_state.go
├── api_portfolio.go
├── api_user_info.go
├── client.go
├── helpers.go
├── helpers_test.go
├── pb
└── types.goмного синглтонов. Рекомендуется использовать меньше хардкода.
Синглтонами в проекте являются только логер и prometheus-коллекторы. И то и то допустимо.
преимущественно технические ошибки не игнорируются, но некоторые ошибки бизнес-логики кажется являются критичными, но не логируются
Если речь идёт о
if err != nil {
if errors.Is(err, tinkoffinvest.ErrNotEnoughStocks) {
return nil
}
return fmt.Errorf("place limit sell order: %v", err)
}То подобные ошибки не логируются, т.к. возникают очень часто. Бот то тратит деньги/ценные бумаги, то набирает их, часто находясь на грани (проблема неоптимальных стратегий). Предполагал, что за подобным лучше следить через алерты в графане.
Не хватает описание методов документации
Стратегии и неочевидные места документированы, а остальной код говорит сам за себя. Не спорю, при желании можно документировать всё и вся 🤷♂️.
Сигнатуры конструкторов имеют много параметров, настройки стратегии можно хранить в одной структуре и передавать её
Можно, но посчитал, что пять аргументов – это ещё немного 🙂 Зависит от стайлгайда конкретного проекта и компании. Неужели это сложно читать и обёртка в структуру принципиально что-то изменит?
func New(
account tinkoffinvest.AccountID,
ignoreInconsistent bool,
tools []ToolConfig,
orderPlacer OrderPlacer,
toolsCache ToolsCache,
) (*Strategy, error) {
}
// ...
strategy, err := bullsbearsmon.New(
tinkoffinvest.AccountID(cfg.Account.Number),
bbMonCfg.IgnoreInconsistent,
toolConfs,
tInvestClient,
toolsCache,
)Иногда рядом группируются несвязные вещи, хорошо их разделять хотя бы визуально (перенос строки).
Сложно без конкретики. Может это наброс к константам выше? 🤷♂️
Встречаются длинные названия для короткоживущих объектов и однобуквенные названия для долгоживущих (иногда глобальны) переменных.
Исхожу из того, что не всегда подобное разделение верно. Переменная может быть короткоживущей, но из-за её однобуквенности вы всё равно не поймёте, что она значит и наоборот. Поэтому важно искать баланс. Видимо, у меня его найти не вышло.
Примеры из проекта:
type l = prometheus.Labels // l for labels
// ...
tradedLots.With(l{"lots_type": lotsTypeForBuy, "figi": change.FIGI.S()}).Set(float64(buys))for _, t := range tools {
if _, ok := confs[t.FIGI]; ok { // t for tool
// ...executedPrice, err := s.orderPlacer.WaitForOrderExecution(ctx, s.account, orderID)
// ...
p := executedPrice.Div(decimal.NewFromInt(int64(conf.stocksPerLot) * lotsInTrade)) // p for price.Бизнес-логика зависит от внешней инфраструктуры и third-party библиотек (нарушение dependency inversion).
Сомнительное замечание, т.к. в этом и цимес реализаций стратегий и клиента к Invest API, что ни в них, ни в тестах к ним вы не встретите импорта протобафных пакетов (которые часто пронизывают подобные проекты, порождая утечки абстракции). Единственный thrid-party тип на весь проект – это decimal.Decimal, но строить обёртки над ним особого смысла не имеет.
Если же речь идёт о том, что хотелось бы, чтобы бизнес-логика была "чистой" коробочкой, принимающей определённые сигналы и выдающей их – то да, это один из многих вариантов реализации стратегии, но не факт, что самый правильный.
Таким образом, на будущее прошу проверяющих быть более конкретными и по возможности избегать вкусовщину. Благодарю!
maxxant
Фидбек №1
сборка простая (есть taskfile и минимальный makefile), но нет документации как собрать проект без этих посторонних средств (например на windows)
не go-way обработка ошибок паники по которым не понятно что пошло не так, если не идти смотреть в код есть хардкод и опции которые нельзя никак сконфигурировать/поменять без перекомпиляции использование божественных классов много синглтонов. Рекомендуется использовать меньше хардкода.
для визуализации используется графана с заранее созданными дашбордами
преимущественно технические ошибки не игнорируются, но некоторые ошибки бизнес-логики кажется являются критичными, но не логируются
стратегии можно подкрутить через конфиг либо вовсе отключить, если нужно. Не хватает описание методов документации
Фидбек №2
Сложно читать код. Сигнатуры конструкторов имеют много параметров, настройки стратегии можно хранить в одной структуре и передавать её. Иногда рядом группируются несвязные вещи, хорошо их разделять хотя бы визуально (перенос строки).
Встречаются длинные названия для короткоживущих объектов и однобуквенные названия для долгоживущих (иногда глобальны) переменных. Бизнес-логика зависит от внешней инфраструктуры и third-party библиотек (нарушение dependency inversion).