---
# System prepended metadata

title: Gold Miner code review

---

# Gold Miner code review

1) `Utilities` - круто и аккуратно, возьму себе идею

2) `class LockItLinkBonus` слишком громоздкий и непростой. Возможно ли выделить подклассы, которым делегировать логические части (Single responsibility)? Например, `LockItLinkWheelsManager`, `LockItLinkAnimationsManager`, `LockItLinkCfgManager`.
   Не совсем понятна роль `LockItLinkBonusUtils` : например `StartRollingWheels()`, `OnWheelsStop()` находятся в Bonus, а `ResetBasicAndBonusWheels()`, `InitLockedSymbolsOnWheel()` - в Utils.
   Почему `LockItLinkBonusUtils` находится за пределами `utilities`?
    Возможно ли сгрупировать десятки мемберов в логически консистентные структуры?
```cpp=
int m_current_block_index = 0;
int m_cfg_show_border_delay = 0;
int m_cfg_delay_xw_model = 0;
int m_cfg_wait_before_delay = 0;
int m_cfg_delay_lock_animation = 0;
int m_cfg_delta_final_animation = 0;
gwc::Time m_cfg_show_light_final = 0;
gwc::Time m_cfg_show_scale_delay = 0;
double m_cfg_fly_animation_scale_up = 0;
double m_cfg_multiplier_move_speed = 0;
int m_cfg_multiplier_spawn_delay = 0;
int m_cfg_delay_run_finish_animation = 0;
gwc::Coord m_cfg_offset_shine_multiplier_x = 0;
gwc::Coord m_cfg_offset_shine_multiplier_y = 0;
```
комментарии выглядят лишними
```cpp=
{ 1, { 2 }},      //case 1
{ 2, { 1, 2, 3 }},//case 2
{ 3, { 2 }},      //case 3
{ 4, { 2 }},      //case 4
{ 5, { 1 }},      //case 5
{ 6, { 3 }},      //case 6
{ 7, { 1, 3}},    //case 7
{ 8, { 3 }},      //case 8
{ 9, { 1 }},      //case 9
{ 10,{ 2 }}       //case 10
```
выигрывают ли эти методы что-то?
```cpp=
int GetLockedSymbolsBlocksSize()
{
    return vcore::VCORE_Game::Instance()->Traits()->LoteriaLaCorona()->GetLockedSymbolsBlocks().size();
}

int GetRowIndexForOffset()
{
    return vcore::VCORE_Game::Instance()->Traits()->LoteriaLaCorona()->GetIndexRowOffset();
}

bool LockItLinkBonus::IsRecoveryGame() const
{
    return vcore::VCORE_Game::Instance()->Traits()->LoteriaLaCorona()->IsRecoveryGame();
}
```
особенно учитывая что ниже всё равно вызываются длинные цепочки
```cpp=
if (m_show_text_on_crown || m_first_call_bonus)
    return vcore::VCORE_Game::Instance()->Traits()->LoteriaLaCorona()->GetLockedBlock();

if (m_offset_animation_call_bonus)
    return vcore::VCORE_Game::Instance()->Traits()->LoteriaLaCorona()->GetLockedSymbols();

const auto& locked_blocks = vcore::VCORE_Game::Instance()->Traits()->LoteriaLaCorona()->GetLockedSymbolsBlocks();
```

Тернарки: я бы заменил 1 на 2:
```cpp
1) return (extra_games) ? false : true;
2) return extra_games == 0;

1) (m_first_call_bonus) ? GetCurrentSymbolsVector(false) : GetCurrentSymbolsVector(true);
2) GetCurrentSymbolsVector(!m_first_call_bonus)
```
магические константы посреди большого файла, которые могут много геморроя принести после правок спайна - лучше выносить куда-то, хотя бы в анонимный namespace
```cpp=
if (track_name == "holdenlink_show") //388 строчка
else if (track_name == "holdenlink_hide") //431 строчка
```
`dynamic_cast`ы лучше проверять на успех (`assert ptr != nullptr`)
```cpp=
cpp::count_ptr<gui::Wait> wait_before_move = dynamic_cast<gui::Wait*>(vlib::ObjectFac::Instance()->LoadAction(buffer));
wait_before_move->SetDuration(delay_before_move);
```


3) 
    `count_*`  = посчитать что-то (глагол)
    `*_count` = количество чего-то (существительное)
    
    `RepeatAnimation` = повторить (глагол, но используется как сущ)
    `RepeatedAnimation`, `LoopedAnimation` = зацикленная (существительное)
    в коде много первого, хотелось бы второе
    
4) `throw "Gold Miner | BonusEnteringModel::MousePressed. Unexpected button id: " + button->GetIdentity();` - в чём преимущество перед `ENGINE_LOG_ERROR`? Насколько я понял исключение нигде не перехватывается для обработки. 
    Если всё же есть смысл в исключении - наверное лучше бросать объект, а не строку. (как чуть позже `throw std::logic_error("GoldMiner.BasicGame.GetRepeatAnimationName. Unexpected symbol_id: " + std::to_string(symbol_id));`)
    
5) ` const auto& bang_positions = []() -> BonusBangAnimation::BangPositions` - мгновенный вызов лямбды - неоправданно запутанно как по мне. Обычного блока кода достаточно.

