Фидбек по конкурсной работе от проверяющих

[b1ng0o]

Фидбек №1

сборка простая (есть taskfile и минимальный makefile), но нет документации как собрать проект без этих посторонних средств (например на windows)

не go-way обработка ошибок
паники по которым не понятно что пошло не так, если не идти смотреть в код
есть хардкод и опции которые нельзя никак сконфигурировать/поменять без перекомпиляции
использование божественных классов
много синглтонов. Рекомендуется использовать меньше хардкода.

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

преимущественно технические ошибки не игнорируются, но некоторые ошибки бизнес-логики кажется являются критичными, но не логируются

стратегии можно подкрутить через конфиг либо вовсе отключить, если нужно. Не хватает описание методов документации

Фидбек №2

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

Встречаются длинные названия для короткоживущих объектов и однобуквенные названия для долгоживущих (иногда глобальны) переменных. Бизнес-логика зависит от внешней инфраструктуры и third-party библиотек (нарушение dependency inversion).

[Antonboom]

Здравствуйте!
Отвечу для истории.

нет документации как собрать проект без этих посторонних средств

Согласен, не думал, что строчку 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 +0x224

panic: 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.Client
• services/portfoliowatcher.Watcher
• services/toolscache.Cache
• strategies/bullsbearmon.Strategy
• strategies/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]

Прекрасная работа. И даже работает )

Единственное что не нравится это сорцы в internal/ . Как раз под руку такая статья подвернулась по этой теме:
https://ido50.net/rants/dont-put-all-your-code-in-internal

[Antonboom]

@maxxant благодарю за фидбек 🙂.

По поводу internal, это общепринятая практика (как в компаниях, в которых я работал, так и в открытых проектах), и она мне по душе.

Автор статьи выдвигает (по моему мнению) сомнительный тезис – "раз выкладываете в open-source, то и не надо ничего инкапсулировать". Видимо, он никогда не работал с сотней приложений и не встречался с адом, когда команды, не думая, начинают строить зависимости от сервисов друг друга. В своём проекте я открыто даю понять, что это закрытое самодостаточное приложение, не экспортирующее никакого API в качестве внешней библиотеки.

Тут как и с любыми другими рекомендациями (привет злосчастному golang-standards
/project-layout), нужно понимать, что ты делаешь и зачем, и не бросаться в крайности от статеек в интернете.

P.S. Недавно я и сам был недоволен архитектурой x/tools/go/analysis и internal-пакетов в нём, которые хорошо бы заэкспортировать.