6
\$\begingroup\$

enter image description here

enter image description here

I just finished a full minesweeper clone which is based on Minesweeper for Windows 95.

I used C++ and Qt to realize it. The full source code can be find in my git hub.

Since this is too much code too review, I want to focus on the logic of the cells. For that I created the following classes:

  • Cell: Represents a single cell in the Minefield
  • CellInputHandler: This QEventfilter gets installed on each Cell to manage movements and right and left clicks with the cells.
  • Minefield: Manages all the cells' events and tells the game when important events happen (bomb hit, uncovered all safe cells, uncovered first cell).

You wonder why CellInputHandler handles the events and not each individual Cell directly. Minesweeper supports the following movement which could not be directly implemented in a single cell:

  • press left mouse button on Cell A and hold it.
  • Move out of Cell A ⟶ Cell A gets "unpressed" on move out
  • Move into Cell B ⟶ Cell B gets "pressed"

In the Cells directly implemented, I could not detect moving into the Cell with a pressed Mouse Button. So an event filter was the solution. Still, I feel this is a strange solution because I have to make it a friend of Cell and friends are often a bad design choice.

So please let me know what you think about the code. Also tell me any other bad practices or improvements I could have made.

Feel free to also check the other classes in the repo. If there's anything strange, let me know.

cell.h

#ifndef CELL_H
#define CELL_H

#include <QWidget>

#include <QElapsedTimer>
#include <QTimer>

class CellInputHandler;

class Cell : public QWidget
{
    Q_OBJECT
public:
    enum class State{
        empty,
        mine
    };

    Cell(State state, QWidget *parent = nullptr);

    void setCountOfNeighbourMines(int count);
    [[nodiscard]] int countOfNeighbourMines() const;

    [[nodiscard]] bool hasMine() const;
    [[nodiscard]] bool hasQuestionmark() const;
    [[nodiscard]] bool isCovered() const;
    [[nodiscard]] bool isFLagged() const;
    [[nodiscard]] bool isPressed() const;
    [[nodiscard]] bool neighbourHasMine() const;
    [[nodiscard]] bool neighbourIsFlagged() const;

public slots:
    void toggleColor(bool value);
    void toggleNewQuestionMarks(bool value);

    void increaseCountOfFlaggedNeighbours();
    void decreaseCountOfFlaggedNeighbours();
    void uncoverIfCoveredAndNoMine();
    void uncoverIfNotFlagged();

    void pressIfCoveredOrQuestionmark();
    void releaseIfCoveredOrQuestionmarkPressed();

    void showMine();
    void setToFlaggedWrong();

signals:
    void hitMine();
    void flagged();
    void unflagged();
    void uncovered();
    void uncoveredEmptyCell();
    void uncoverAreaWithNoMines();
    void uncoverNotFlaggedNeighbours();
    void pressed();
    void released();
    void pressNeighbours();
    void releaseNeighbours();

protected:
    void paintEvent(QPaintEvent *event) override;

private slots:
    void mark();

private:
    enum class DisplayType{
        covered,
        coveredPressed,
        neigboursHave0Mines,
        neigboursHave1Mine,
        neigboursHave2Mines,
        neigboursHave3Mines,
        neigboursHave4Mines,
        neigboursHave5Mines,
        neigboursHave6Mines,
        neigboursHave7Mines,
        neigboursHave8Mines,
        questionmark,
        questionmarkPressed,
        flagged,
        mine,
        mineExploded,
        flaggedWrong,
    };

    QImage displayImage(DisplayType type);

    void uncover();
    void uncoverMine();
    void setToUncoveredDisplayType();

    friend class CellInputHandler;

    void handleMousePressEvent(QMouseEvent *event);
    void handleMouseReleaseEvent(QMouseEvent *event);

    void handleMouseMoveEventInsideLeftButton(QMouseEvent *event);
    void handleMouseMoveEventOutsideLeftButton(QMouseEvent *event);

    void handleMouseMoveEventInsideBothButtons(QMouseEvent *event);
    void handleMouseMoveEventOutsideBothButtons(QMouseEvent *event);

    const bool mHasMine;
    bool mNeighboursPressed;
    bool mQuestionMarksOn;
    bool mColorOn;
    int mCountOfNeighbourMines;
    int mCountOfNeigboursFlagged;
    QElapsedTimer mElapsedTimer;
    QTimer mSingleMouseTimerLeft;
    QTimer mSingleMouseTimerRight;
    DisplayType mDisplayType;
};

