# Collections code review
## Disclaimer:
до конца не разобрался в архитектуре, могу ошибаться с заключениями
некоторые моменты субъективные
***
## ENGLISH
your -> you
devided -> divided, и я бы заменил 'stamps sets' -> 'sets'
не успел заскриншотить - в туториале при открытии пака "all collected stamps will show here" 'will show' -> 'will be shown'
"Sorry, new pack is not avaliable...": 'are short of'->'are out of '
"Play your favourite games to collect the stamps!": 'favourite'(BrE)->'favorite'(AmE)
"River reserves\ntheright" 'theright'->'the right'
***
***
## CODE
### CollectionsComplexTypes
название `vsdk::TutorialLayer`, `vsdk::TutorialData` выглядят слишком обобщённо - может привести к конфликтам и непониманию. Возможно лучше `CollectionsTutorial...`? Либо же для Collections создать под-namespace `vsdk::collections::TutorialLayer`?
***
```cpp=
TutorialData(
std::string&& identity_prefix_,
std::string&& seen_tag_,
std::string&& main_panel_name_,
std::vector<std::string>&& active_buttons_names_);
: m_identity_prefix(std::move(identity_prefix_))
, m_seen_tag(std::move(seen_tag_))
void SetLayers(std::vector<TutorialLayer>&& layers)
```
`rvalue reference` заставляет пользователя оперировать `lvalue`(что очень неудобно), и приводит к куче ошибок `error: cannot bind ‘const string {aka const std::basic_string}’ lvalue to ‘std::string&& {aka std::basic_string&&}’` если это не `lvalue`. `std::move` в таком случае бесполезно.
Можно использовать "pass by value and move".
зачем подчёркивания сзади?
`m_page_count` неинициализированна без `SetLayers()` - может привести к тонким багам
upd: а она вообще нужна? почему не пользоваться `m_layers.size()` напрямую??
***
```cpp=
bool TutorialLayer::IsShowWithDelay() const
{
return show_with_delay;
}
```
выглядит ненужным в структуре с публичными полями (+English: IsShow++n++WithDelay)
***
```cpp=
assert(dummy_panel);
assert(origin_parent);
assert(origin_panel)
```
стандартные `assert` не приведут к проблемам кросплатформенности? для них вроде специально переопределяют макросы (при чём ниже используется `VLIB_ASSERT` - позже увидел)(`VCORE_Collections` так же использует `assert`)
***
```cpp=
const auto page_count = static_cast<int>(m_layers.size());
if (page < page_count)
return m_layers.at(page);
throw std::runtime_error("TutorialData.GetLayer Out of range");
```
работа зря, так как vector::at() всё это делает сам (+для таких случаев есть std::out_of_range)
все 4 строчки равнозначны `return m_layers.at(page)`
то же и с `unordered_map`
***
### CollectionsTutorialManager
cубъективно сложно понять разницу между `SetPage(++m_current_page)` и `SetNextPage()`
***
настораживает передавание полей класса как аргументы мемберов:
```cpp=
RearrangeLayers(m_current_page, true);
TutorialData(m_tutorial_mode)
IsEndPage(m_current_page)
```
почему просто не использовать поле в тех функциях? так получается в мембере 2 копии значения, легко запутаться
***
```cpp=
, m_animation_helper(nullptr)
{
vlib::ObjectFac::Instance()->AddSource("COLLECTIONS#TUTORIAL_MANAGER", this);
m_animation_helper = new CollectionTutorialAnimationHelper;
```
первая инициализация выглядит излишней. Свой отдельный класс - не понял задумки. Почему изнутри сразу не сделать `vlib::ObjectFac::Instance()->AddSource("COLLECTIONS#TUTORIAL_ANIM_HELPER", this);`?
***
### FishingBase
с названиями как `PrepareCollectionsInGame()`, `FishingBase_AppPlugIn::m_collections` нужно поаккуратнее, так как в FishingBase есть `GameObjCollection`, `OriginalTypeCollection`, `PresetsCollection`, `ProfitCollection`, `ConfigCollection`, `FishWashMethodCollection`, `PathCollection` и ещё куча "коллекций"
***
### VCORE
`SetCollectionGetWinProcessState` - `set` и `get` в одном названии + не упомянается Pending, что собственно делается внутри
***
```cpp=
void VCORE_Collections::SetPendingWinProcessState(const PendingWinData& win_data, bool state) {...
auto condition = [win_data](const auto& item){...}
```
излишние копии
***
```cpp=
if (IsWelcomeEntry())
pending_wins_count += 1; //TODO: check with the server
```
unresolved TODO
***
### AbstractCommonFeature
методы `DoSomething()/OnDoSomething()` меня лично путают. Если мне нужна кастомная логика - мне нужно перегружать `OnDoSometing()` - другие не виртуальные. Но у меня так же есть паблик метод `DoSomething()`...
cубъективно намного более очевидная логика как в `AbstractEffect::Run()` - флаг устанавливается в родителе, я в override просто вызываю родителя. Нету OnRun().
***
поля `m_...` приватные, то есть недоступные в детях - задумано обращаться *только* через get/set?
***
### PopupManager
слишком много responsibility в одном классе: он и с сервером общается, и управляет открытием паков, и слушает кнопки, и управляет визуалами
***
### CollectionsHelper
ProcessUpdatePanelImgErrors не делает ничего - в любом случае выводится одно и то же сообщение
***
### CollectionsManager
```cpp=
vcore::VCORE_Collections& CollectionsManager::GetDataManager() const
{
return vcore::VCORE_Account::Instance()->GetCollectionsManager();
}
```
много названий одного и того же: `VCORE_Collections`, `m_collections_manager` - но он используется как `DataManager` в `vsdk::CollectionsManager`
***
у `vsdk::CollectionsManager` слишком много responsibility. Он наследуется от *фичи*(?? плюс это единственный ребёнок) и может открываться/закрываться(?). В то же время слушает кнопки, билдит другие классы, отправляет `pending_win` на сервер, обновляет UI.
Sign in with Wallet
Connect another wallet