6) `m_counter_view{ nullptr }` `m_bonus_game_selected = false` `m_mult_animation_activated{ true }` - хотелось бы последовательности и единого стиля дефолтов в hpp. Например, `m_bonus_game_selected{ false }`

7) я бы заменил 1 на 2
```cpp
1) bla = series_animation_duration < 1000 ? 1001 : series_animation_duration;
2) const gwc::Time MIN_DURATION = 1000; bla = std::max(MIN_DURATION, series_animation_duration);
```

8) не совсем логично выглядят вертикальные отступы (пустые строки)
```cpp=
const auto element = series->GetElement(i);
auto movie_name = element->GetMovieName();

auto movie = vgui::ResourceManager::Instance()->GetMovie(movie_name);

auto duration = movie->GetTotalTime();
tmp = cpp::max(tmp, duration);
```

```cpp=
Notify(GOLD_MINER_HOLD_N_LINK_BONUS.SET_SPIN_CONTINUE);

Notify(GOLD_MINER_HOLD_N_LINK_BONUS.START_CARDS_SHINE_ANIMATION);

if (m_offset_animation_call_bonus)
{
    m_offset_animation_call_bonus = false;

    ShowLockedSymbolsOnWheels();

    LockItLinkBonusUtils::InitMultTextScattersPosition(true, m_offset_animation_call_bonus, GetRowIndexForOffset());

    SetScatterMultiplierFromLockedSymbols();

    LockItLinkBonusUtils::InitLockedSymbolsOnPositions();

    LockItLinkBonusUtils::ResetLockedSymbols();

    LockItLinkBonusUtils::ShowLockedBlockMultipliers();
...
    LockItLinkBonusUtils::HideOriginSymbols(locked_symbol.pos.col, locked_symbol.pos.row);
    const auto& wheel = GetBonusWheelNum(locked_symbol.pos.col, locked_symbol.pos.row);
    m_wheels->GetWheel(wheel)->HideSymbol(0);
```

9) хотелось бы поразбивать большие методы типа
```cpp=
void SeriesProcessor::UpdateColormodeForNoActiveCells(bool use_normal_mode)
{
    if (!m_abstract_wheels || m_noactive_color_mode.IsEqualRGBA(m_normal_color_mode))
        return;

    const auto wheel_count = m_abstract_wheels->GetWheelCount();
    for (int wheel = 0; wheel < wheel_count; ++wheel)
    {
        const auto visible_count = m_abstract_wheels->GetWheel(wheel)->GetVisibleCount();
        for (int row = -1; row < visible_count; ++row)
        {
            if (GameUtilities::IsHoldLinkBonus())
            {
                const auto& fig_id = m_abstract_wheels->GetWheel(wheel)->GetContents(row);
                if (fig_id >= GameUtilities::GetHoldSymbolIdWithMult())
                {
                    const auto& row_index_for_offset = vcore::VCORE_Game::Instance()->Traits()->LoteriaLaCorona()->GetIndexRowOffset();

                    if (row_index_for_offset != -1)
                    {
                        if (row_index_for_offset == row)
                            continue;
                    }
                    else
                        continue;
                }
            }

            if (use_normal_mode)
                m_abstract_wheels->GetWheel(wheel)->GetWheelView(row)->SetRGBAMode(m_normal_color_mode);
            else if (row < 0 || row >= visible_count || !m_overlay_movie_panels[wheel][row]->GetImage())
                m_abstract_wheels->GetWheel(wheel)->GetWheelView(row)->SetRGBAMode(m_noactive_color_mode);

            if (auto* label_with_multiplier = vlib::ObjectFac::Instance()->LoadLabel(fmt::format("scatter.text.W{}#R{}", wheel, row)))
            {
                if (use_normal_mode || !m_preprocess_finished)
                    label_with_multiplier->SetRGBAMode(m_normal_color_mode);
                else
                    label_with_multiplier->SetRGBAMode(m_noactive_color_mode);

                label_with_multiplier->SetAlpha(255);
            }
        }
    }
}
```
что-то в духе
```cpp=
void ::UpdateColorMode(use_normal_mode)
{
    if (...) return;
    for (wheel in wheels)
        for (row in rows)
            UpdateColorModeForCell(wheel, row, use_normal)
}

void ::UpdateColorModeForCell(wheel, row, use_normal)
{
    if (GameUtilities::IsHoldLinkBonus())
        UpdateColorModeForCellInHoldLink(wheel, row)
    
    UpdateColorModeForView(wheel, row, use_normal)
    UpdateColorModeForLabel(wheel, row)
}

void ::UpdateColorModeForView(wheel, row, use_normal) {...}

void ::UpdateColorModeForLabel(wheel, row) {...}
```
можно даже подумать чтобы вынести это в отдельный подкласс (Single Responsibility)

10) долго думал над результатом `Vec2-Vec2+Coord*double` - потом понял, что `size` и `SIZE`, а так же `anchor_px` и `ANCHOR` - разные вещи. Не самый очевидный код
```cpp=
auto pos = overlay_panel->ConvertAbsPosToParentSpace(overlay_panel->GetAnchorAbsPos());
const auto& anchor_px = overlay_panel->GetAnchorInPixels();
const auto& span = overlay_panel->GetSpan();

const gwc::Coord& SPAN = 480;
constexpr auto ANCHOR = 0.5;
pos = pos - anchor_px + span * ANCHOR;
```

11) в разметке непривычно видеть смешанные Panel и Action+Drivers, но тут дело вкуса