#endif // CELL_H

cell.cpp

#include "cell.h"

#include "converttograyscale.h"
#include "cellinputhandler.h"

#include <QApplication>
#include <QIcon>
#include <QPainter>
#include <QStylePainter>
#include <QStyleOptionButton>
#include <QMouseEvent>
#include <QImage>

#include <QDebug>

Cell::Cell(Cell::State state, QWidget *parent)
    :QWidget{ parent },
      mHasMine{ static_cast<bool>(state) },
      mNeighboursPressed{ false },
      mQuestionMarksOn{ true },
      mColorOn{ true },
      mCountOfNeighbourMines{ 0 },
      mCountOfNeigboursFlagged{ 0 },
      mDisplayType{ DisplayType::covered }
{
    setFixedSize(displayImage(mDisplayType).size());

    mElapsedTimer.start();

    constexpr auto intervall = 50;
    for(QTimer* timer : {&mSingleMouseTimerRight, &mSingleMouseTimerLeft}){
        timer->setInterval(intervall);
        timer->setSingleShot(true);
    }

    connect(&mSingleMouseTimerLeft, &QTimer::timeout,
            this, &Cell::pressIfCoveredOrQuestionmark);
    connect(&mSingleMouseTimerRight, &QTimer::timeout,
            this, &Cell::mark);

    setMouseTracking(true);
}

void Cell::setCountOfNeighbourMines(int count)
{
    constexpr auto minNeighbourMines = 0;
    constexpr auto maxNeighbourMines = 8;

    Q_ASSERT(count >= minNeighbourMines && count <= maxNeighbourMines);

    mCountOfNeighbourMines = count;
}

int Cell::countOfNeighbourMines() const
{
    return mCountOfNeighbourMines;
}

bool Cell::hasMine() const
{
    return mHasMine;
}

bool Cell::hasQuestionmark() const
{
    return mDisplayType == DisplayType::questionmark;
}

bool Cell::isCovered() const
{
    return mDisplayType == DisplayType::covered;
}

bool Cell::isFLagged() const
{
    return mDisplayType == DisplayType::flagged;
}

bool Cell::isPressed() const
{
    return mDisplayType == DisplayType::coveredPressed ||
            mDisplayType == DisplayType::questionmarkPressed;
}

bool Cell::neighbourHasMine() const
{
    return mCountOfNeighbourMines != 0;
}

bool Cell::neighbourIsFlagged() const
{
    return mCountOfNeigboursFlagged != 0;
}

void Cell::toggleColor(bool value)
{
    mColorOn = value;
    update();
}

void Cell::toggleNewQuestionMarks(bool value)
{
    mQuestionMarksOn = value;
}

void Cell::increaseCountOfFlaggedNeighbours()
{
    ++mCountOfNeigboursFlagged;
    Q_ASSERT(mCountOfNeigboursFlagged <= 8);
}

void Cell::decreaseCountOfFlaggedNeighbours()
{
    --mCountOfNeigboursFlagged;
    Q_ASSERT(mCountOfNeigboursFlagged >= 0);
}

void Cell::uncoverIfCoveredAndNoMine()
{
    if (hasMine() || !isCovered()) {
        return;
    }

    setToUncoveredDisplayType();
    update();

    if(!neighbourHasMine()) {
        emit uncoverAreaWithNoMines();
    }
}

void Cell::uncoverIfNotFlagged()
{
    if (isFLagged() || mDisplayType == DisplayType::flaggedWrong) {
        return;
    }

    uncover();
    update();

    if(!neighbourHasMine()) {
        emit uncoverAreaWithNoMines();
    }
}

void Cell::pressIfCoveredOrQuestionmark()
{ 
    if(mSingleMouseTimerLeft.isActive()) {
        mSingleMouseTimerLeft.stop();
    }

    if(mDisplayType == DisplayType::covered) {
        mDisplayType = DisplayType::coveredPressed;
        emit pressed();
        update();
    }
    else if(mDisplayType == DisplayType::questionmark) {
        mDisplayType = DisplayType::questionmarkPressed;
        emit pressed();
        update();
    }
}

