# 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, но тут дело вкуса
Sign in with Wallet
Connect another wallet