# 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
Sign in with Wallet
Connect another wallet