void Cell::releaseIfCoveredOrQuestionmarkPressed()
{
    if(mSingleMouseTimerLeft.isActive()) {
        mSingleMouseTimerLeft.stop();
    }

    if(mDisplayType == DisplayType::coveredPressed) {
        mDisplayType = DisplayType::covered;
        emit released();
        update();
    }
    else if(mDisplayType == DisplayType::questionmarkPressed) {
        mDisplayType = DisplayType::questionmark;
        emit released();
        update();
    }
}

void Cell::showMine()
{
    if(hasMine()) {
        mDisplayType = DisplayType::mine;
        update();
    }
}

void Cell::setToFlaggedWrong()
{
    mDisplayType = DisplayType::flaggedWrong;
    update();
}

void Cell::paintEvent(QPaintEvent *event)
{
    Q_UNUSED(event)
    QPainter painter{ this };

    auto image = displayImage(mDisplayType);

    if(!mColorOn) {
        image = convertToGrayscale(image);
    }

    painter.drawImage(rect(), image);
}

void Cell::mark()
{
    switch (mDisplayType) {
    case DisplayType::covered:
        mDisplayType = DisplayType::flagged;
        emit flagged();
        update();
        break;
    case DisplayType::flagged:
        if(mQuestionMarksOn) {
            mDisplayType = DisplayType::questionmark;
        }
        else {
            mDisplayType = DisplayType::covered;
        }
        emit unflagged();
        update();
        break;
    case DisplayType::questionmark:
        mDisplayType = DisplayType::covered;
        update();
        break;
    default:
        break;
    }
}

QImage Cell::displayImage(Cell::DisplayType type)
{
    switch(type){
        case DisplayType::covered:
            return QImage{":/ressources/cell_covered.png"};
        case DisplayType::coveredPressed:
            return QImage{":/ressources/cell_covered_pressed.png"};
        case DisplayType::neigboursHave0Mines:
            return QImage{":/ressources/cell_0.png"};
        case DisplayType::neigboursHave1Mine:
            return QImage{":/ressources/cell_1.png"};
        case DisplayType::neigboursHave2Mines:
            return QImage{":/ressources/cell_2.png"};
        case DisplayType::neigboursHave3Mines:
            return QImage{":/ressources/cell_3.png"};
        case DisplayType::neigboursHave4Mines:
            return QImage{":/ressources/cell_4.png"};
        case DisplayType::neigboursHave5Mines:
            return QImage{":/ressources/cell_5.png"};
        case DisplayType::neigboursHave6Mines:
            return QImage{":/ressources/cell_6.png"};
        case DisplayType::neigboursHave7Mines:
            return QImage{":/ressources/cell_7.png"};
        case DisplayType::neigboursHave8Mines:
            return QImage{":/ressources/cell_8.png"};
        case DisplayType::questionmark:
            return QImage{":/ressources/cell_questionmark.png"};
        case DisplayType::questionmarkPressed:
            return QImage{":/ressources/cell_questionmark_pressed.png"};
        case DisplayType::flagged:
            return QImage{":/ressources/cell_flagged.png"};
        case DisplayType::mine:
            return QImage{":/ressources/cell_mine.png"};
        case DisplayType::mineExploded:
            return QImage{":/ressources/cell_mine_explode.png"};
        case DisplayType::flaggedWrong:
            return QImage{":/ressources/cell_nomine.png"};
    }
    return QImage{};
}

void Cell::uncover()
{
    if(hasMine()) {
        uncoverMine();
    }
    else {
        setToUncoveredDisplayType();
    }
    emit uncovered();
    update();
}

void Cell::uncoverMine()
{
    mDisplayType = DisplayType::mineExploded;
    emit hitMine();
}

void Cell::setToUncoveredDisplayType()
{
    mDisplayType = static_cast<DisplayType>(
    static_cast<int>(
        DisplayType::neigboursHave0Mines) + mCountOfNeighbourMines);
    emit uncoveredEmptyCell();
}


