Saper w C++ i SFML

0

Napisałem sapera dla treningu, macie jakieś wskazówki co kodu?
Mam całkiem niezłą wiedzę z algorytmiki/struktur danych jak na entry level, ale wydaje mi się, że OOP/OOD kuleje, próbować dostać się na staż studencki/working student, czy jeszcze wstyd?
https://github.com/speedAde/Minesweeper---SFML
Nie zwracałem uwagi na na względy estetyczne.

2

Ja pod koniec swojego technikum klepnąłem taką gierkę w C++/SFML/Box2D: https://www.indiedb.com/games/pixzen i przyznam szczerze że na niewiele mi się ona zdała. Wydaje mi się że szukając stażu w korporacji lepiej mieć w portfolio jakąś aplikację webową w Javie/Springu niż gierkę w niszowej technologii.

4
  1. Brakuje Makefile albo CMakeLists.txt albo chociaż krótkiej wzmianki w readme, że "wystarczy zrobić $CC main.cpp".

  2. void Tile::setFlag()
    {
    if (isFlag == false)
        isFlag = true;
    else
        isFlag = false;
    }

    Tego if-else idzie zastąpić prostym isFlag = !isFlag.

  3. class WindowDrawer
    {
    private:
    const unsigned TILE_SIZE = 21;
    std::array<sf::Texture, 3> buttonTextures;
    std::array<sf::Texture, 12> textures;

    Do niektórych rzeczy masz consty, do innych wrzucasz magic numbers.

  4. class WindowDrawer
    {
    private:
    bool isGameLost;
    bool isGameWon;

    Te dwa boole mnie trochę smrodzą, bo w końcu gra nie może być wygrana i przegrana jednocześnie. W klasie Tile masz dwa boole: isFlag oraz isBomb - tam ma to sens, bo oflagowanie oraz zawieranie bomby są od siebie niezależne. Tutaj stworzyłbym zamiast tego jakiś trójwartościowy enum i w nim przechowywał wynik gry. Dla wygody można wtedy isGameLost przerobić na metodę zwracającą boola.

2

bool LogicManager::clickOnATile(const sf::Vector2i& mousePosition, int mouseButtton) // 0/1 = PPM/LPM

Objaśniający komentarz to znak, że nie udało Ci się wyrazić czegoś za pomocą kodu, poza tym komentarze prędzej czy później powodują problemy. Zamiast tego stwórz lepiej enum class z wartościami typu LeftButton i RightButton.

W LogicManager.h wrzuciłeś kod (definicje) dla struktury TileToChange. Powinien być w pliku .cpp, a w pliku .h tylko deklaracje.

bool Tile::getIsBomb()
Moim zdaniem lepsza nazwa to isBomb.

bool Tile::canBeRevealed()
{
    if (isFlag || wasRevealed) return false;
    return true;
}

Mnie trochę zmylił ten if. Czytelniej będzie, jeśli pojedyncze instrukcje w ifie będziesz umieszczał pod nim, z wcięciami, czyli:

if (isFlag || wasRevealed) 
    return false;

for (int i = 0; i < 12; ++i)
Nie wiadomo czym jest 12, a jeśli wiadomo to trzeba to wyciągać z kontekstu kodu. Magic numbery powinieneś opakowywać w zmienne objaśniające.

if (!isGameLost && !isGameWon)
Co to w ogóle jest?:P A już mówiąc o takich pojedynczych warunkach to zamiast robić negację !isGameWon warunek powinien brzmieć isGameLost, jest to wtedy bardziej zrozumiałe, ale mieszanki w postaci tych dwóch to nie rozumiem. Jeśli to wynik jakichś modyfikacji i zaśmiecania kodu to powinieneś to jak najszybciej wywalić.

Fragment z void WindowDrawer::checkMouse():

localPosition = sf::Mouse::getPosition(window);
    if (localPosition.y > TILE_SIZE * mapSize && localPosition.y > 0)
    {
        if (localPosition.y < TILE_SIZE * mapSize + 30)
        {
            if (localPosition.x >= 0 && localPosition.x <= TILE_SIZE * mapSize)
            {
                if (sf::Mouse::isButtonPressed(sf::Mouse::Left))
                    restartGame();
            }
        }
    }

Takie drabiny ifów są niedopuszczalne, a dalej w metodzie jest jeszcze gorzej. Warunki z wszystkich czterech ifów możesz umieścić w jednym i już będzie troszkę lepiej:

if(localPosition.y < TILE_SIZE * mapSize + 30 && localPosition.x >= 0 
  && localPosition.y < TILE_SIZE * mapSize + 30 
  && ... itp.)

Poza tym zauważ, że nie widać na pierwszy rzut oka co to robi, trzeba się wczytać i przeanalizować, żeby zrozumieć dokładnie jak to działa.
Każdy warunek warto opakować w boola, żeby objaśniał co ten warunek sprawdza, czyli np. mouseYPositionOnRestartButton albo coś tego typu, a cały ten blok wywalić do osobnej metody o nazwie w stylu handleRestartingGameButton.

Nazwa void WindowDrawer::checkMouse() dla całej metody też niewiele mówi, więc warto ją nazwać jakoś dokładniej.

Powinieneś ogólnie w całym swoim kodzie pousuwać takie zagnieżdżenia jak to wyżej, bo dużo tego jest. Takie bloki można wydzielić do dobrze opisującej je metody. Wszędzie, gdzie masz jakiś blok instrukcji, powinieneś go opakować w opisową nazwę, żeby każdy kto to czyta mógł od razu ogarnąć co to robi bez wczytywania się w szczegóły, Ty też po jakimś czasie zapomnisz co to robiło.

Długość pojedynczej metody też nie powinna być dłuższa niż 20 linii, a lepiej jeszcze krótsze w stylu 5 - 10.

W void WindowDrawer::drawWindow() wywołanie checkMouse(); oraz obsługa zamykania okna łamią zasadę pojedynczej odpowiedzialności, bo nie tylko rysuje okno, ale też obsługuje zdarzenia myszki i zamykania i to po cichu, bo nazwa metody mówi tylko o rysowaniu. Jeśli nie miałeś okazji to poczytaj o SOLID.

Nazwa klasy LogicSetter mogłaby być lepsza.

W readme nie ma opisane jak uruchomić tę grę.

Warto, żebyś napisał testy jednostkowe i umieścił je w projekcie.

To tak w sumie tyle po takim powierzchownym czytaniu, takiego kodu lepiej nie pokazuj w CV, bo tylko Ci zaszkodzi.

2

Nie umieszczaj głównej pętli gry w konstruktorze WindowDrawer. Konstruktor jest od tworzenia/inicjalizacji obiektów.

1 użytkowników online, w tym zalogowanych: 0, gości: 1, botów: 0