From e07a9c8167eb3186a26a6c060b8246aa429abfb8 Mon Sep 17 00:00:00 2001 From: Dmitry Vorobyov Date: Sun, 11 Jan 2026 20:05:10 +0100 Subject: [PATCH] review --- Cpp/ncurses/tasks/2/game/arrow.cpp | 10 ++++----- Cpp/ncurses/tasks/2/game/arrow.h | 10 ++++----- Cpp/ncurses/tasks/2/game/game.cpp | 35 +++++++++++++++++++++--------- Cpp/ncurses/tasks/2/game/game.h | 11 +++++----- Cpp/ncurses/tasks/2/game/main.cpp | 14 ++++++------ 5 files changed, 47 insertions(+), 33 deletions(-) diff --git a/Cpp/ncurses/tasks/2/game/arrow.cpp b/Cpp/ncurses/tasks/2/game/arrow.cpp index a7687cc..c99c02c 100644 --- a/Cpp/ncurses/tasks/2/game/arrow.cpp +++ b/Cpp/ncurses/tasks/2/game/arrow.cpp @@ -1,9 +1,9 @@ #include -#include +#include // снова сишная stl #include "arrow.h" -Arrow::Arrow(int ColorId) +Arrow::Arrow(int ColorId) // тут тоже можно попробовать инициализировать поля через список инициализации { pos.y = rand() % (getmaxy(stdscr) - 1) + 1; if(pos.y % 2 != 0) @@ -14,11 +14,11 @@ Arrow::Arrow(int ColorId) dir = RIGHT; this->ColorId = ColorId; - int main_color = rand() % 3; + int main_color = rand() % 3; // не вижу, что main_color изменяется, поэтому хочется const (я еще люблю идиому ААА Almost Always Auto, чтобы избежать неожиданных кастов, а тип видно во всех IDE, хотя часто знание конкретного типа необязательно, при наличии говорящего имени переменной) for(int i = 0; i < 3; i++) { if(i == main_color) - Color[i] = 1000; + Color[i] = 1000; // 1000 - magic number else Color[i] = rand() % 1000; } @@ -44,7 +44,7 @@ void Arrow::Move() case DOWN: pos.y++; break; } ValidNewPos(); -// mvaddch(last_pos.y, last_pos.x, ' '); +// mvaddch(last_pos.y, last_pos.x, ' '); // зобми код? attron(COLOR_PAIR(ColorId)); mvaddch(pos.y, pos.x, ch); attroff(COLOR_PAIR(ColorId)); diff --git a/Cpp/ncurses/tasks/2/game/arrow.h b/Cpp/ncurses/tasks/2/game/arrow.h index 2f0e271..a749de3 100644 --- a/Cpp/ncurses/tasks/2/game/arrow.h +++ b/Cpp/ncurses/tasks/2/game/arrow.h @@ -1,8 +1,8 @@ - -class Arrow +// #pragma once +class Arrow // тут тоже можно pimpl добавить { private: - struct Pos + struct Pos // мне нравится всегда полные названия давать сущностям чтобы избежать любого недопонимания { int y, x; }; @@ -23,7 +23,7 @@ class Arrow int ChangeDirChance = 3; int ColorId; - int Color[3]; + int Color[3]; // std::array void ValidNewPos(); @@ -32,7 +32,7 @@ class Arrow public: Arrow(int ColorId); void Tick(); - Pos GetPos() {return pos;} + Pos GetPos() {return pos;} // ради однообразия, реализацию можно было бы перенести в cpp void SetDir(int dir) {this->dir = dir;} }; diff --git a/Cpp/ncurses/tasks/2/game/game.cpp b/Cpp/ncurses/tasks/2/game/game.cpp index cf7840c..8f9baee 100644 --- a/Cpp/ncurses/tasks/2/game/game.cpp +++ b/Cpp/ncurses/tasks/2/game/game.cpp @@ -5,7 +5,7 @@ Game::Game() { - WinBox.height = 3; + WinBox.height = 3; // эти штуки можно инициализировать через список инициализации: Game::Game() : WinBox(getmaxy(stdscr) - WinBox.height) / 2, getmaxx(stdscr) - WinBox.weight) / 2, 3, 3) WinBox.weight = 3; WinBox.y = (getmaxy(stdscr) - WinBox.height) / 2; @@ -18,7 +18,7 @@ Game::Game() } } -Game::~Game() +Game::~Game() // почитай про rule of 5. Если пользоваться умными указателями, то деструктор тут даже не понадобится { for(Arrow* arrow : arrows) { @@ -26,7 +26,7 @@ Game::~Game() } } -void Game::Tick() +void Game::Tick() // я бы посмотрел как можно побить этот метод на несколько, т.к. тут и вывод на экран, и условия победы, и еще какие-то условия определяются. Есть подозрение на нарушение SRP { if(tick < 0) tick = 0; @@ -42,11 +42,12 @@ void Game::Tick() for(auto it = arrows.begin(); it != arrows.end();) { (*it)->Tick(); - if(CheckWin(*it)) + if(CheckWin(*it)) // это можно заменить на std::erase(std::remove_if(...)) { arrowsLeft--; delete *it; - arrows.erase(it); + /* it = */ arrows.erase(it); // если тут бы были unique_ptr, то earse() бы дернул деструктор unique_ptr, а тот - почистил свои ресурсы. + // ^ без этого присваивания итератор станет невалидным } else ++it; @@ -59,7 +60,7 @@ void Game::Tick() if(last_size.x != size.x || last_size.y != size.y) { for(int y = WinBox.y; y < WinBox.height + WinBox.y; y++) - for(int x = WinBox.x; x < WinBox.weight + WinBox.x; x++) + for(int x = WinBox.x; x < WinBox.weight + WinBox.x; x++) // сложность n^2 тут устраивает? { mvaddch(y, x, ' '); refresh(); @@ -83,24 +84,32 @@ void Game::Tick() nodelay(stdscr, FALSE); napms(200); getch(); - this->~Game(); + this->~Game(); // как-то странно выглядит вызов своего же деструктора } napms(1); } -bool Game::CheckWin(Arrow* arrow) +bool Game::CheckWin(Arrow* arrow) // я бы тут ожидал видеть const Arrow &, т.к. arrow тут не меняется. Еще можно метод сделать константным { - int y, x; + int y, x; // сразу бы и задать значения `const auto y = arrow->GetPos().y;` y = arrow->GetPos().y; x = arrow->GetPos().x; + // можно одним ретерном тут обойтись (с условием мог ошибиться, но идея, думаю, понятна) + // return true + // && y >= WinBox.y + // && y <= WinBox.y + WinBox.height - 1 + // && x >= WinBox.x + // && x <= WinBox.x + WinBox.weight - 1; + if(y < WinBox.y || y > WinBox.y + WinBox.height - 1) return false; if(x < WinBox.x || x > WinBox.x + WinBox.weight - 1) return false; return true; + } @@ -118,6 +127,12 @@ void Game::PrintDisplay() { for(int x = 0; x < getmaxx(stdscr); x++) mvaddch(0, x, ' '); + + // если сразу задать значение для display, то можно сделать строку константной, а константность - это хорошо. + // const auto display = std::string("Move left: ") + std::to_string(MoveLimit - MoveCount) + + // " Input left: " + std::to_string(InputLimit - InputCount) + + // " Arrow left: " + std::to_string(arrowsLeft); + std::string display; display += "Move left: "; display += std::to_string(MoveLimit - MoveCount); @@ -172,7 +187,7 @@ void Game::UpdateInput() { if(input >= 0) { - if(tick - delayTick > delay) + if(tick - delayTick > delay) // это условие можно объединить с условием выше { for(Arrow* arrow : arrows) arrow->SetDir(input); diff --git a/Cpp/ncurses/tasks/2/game/game.h b/Cpp/ncurses/tasks/2/game/game.h index e714c42..43e3b84 100644 --- a/Cpp/ncurses/tasks/2/game/game.h +++ b/Cpp/ncurses/tasks/2/game/game.h @@ -7,20 +7,19 @@ struct Box { int y; int x; - int weight; + int weight; // опечатка или это вес? int height; }; - -class Game +class Game // тут можно вынести много чего, если не все поля и методы, в pimpl (почитай про эту идиому) { -private: - enum direction +private: // кмк, удобнее когда сверху публичные методы и поля, а приватные - внизу + enum direction // дучще использовать enum class, т.к. enum - это просто int { LEFT, RIGHT, UP, - DOWN + DOWN // в подобных случаях можно ставить запятую после DOWN чтобы уменьшить дифф при добавлении еще одной строки в enum }; struct Size diff --git a/Cpp/ncurses/tasks/2/game/main.cpp b/Cpp/ncurses/tasks/2/game/main.cpp index 5ba1a40..5f2d919 100644 --- a/Cpp/ncurses/tasks/2/game/main.cpp +++ b/Cpp/ncurses/tasks/2/game/main.cpp @@ -1,5 +1,5 @@ #include -#include +#include // это же сишная либа, лучше использовать плюсовую в плюсовом коде #include #include "game.h" @@ -7,16 +7,16 @@ int main() { - srand(time(NULL)); + srand(time(NULL)); // <- nulptr initscr(); noecho(); curs_set(0); - nodelay(stdscr, TRUE); + nodelay(stdscr, TRUE); // TRUE - это макрос, булевый true нельзя использовать? keypad(stdscr, TRUE); use_default_colors(); start_color(); - Game* game = new Game; + Game* game = new Game; // что если просто на стеке создать? int input; while(game) @@ -27,13 +27,13 @@ int main() game->HandleInput(input); switch(input) { - case 'q': + case 'q': // я бы попробовал сделать это через enum class case 'Q': case ' ': case 27: delete game; break; } } - delete game; + delete game; // как-будто бы тут может быть двойное удаление, т.к. на строке 33 game так же удаляется endwin(); -} +} \ No newline at end of file