void Cell::handleMousePressEvent(QMouseEvent *event)
{
    if(!(event->buttons().testFlag(Qt::LeftButton) ||
         event->buttons().testFlag(Qt::RightButton))) {
        return;
    }

    if(event->buttons().testFlag(Qt::LeftButton)) {
        mSingleMouseTimerLeft.start();
    }
    else if (event->buttons().testFlag(Qt::RightButton)){
        mSingleMouseTimerRight.start();
    }

    const auto elapsedTime = mElapsedTimer.restart();

    if(elapsedTime >= QApplication::doubleClickInterval()) {
        return;
    }

    if((mSingleMouseTimerLeft.isActive() &&
        event->buttons().testFlag(Qt::RightButton)) ||
        (mSingleMouseTimerRight.isActive() &&
         event->buttons().testFlag(Qt::LeftButton))){

        if(!isPressed()) {
            pressIfCoveredOrQuestionmark();
            mNeighboursPressed = true;
            emit pressNeighbours();
        }
        for(QTimer* timer : { &mSingleMouseTimerRight,
            &mSingleMouseTimerLeft }) {
            timer->stop();
        }
    }
}

void Cell::handleMouseReleaseEvent(QMouseEvent *event)
{
    if(mNeighboursPressed) {
        if(event->button() == Qt::LeftButton ||
                event->button() == Qt::RightButton)
        {
            mNeighboursPressed = false;

            if(mCountOfNeigboursFlagged == mCountOfNeighbourMines) {
                if(isPressed()) {
                    uncover();
                }
                emit uncoverNotFlaggedNeighbours();
                emit uncoverAreaWithNoMines();
            }
            else {
                if(isPressed()) {
                    releaseIfCoveredOrQuestionmarkPressed();
                }
                emit releaseNeighbours();
            }
        }
    }
    else if(event->button() == Qt::LeftButton) {
        uncover();

        if(mDisplayType == DisplayType::neigboursHave0Mines) {
            emit uncoverAreaWithNoMines();
        }
    }
}

void Cell::handleMouseMoveEventInsideLeftButton(QMouseEvent *event)
{
    Q_UNUSED(event)

    if(!isPressed()) {
        pressIfCoveredOrQuestionmark();
    }
}

void Cell::handleMouseMoveEventOutsideLeftButton(QMouseEvent *event)
{
    Q_UNUSED(event)

    if(mSingleMouseTimerLeft.isActive()) {
        mSingleMouseTimerLeft.stop();
    }

    if(isPressed()) {
        releaseIfCoveredOrQuestionmarkPressed();
    }
}

void Cell::handleMouseMoveEventInsideBothButtons(QMouseEvent *event)
{
    handleMouseMoveEventInsideLeftButton(event);

    mNeighboursPressed = true;
    emit pressNeighbours();

}

void Cell::handleMouseMoveEventOutsideBothButtons(QMouseEvent *event)
{
    handleMouseMoveEventOutsideLeftButton(event);

    if(mNeighboursPressed) {
        mNeighboursPressed = false;
        emit releaseNeighbours();
    }
}

cellinputhandler.h

#ifndef CELLINPUTHANDLER_H
#define CELLINPUTHANDLER_H

#include <QObject>
#include <QElapsedTimer>
#include <QTimer>

class Cell;
class QMouseEvent;

class CellInputHandler : public QObject
{
    Q_OBJECT

public:
    explicit CellInputHandler(QObject *parent = nullptr);

protected:
    bool eventFilter(QObject *watched, QEvent *event) override;

private:
    void handleMouseButtonPressEvents(QObject *watched, QEvent *event);
    void handleMouseButtonReleaseEvents(QObject *watched, QEvent *event);
    void handleMouseMoveEvents(QEvent *event);

    void cellMoveInsideHandle(Cell *cell, QMouseEvent *mouseEvent);
    void cellMoveOutsideHandle(Cell *cell, QMouseEvent *mouseEvent);

    Cell *mLastCell;
};

#endif // CELLINPUTHANDLER_H

cellinputhandler.cpp

#include "cellinputhandler.h"

#include "cell.h"

#include <QApplication>
#include <QEvent>
#include <QMouseEvent>

#include <QDebug>

CellInputHandler::CellInputHandler(QObject *parent)
    : QObject{ parent },
      mLastCell{ nullptr }
{
}

bool CellInputHandler::eventFilter(QObject *watched, QEvent *event)
{
    if(event->type() == QEvent::MouseButtonPress){
        handleMouseButtonPressEvents(watched, event);
        return true;
    }
    if(event->type() == QEvent::MouseButtonRelease){
        handleMouseButtonReleaseEvents(watched, event);
        return true;
    }
    if(event->type() == QEvent::MouseMove) {
        handleMouseMoveEvents(event);
        return true;
    }
    return false;
}

