---
# System prepended metadata

title: Collections code review

---

# Collections code review

## Disclaimer:
до конца не разобрался в архитектуре, могу ошибаться с заключениями
некоторые моменты субъективные
***

## ENGLISH
![](https://i.imgur.com/MSOVC1J.jpg)
your -> you

![](https://i.imgur.com/2nCn1l4.png)
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.