review

Cpp/ncurses/tasks/2/game/arrow.cpp

@@ -1,9 +1,9 @@
 #include <ncurses.h>
-#include <stdlib.h>
+#include <stdlib.h> // снова сишная 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));

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<int, 3>
 
 
     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;}
 };

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,15 +18,15 @@ Game::Game()
     }
 }
 
-Game::~Game()
+Game::~Game() // почитай про rule of 5. Если пользоваться умными указателями, то деструктор тут даже не понадобится
 {
     for(Arrow* arrow : arrows)
     {
         delete arrow;
     }
 }
 
-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);

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 

Cpp/ncurses/tasks/2/game/main.cpp

@@ -1,22 +1,22 @@
 #include <ncurses.h>
-#include <stdlib.h>
+#include <stdlib.h> // это же сишная либа, лучше использовать плюсовую в плюсовом коде
 #include <ctime>
 
 #include "game.h"
 
 
 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();
-}
+}