void CellInputHandler::handleMouseButtonPressEvents(
        QObject *watched, QEvent *event)
{
    auto mouseEvent = static_cast<QMouseEvent*>(event);
    auto cell = qobject_cast<Cell *>(watched);

    cell->handleMousePressEvent(mouseEvent);

    mLastCell = cell;
}

void CellInputHandler::handleMouseButtonReleaseEvents(
        QObject *watched, QEvent *event)
{
    Q_UNUSED(watched)

    auto mouseEvent = static_cast<QMouseEvent*>(event);
    auto widget = QApplication::widgetAt(QCursor::pos());
    auto cell = qobject_cast<Cell *>(widget);

    if(cell) {
        cell->handleMouseReleaseEvent(mouseEvent);
        mLastCell = cell;
    }
    else if(mLastCell) {
        mLastCell->handleMouseReleaseEvent(mouseEvent);
    }
}

void CellInputHandler::handleMouseMoveEvents(QEvent *event)
{
    auto mouseEvent = static_cast<QMouseEvent*>(event);

    if(mouseEvent->buttons().testFlag(Qt::LeftButton)) {
        auto widget = QApplication::widgetAt(mouseEvent->globalPos());

        if(widget) {
            auto cell = qobject_cast<Cell *>(widget);

            if(mLastCell && (!cell || cell != mLastCell)) {
                cellMoveOutsideHandle(mLastCell, mouseEvent);
            }
            if(!cell) {
                mLastCell = nullptr;
            }
            else if(cell != mLastCell) {
                cellMoveInsideHandle(cell, mouseEvent);
                mLastCell = cell;
            }
        }
    }
}

void CellInputHandler::cellMoveInsideHandle(
        Cell *cell, QMouseEvent *mouseEvent)
{
    if(mouseEvent->buttons().testFlag(Qt::RightButton)) {
        cell->handleMouseMoveEventInsideBothButtons(mouseEvent);
    }
    else {
        cell->handleMouseMoveEventInsideLeftButton(mouseEvent);
    }
}

void CellInputHandler::cellMoveOutsideHandle(
        Cell *cell, QMouseEvent *mouseEvent)
{
    if(mouseEvent->buttons().testFlag(Qt::RightButton)) {
        cell->handleMouseMoveEventOutsideBothButtons(mouseEvent);
    }
    else {
        cell->handleMouseMoveEventOutsideLeftButton(mouseEvent);
    }
}

minefield.h

#ifndef MINEFIELD_H
#define MINEFIELD_H

#include <QVector>
#include <QWidget>

#include "cell.h"

#include <vector>

class CellInputHandler;

class Minefield : public QWidget
{
    Q_OBJECT
public:   
    Minefield(const QVector<Cell *> &cells, int width, int height,
              QWidget *parent = nullptr);

    [[nodiscard]] int fieldWidth() const;
    [[nodiscard]] int fieldHeight() const;
    [[nodiscard]] int countOfMines() const;
    [[nodiscard]] int minesLeft() const;

signals:
    void toggleColorInCells(int value);
    void toggleNewQuesionMarksInCells(int value);

    void uncoveredFirstCell();
    void uncoveredEmptyCell();
    void uncoveredAllSafeCells();

    void pressedCell();
    void releasedCell();

    void mineExploded();
    void minesLeftChanged(int minesLeft);  

private slots:
    void flaggedCell();
    void unflaggedCell();

    void checkIfFirstCellIsUncovered();
    void checkIfSafeCellsUncovered();    

private:   
    void connectWithCells();
    void addCellsToLayout();

    void showAllMines();
    void showWrongFlaggedCells();
    void disableInput();

    bool mFirstCellUncovered{ false };
    bool mSafeCellsUncovered{ false };
    QVector<Cell *> mCells;
    int mFieldWidth;
    int mFieldHeight;
    int mMinesLeft;
    CellInputHandler *mCellInputHandler;
};

#endif // MINEFIELD_H

minefield.cpp

#include "minefield.h"

#include "cell.h"
#include "cellinputhandler.h"
#include "cellutility.h"

#include <QDebug>
#include <QGridLayout>

