---
# System prepended metadata

title: Da Vinci Diamonds code review

---

# Da Vinci Diamonds code review

!!! __не хватает header guard-ов pragma или ifndef__
!!! __класс GameHelper родительский, но не имеет ни одного виртуального метода - на что напарывается в BonusGame::GetCascadeSize()__
!! __BasicWheels/BonusWheels - полная копипаста, BasicGame/BonusGame - много копипасты. Возможно ли вынести общего предка?__
!! __CascadeWheelsHelper слишком большой и запутанный. Возможно ли повыносить методы/разбить на подклассы?__
! __В разметке непривычно видеть перемешанные Panel/Acion/Driver - привычнее сначала все Panel, потом все Action+Driver__

### BasicWheels:
1)
```cpp
BasicWheels::BasicWheels(cpp::count_ptr<CascadeWheelsHelper> wheels_helper)
{
    m_cascade_wheels = wheels_helper;
}
```
используй лист инициализации: `: m_cascade_wheels(wheels_helper)`

2) вместо комментария можно вынести метод с этим же названием
```cpp
// Set gap size to center
for (auto wheel : m_wheels)
...
```

3) в некоторых случаях используешь `<algorithm>`, даже нестандартные как `std::none_of` - a в некоторых ручками - как 
```cpp
for (auto wheel : new_symbols)
{
    if (wheel.empty())
        ++m_stopped; 
}
```
мелочь, но хотелось бы последовательности

4) так же субъективно показалось непоследовательным - `BasicWheels` и `BonusWheels` содержат в себе `CascadeWheelsHelper`, а `BasicGame` и `BonusGame` наследуются от `GameHelper`. Является ли игра `GameHelper`ом? (LSP)

### BigWinFeature
1) копипаста `BigWinFeature::ModelStateChanged` и `BigWinFeature::CounterStateChanged`

### BonusGame
1) при `dynamic_cast` полезно вставить `ASSERT` или `if(... != nullptr)`
```cpp
auto* cascade_wheels = dynamic_cast<BonusWheels*>(wheels.get());
cascade_wheels->StartCascade(m_destroy_series_symbols, new_symbols);
```

### BonusInBonus
1) есть метод `SetActionEndHandler`
```cpp    
auto* action = vlib::ObjectFac::Instance()->LoadAction("fg.stars.explousions.fade.down");
action->SetEndHandler([this](cpp::count_ptr<gui::Action>) {  Notify(SOUND_CUSTOM_EVENT.SET_VOLUME, 100); });
```

### CascadeWheelsHelper - cамый сложный файл
1)
```cpp
enum SymID
{
    eWILD = 0,
    eBONUS = 1,
    eDIAMOND = 2,
    eMONA_LISA = 3,
    eFRANCHINUS_GAFFURIUS = 4,
    eGINEVRA_DE_BENCI = 5,
    ePURPLE_STOUNE = 6,
    eGREEN_STOUNE = 7,
    eORANGE_STOUNE = 8
};
```
STONE - опечатка. Цифры излишни, хотя перестраховаться - не страшно.

2) `Explosion_symbol` - название метода неформатированное, неинформативное, и сам метод огроменный
магические константы я бы вынес - типа `const gwc::Color ORANGE(255, 153, 51)`

3)
```cpp
const gwc::Time delay_symbol = vsdk::SlotCfg::Instance()->GetCutomData()["fakeCascadeConfig"]["delayBetweenSymbol"];
const gwc::Time delay_wheel = vsdk::SlotCfg::Instance()->GetCutomData()["fakeCascadeConfig"]["delayBetweenWheel"];
const gwc::Time duration_symbol_one_cell = vsdk::SlotCfg::Instance()->GetCutomData()["fakeCascadeConfig"]["durationSymbolOneCell"];
```
можно один раз достать json `CustomData`, избежать копипасты и лишней стены текста
( и можно отрефакторить имя `GetCuStomData()` )

4) в общем - файл и методы здоровенные
5) в нескольких местах такую конструкцию видел:
```cpp
std::string sound_name;
switch (sound_type)
{
case SoundType::eDROP_SMALL: sound_name = "drop_small"; break;
case SoundType::eDROP_MIDDLE: sound_name = "drop_middle"; break;
case SoundType::eDROP_LARGE: sound_name = "drop_large"; break;
case SoundType::eEXPLOSION_SMALL: sound_name = "expl_small"; break;
case SoundType::eEXPLOSION_MIDDLE: sound_name = "expl_middle"; break;
case SoundType::eEXPLOSION_LARGE: sound_name = "expl_large"; break;
default: break;
}
```
не проще ли вынести это в мапу, и доступаться по индексу? Так и лишнего синтаксиса switch не надо, и можно вынести данные от их обработки.

6) операторы можно соединять:
```cpp
figure_id = *new_symbol_id;
++new_symbol_id;
// figure_id = *new_symbol_id++

drop_from_pos.y = drop_from_pos.y - wheel->GetWheelView(-1)->GetSpan(gui::Axis::Y) * (new_cell_i_offset - 1);
// drop_from_pos.y -= ...
```

7) непоследовательность в переносах сигнатур методов
```cpp
CreateCascadeAction(
    const cpp::count_ptr<vgui::VGUI_Panel>& view
    , const gwc::Vec2& destination
    , const gwc::Time& fall_time
    , const gwc::Time& wait_time)
    {
```
```cpp
void CascadeWheelsHelper::AdditionalSymbols
(
    vlib::VLIB_WheelsView* wheels
    , const std::vector<vcore::VCORE_Route::Position_t>& destroy_symbols
    , const vcore::VCORE_Reels::Matrix_t& new_symbols
) {
```


### GameHelper

1) Копипаста
```cpp
bool GameHelper::IsLastCascade(const int cascade_number)
{
    const int cascade_count = vcore::VCORE_Game::Instance()->Traits()->FruitTumbling()->GetCascadeSeries().size();
    return cascade_number == cascade_count - 1;
}

const int GameHelper::GetCascadeSize()
{
    return vcore::VCORE_Game::Instance()->Traits()->FruitTumbling()->GetCascadeSeries().size();
}
```

### RulesModel, GameShell, etc.
1) такие маленькие классы можно наверное сделать header-only. Или даже подумать собрать в один файл Helpers или что-то подобное.

### SeriesProcessor
1) унифицированно не лучше ли назвать SRModel?

2) избыточные условия
```cpp
if (score > 0)
{
    if (mult >= win_small && mult < win_middle)
    {
        PLaySound(SoundType::eSMALL_WIN);
    }
    else if (mult >= win_middle && mult < win_large)
    {
        PLaySound(SoundType::eMIDDLE_WIN);
    }
    else if (mult >= win_large)
    {
        PLaySound(SoundType::eLARGE_WIN);
    }
}
```
```cpp
if (score > 0)
{
    if (mult < win_middle)
    {
        PLaySound(SoundType::eSMALL_WIN);
    }
    else if (mult < win_large)
    {
        PLaySound(SoundType::eMIDDLE_WIN);
    }
    else
    {
        PLaySound(SoundType::eLARGE_WIN);
    }
}
```
должны ли проигрываться звуки при `mult < win_small` ? в нынешнем варианте - нет


### Разметка
1) можно ли избавиться от почти однострочных файлов - например markers_left, markers_right, move_button_spin?
как вариант, перенести их в app_plugin_decors