Minefield::Minefield(const QVector<Cell *> &cells, int width, int height,
                     QWidget *parent)
    :QWidget{ parent },
      mCells{ cells },
      mFieldWidth{ width },
      mFieldHeight{ height },
      mMinesLeft{ countOfMines()},
      mCellInputHandler{ new CellInputHandler{ this } }
{
    Q_ASSERT(mCells.size() == (mFieldWidth * mFieldHeight));

    connectCellsWithNeighbourCells(mCells, mFieldWidth, mFieldHeight);

    for(auto &cell : mCells) {
        cell->installEventFilter(mCellInputHandler);
    }

    connectWithCells();
    addCellsToLayout();
}

int Minefield::fieldWidth() const
{
    return mFieldWidth;
}

int Minefield::fieldHeight() const
{
    return mFieldHeight;
}

int Minefield::countOfMines() const
{
    auto count = 0;
    for(const auto& cell : mCells) {
        if(cell->hasMine()) {
            ++count;
        }
    }
    return count;
}

int Minefield::minesLeft() const
{
    return mMinesLeft;
}

void Minefield::flaggedCell()
{
    --mMinesLeft;
    emit minesLeftChanged(mMinesLeft);
}

void Minefield::unflaggedCell()
{
    ++mMinesLeft;
    emit minesLeftChanged(mMinesLeft);
}


void Minefield::checkIfFirstCellIsUncovered()
{
    if(!mFirstCellUncovered) {
        mFirstCellUncovered = true;

        for(const auto &cell : mCells) {
            disconnect(cell, &Cell::uncovered,
                       this, &Minefield::checkIfFirstCellIsUncovered);
        }

        emit uncoveredFirstCell();
    }
}

void Minefield::checkIfSafeCellsUncovered()
{
    if(!mSafeCellsUncovered && allSafeCellsUncovered(mCells)) {
        mSafeCellsUncovered = true;

        for(const auto &cell : mCells) {
            disconnect(cell, &Cell::uncovered,
                       this, &Minefield::checkIfSafeCellsUncovered);
        }

        disableInput();
        emit uncoveredAllSafeCells();
    }
}

void Minefield::connectWithCells()
{
    for(const auto &cell : mCells) {
        connect(cell, &Cell::pressed,
                this, &Minefield::pressedCell);
        connect(cell, &Cell::released,
                this, &Minefield::releasedCell);

        connect(cell, &Cell::uncoveredEmptyCell,
                this, &Minefield::uncoveredEmptyCell);

        connect(cell, &Cell::flagged,
                this, &Minefield::flaggedCell);
        connect(cell, &Cell::unflagged,
                this, &Minefield::unflaggedCell);

        connect(cell, &Cell::uncovered,
                this, &Minefield::checkIfFirstCellIsUncovered);
        connect(cell, &Cell::uncovered,
                this, &Minefield::checkIfSafeCellsUncovered);

        connect(this, &Minefield::toggleNewQuesionMarksInCells,
                cell, &Cell::toggleNewQuestionMarks);
        connect(this, &Minefield::toggleColorInCells,
                cell, &Cell::toggleColor);

        if(cell->hasMine()) {
            connect(cell, &Cell::hitMine,
                    [=](){

                showWrongFlaggedCells();
                showAllMines();
                disableInput();
                emit mineExploded();
            });
        }
    }
}

void Minefield::addCellsToLayout()
{
    auto layout = new QGridLayout;
    layout->setSpacing(0);
    layout->setContentsMargins(0,0,0,0);

    for(int i = 0; i < mCells.size(); ++i) {
        auto column = static_cast<int>(i %  mFieldWidth);
        auto row = static_cast<int>(i /  mFieldWidth);

        layout->addWidget(mCells[i], row, column);
    }
    setLayout(layout);
}

void Minefield::showAllMines()
{
    for(const auto cell : mCells) {
        if(cell->hasMine() && cell->isCovered()) {
            cell->showMine();
        }
    }
}

void Minefield::showWrongFlaggedCells()
{
    for(const auto &cell : mCells) {
        if(!cell->hasMine() && cell->isFLagged()) {
            cell->setToFlaggedWrong();
        }
    }
}

void Minefield::disableInput()
{
    setAttribute(Qt::WA_TransparentForMouseEvents);
}
\$\endgroup\$

2 Answers 2

2
+100
\$\begingroup\$

I see some things that may help you improve your code.

Don't leak memory

I see that there are a number of places in which new is invoked but there is no corresponding delete. For example, one leak can be addressed by adding this destructor:

Minefield::~Minefield() {
    delete mCellInputHandler;
}

Put friend declarations in public section

It's misleading to put a friend declaration (as in friend class CellInputHandler in cell.h) in a private section. Because access specifiers have no effect on friend declarations, any friend designation is essentially public. For this reason, I advocate always putting friend declarations in public sections.

Write code for human comprehension

The Cell constructor has this peculiar line:

mHasMine{ static_cast<bool>(state) },

I had to look up the definition of state and found this:

enum class State{
    empty,
    mine
};

Rather than relying on an unintuitive static_cast, I would suggest instead writing this:

  mHasMine{ state == State::mine },

Put static initializers into declaration

When you have a class that has a member data items that are initialized to constants (as with Cell), I recommend using in-class initializers instead of explicitly listing them as part of the constructor. See C.48 for details.

Prefer a switch to long if chain

In the CellInputHandler::eventFilter() we have this code:

if(event->type() == QEvent::MouseButtonPress){
    handleMouseButtonPressEvents(watched, event);
    return true;
}
if(event->type() == QEvent::MouseButtonRelease){
    handleMouseButtonReleaseEvents(watched, event);
    return true;
}
if(event->type() == QEvent::MouseMove) {
    handleMouseMoveEvents(event);
    return true;
}
return false;

I would suggest instead writing it like this:

bool result{true};
switch (event->type()) {
    case QEvent::MouseButtonPress:
        handleMouseButtonPressEvents(watched, event);
        break;
    case QEvent::MouseButtonRelease:
        handleMouseButtonReleaseEvents(watched, event);
        break;
    case QEvent::MouseMove:
        handleMouseMoveEvents(event);
        break;
    default:
        result = false;
}
return result;

The resulting object code is likely to be very similar, but it has an advantage for human comprehension. First, it's easy to see that only a single value is being interrogated. Second, there is a single return which, to me, makes it simpler for readers of the code to understand the flow.

Rethink your event handler

The typical approach in Qt is to have each object handle its own events. Since Minefield is derived from QWidget, I would have expected that it would have an event handler. Instead we have the separate CellInputHandler implemented as an EventFilter. I would suggest that the events handled within CellInputHandler would be better expressed as part of the event() override function in Minefield.

Even better, however, would be to eliminate that completely and simply let the Cell objects handle their own events. Let's consider the scenario you mentioned:

  1. press left mouse button on Cell A and hold it.
  2. Move out of Cell A -> Cell A gets "unpressed" on move out
  3. Move into Cell B -> Cell B gets "pressed"

All that's needed is to provide handlers for QEvent::Enter and QEvent::Leave. Pseudocode:

Enter: 
   if LeftMouseButtonDown, call LeftMouseButtonDown handler

Leave:
    if LeftMouseButtonDown, restore to normal state

Note: Unfortunately, as pointed out in the comment, this doesn't actually work out of the box because a QWidget does a grabMouse() on mouse button press events. However, I suspect that one could issue an explicit releaseMouse() call from within the handler and still keep this approach. I may experiment with that if I get some time.

Encapsulate more

I don't think there's much reason for any class other than Minefield to know about individual Cells. Therefore, I'd suggest changing the interface so that Minefield's constructor is only give a width and height and all Cell creation and further handling would be inside the class.

\$\endgroup\$
6
  • \$\begingroup\$ my plan was to implement enter and leave in cell (it seems much cleaner) but it does not work the sliding between cells. See this discussion: forum.qt.io/topic/107430/…. The Cells are available in the constructor so I can pass not random Cells for test to the Minefield. And about the memory leak. mCellInputHandler gets the Minefield as its parent in the Minefield Constructor.so it should be deleted when Minefield is destroyed \$\endgroup\$
    – Sandro4912
    Nov 18, 2019 at 5:20
  • \$\begingroup\$ I had forgotten that a widget does a MouseCapture on mouse press events, so I understand the dilemma now. As for the memory leak, you might try running valgrind to check your assumption. I did and found the application was leaking memory. \$\endgroup\$
    – Edward
    Nov 18, 2019 at 14:01
  • \$\begingroup\$ yes I will try valgrind. I think I forgot to run it. The cell think is quite a big dilemma. They way its solved now it really looks strange. I was thinking maybe the change between the cells can be implemented by using drag and drop. \$\endgroup\$
    – Sandro4912
    Nov 18, 2019 at 14:18
  • \$\begingroup\$ I found the memory leak in the class Game. I make QTime *mElapsedTime on the heap but it never gets freed. So I freed it in the destructor. After that no more errors from valgrind \$\endgroup\$
    – Sandro4912
    Nov 18, 2019 at 17:32
  • \$\begingroup\$ That's great! Also, I've tried quite a few things with Qt to address the event issue and I am now persuaded that your approach with the EventFilter is probably the best currently available approach. If there were an elegant way to convince Qt widgets not to grabMouse that would likely be better, but I haven't found a good way to do that other than the EventFilter approach you've already implemented. I don't know if you've learned anything from my answer, but I have learned something from your question, so thank you for asking it! \$\endgroup\$
    – Edward
    Nov 18, 2019 at 20:30
2
\$\begingroup\$

Nice, well-presented code, apart from some spacing inconsistency following for, if etc.


I think the Cell constructor should be explicit. It's odd that it's missing initializers for the timers (I use gcc -Weffc++, so tend to include them even for types that won't end up uninitialized).

   constexpr auto intervall = 50;

would be better named interval.


void Cell::paintEvent(QPaintEvent *event)
{
    Q_UNUSED(event)

We could simply leave the argument unnamed instead:

void Cell::paintEvent(QPaintEvent*)
{

Neither cell.cpp nor cellinputhandler.cpp needs <QDebug>.


This looks suspect:

auto mouseEvent = static_cast<QMouseEvent*>(event);
auto widget = QApplication::widgetAt(QCursor::pos());

Why are we not using the event's position here, as we do in handleMouseMoveEvents()? I don't think we can rely on QCursor::pos() being race-free relative to event delivery.


A bit of duplication here:

if(!(event->buttons().testFlag(Qt::LeftButton) ||
     event->buttons().testFlag(Qt::RightButton))) {
    return;
}

if(event->buttons().testFlag(Qt::LeftButton)) {
    mSingleMouseTimerLeft.start();
}
else if (event->buttons().testFlag(Qt::RightButton)){
    mSingleMouseTimerRight.start();
}

We could combine those into a single conditional:

if (event->buttons().testFlag(Qt::LeftButton)) {
    mSingleMouseTimerLeft.start();
}
else if (event->buttons().testFlag(Qt::RightButton)) {
    mSingleMouseTimerRight.start();
}
else {
    return;
}

minefield.h has no need to include <vector> (perhaps left over from an earlier version?).

\$\endgroup\$
6
  • \$\begingroup\$ what does -Weffc++ do? With intervall I guess the german in me slipped through. I thought Q_UNUSED is the more Qt way to hide unused parameters. Normally I tend to do void Cell::paintEvent(QPaintEvent * /*event*/) so its still possible to read the meaning. \$\endgroup\$
    – Sandro4912
    Nov 2, 2019 at 16:22
  • 1
    \$\begingroup\$ -Weffc++ enables warnings about violations of several style guidelines from Scott Meyers’ Effective C++ series of books. \$\endgroup\$ Nov 4, 2019 at 9:16
  • \$\begingroup\$ ah these books are here on my shelf. do you know how to enable this flag with qt creator? \$\endgroup\$
    – Sandro4912
    Nov 4, 2019 at 17:03
  • \$\begingroup\$ No idea, but if it uses qmake you should be able to append to QMAKE_CXXFLAGS in the project file. Beware that you'll want to use -isystem rather than -I for the Qt includes if you add -Weffc++, or you'll get a mountain of warnings from Qt's own headers! I also found that moc-built sources fail my normal choice of warning flags, so had to suppress them there, too. \$\endgroup\$ Nov 4, 2019 at 17:12
  • \$\begingroup\$ i added this in the pro file QMAKE_CXXFLAGS = -Weffc++ -isystem like you said i get a million warnings from qt headers. but why? i thought i enabled isystem \$\endgroup\$
    – Sandro4912
    Nov 4, 2019 at 17:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.