23
\$\begingroup\$

During my hiatus, I decided to revisit some of the Tic-Tac-Toe questions here. I created a simple Tic-Tac-Toe GUI game, and then thought, might as well do the Ultimate Tic-Tac-Toe challenge.

I am looking for ways to refactor my code. Particularly in MainWindow.cpp.

I think this is a mess. I spent very little time on this project and just started hacking away. Nonetheless, the computer does play a "perfect" game.

Here is the Github repo. I have uploaded a compiled binary for Linux.

Sample game against another online AI

MainWindow.h

#ifndef MAINWINDOW_H
#define MAINWINDOW_H

#include "dialog.h"
#include "tictactoe.h"
#include <string.h>
#include <iostream>
#include <QMainWindow>
#include <QPushButton>
#include <QDebug>
#include <QThread>
#include <QtConcurrent>
#include <QFuture>

namespace Ui {
class MainWindow;
}

class MainWindow : public QMainWindow
{
    Q_OBJECT

public:
    int player = 1; //Keeps track of current


    explicit MainWindow(QWidget *parent = 0);
    void setUpGrid();
    void setChar(char);
    void Play();
    void CheckWinner(int currentGrid);

    void colorBoard(int nextGrid, int move);
    void colorBoardWin(int nextGrid,int player);
    void colorBoardUltimateWin(int player);

    void computer(int grid, int player);
    ~MainWindow();

signals:
    void turnComplete(int);
    void computer_(int,int player);

public slots:
    int itemClicked();
    void humanMoves();

    //void computer(int grid,int player);
    void prediction(QString);


private slots:

    void begin(int strength);

    void invalidMove();
    void computerMove(int,int);


    void on_playAgain_clicked();

private:
    Ui::MainWindow *ui;
    Dialog *options = new Dialog;

    QPushButton *itemButtons[9][5];
    QGridLayout *layouts[9];
    QFrame *frames[9];



    QPushButton *createButton(QString&, const QString/*const char**/);

    TicTacToe *game = new TicTacToe();

    int strength;
    bool humanTurn;

    int currentGrid = -1;
    int nextGrid = -1;


    int wonGrids[9] = {};
    int previousMove[2] = {-1,-1};  //Stores currentGrid, move


    int test = -1;


    QThread *pthread = new QThread();

};

#endif // MAINWINDOW_H

MainWindow.cpp

#include "mainwindow.h"
#include "ui_mainwindow.h"


MainWindow::MainWindow(QWidget *parent) :
    QMainWindow(parent),
    ui(new Ui::MainWindow)
{
    ui->setupUi(this);

    humanTurn = false;

    ui->playAgain->setVisible(false);

    options->setWindowFlags(Qt::WindowStaysOnTopHint);
    options->setModal(true);
    options->show();

    setUpGrid();

    game->moveToThread(pthread);


    connect(options,SIGNAL(choosen(int)),this,SLOT(begin(int)));
    connect(game,SIGNAL(humanMoves()),this,SLOT(humanMoves()));
    connect(game,SIGNAL(computerMove(int,int)),this,SLOT(computerMove(int,int)));
    connect(game,SIGNAL(prediction(QString)),this,SLOT(prediction(QString)));

    //connect(this,SIGNAL(computer_(int,int)),this,SLOT(computer(int,int)));

    pthread->start();
}

MainWindow::~MainWindow()
{
    delete ui;
}

void MainWindow::setUpGrid(){




    frames[0] = ui->gridFrame;
    frames[1] = ui->gridFrame_2;
    frames[2] = ui->gridFrame_3;
    frames[3] = ui->gridFrame_4;
    frames[4] = ui->gridFrame_5;
    frames[5] = ui->gridFrame_6;
    frames[6] = ui->gridFrame_7;
    frames[7] = ui->gridFrame_8;
    frames[8] = ui->gridFrame_9;


    layouts[0] = ui->gridLayout;
    layouts[0]->parent()->setObjectName(QString::number(0));
    layouts[1] = ui->gridLayout_2;
    layouts[1]->parent()->setObjectName(QString::number(1));
    layouts[2] = ui->gridLayout_3;
    layouts[2]->parent()->setObjectName(QString::number(2));
    layouts[3] = ui->gridLayout_4;
    layouts[3]->parent()->setObjectName(QString::number(3));
    layouts[4] = ui->gridLayout_5;
    layouts[4]->parent()->setObjectName(QString::number(4));
    layouts[5] = ui->gridLayout_6;
    layouts[5]->parent()->setObjectName(QString::number(5));
    layouts[6] = ui->gridLayout_7;
    layouts[6]->parent()->setObjectName(QString::number(6));
    layouts[7] = ui->gridLayout_8;
    layouts[7]->parent()->setObjectName(QString::number(7));
    layouts[8] = ui->gridLayout_9;
    layouts[8]->parent()->setObjectName(QString::number(8));



    for(int i=0; i<9; i++){
        for(int j=0; j<9; j++){
            QString text = "";
            //        itemButtons[i] = new QPushButton();
            //        connect(itemButtons[i],SIGNAL(clicked()),this,SLOT(itemClicked()));
            itemButtons[i][j] = createButton(text,SLOT(itemClicked()));
            itemButtons[i][j]->setObjectName(QString::number(j));
        }
    }



    for(int j=0; j<9; j++)
        for(int i=0; i<9; i++){
            int row = i/3;
            int column = i%3;
            if(i / 9 == 0)
                layouts[0]->addWidget(itemButtons[0][i],row,column);
            if(i / (9*2) == 0)
                layouts[1]->addWidget(itemButtons[1][i],row,column);
            if(i / (9*3) == 0)
                layouts[2]->addWidget(itemButtons[2][i],row,column);
            if(i / (9*4) == 0)
                layouts[3]->addWidget(itemButtons[3][i],row,column);
            if(i / (9*5) == 0)
                layouts[4]->addWidget(itemButtons[4][i],row,column);
            if(i / (9*6) == 0)
                layouts[5]->addWidget(itemButtons[5][i],row,column);
            if(i / (9*7) == 0)
                layouts[6]->addWidget(itemButtons[6][i],row,column);
            if(i / (9*8) == 0)
                layouts[7]->addWidget(itemButtons[7][i],row,column);
            if(i / (9*9) == 0)
                layouts[8]->addWidget(itemButtons[8][i],row,column);
        }





}


QPushButton *MainWindow::createButton(QString &text, const QString member/*const char *member*/){
    //Creates button and sets size policy. Connects button's signal to slot

    QPushButton *button = new QPushButton;
    button->setSizePolicy(QSizePolicy::Expanding,QSizePolicy::Expanding);
    button->setText(text);
    button->setCheckable(true);
    button->setAutoExclusive(true);

    connect(button, SIGNAL(clicked()),this,member.toStdString().c_str()); //connect button signal to SLOT(itemClicked)
    return button;
}

int MainWindow::itemClicked(){
    //Slot for when a tile has been clicked.

    QPushButton *clickedItem = qobject_cast<QPushButton*>(sender());

    clickedItem->setAutoExclusive(false);
    clickedItem->setChecked(false);
    clickedItem->setAutoExclusive(true);

    currentGrid = clickedItem->parent()->objectName().toInt();

    if(currentGrid != nextGrid && nextGrid != -1 && !wonGrids[nextGrid]){
        qDebug() << "NOPE";
        invalidMove();
        return -1;
    }

    //Read user symbol, and set button to that symbol
    if(humanTurn && game->isLegal(clickedItem->objectName().toInt(),currentGrid) && !wonGrids[currentGrid]){

        clickedItem->setText(QString(QChar(game->gridChar(game->human))));        //Sets pushbutton text to player's char.

        game->humanMove(clickedItem->objectName().toInt(),currentGrid);           //Sets subBoard with player's char


        CheckWinner(currentGrid);
        player = 1;

        nextGrid = clickedItem->objectName().toInt();                             //next move for computer will need to be where player sent it to.


       // emit computer_(nextGrid,1);
        QFuture<void> future = QtConcurrent::run(this,&MainWindow::computer,nextGrid,1);
        //game->CalculateGrid(nextGrid,1);
        qDebug() << "RETURN: ";

    }
    //else if(!game->isLegal(clickedItem->objectName().toInt(),currentGrid))
        //invalidMove();
}

void MainWindow::begin(int strength){
    //Sets player character. Calls TicTacToe::Play()
    //The Game begins.

    QString text = "You are playing as " + QString(QChar(options->getChar()));
    this->strength = strength;
    game->setDepth(strength);

    if(options->getChar() == 'O'){
        text.append(". Computer moves first.");
        player = 1;
        int r = std::rand() % 9;
        game->CalculateGrid(r,1); //Computer moves.
    }
    else{
        text.append(". Select a square.");
        humanTurn = true;
        player = -1;
    }

    ui->announce->setText(text);

}

void MainWindow::humanMoves(){

    humanTurn = true;
    ui->announce->setText("Make a move.");

}


void MainWindow::computerMove(int move,int grid){


    //When the computer goes first.
    if(grid != -1)
        nextGrid = grid;



    qDebug() << "PLAYER: " << player;
    humanTurn = false;
    itemButtons[nextGrid][move]->setText(QString(QChar(game->gridChar(player))));

    //Change player to opponent
    if(player == 1)
        player = -1;
    else
        player = 1;

    colorBoard(nextGrid,move);

    //Check if computer has won
    CheckWinner(nextGrid);

    //If not, then player's next move will be
    nextGrid = move;

    qDebug() << "Player must move here: " << nextGrid;

    humanMoves();


    //emit computer(nextGrid);
}

void MainWindow::CheckWinner(int grid){

    QString announce;
    if(game->winner(grid) == -1){
        game->setGridState(grid,-1);

        qDebug() << "PLAYER WON AT " << grid;

        humanTurn = false; //Disable clicking

        wonGrids[grid] = -1;
        colorBoardWin(grid,-1);

        ui->playAgain->setVisible(true);
    }
    else if(game->winner(grid) == 1){
        game->setGridState(grid,1);

        qDebug() << "COMPUTER WON AT " << grid;

        wonGrids[grid] = 1;
        colorBoardWin(grid,1);

        ui->playAgain->setVisible(true);
    }
    else if(game->winner(grid) == -5){

        announce = "TIE! on board" + QString::number(grid);

        ui->announce->setText(announce);

        ui->playAgain->setVisible(true);
        return;
    }

    //Now check ultimate win
    if(game->ultWin() == -1)
        qDebug() << "HUMAN WIN";
    else if(game->ultWin() == 1){

        qDebug() << "COMPUTER WIN";

        frames[game->ultimateWinGrids[0]]->setStyleSheet("background-color: red");
        frames[game->ultimateWinGrids[1]]->setStyleSheet("background-color: red");
        frames[game->ultimateWinGrids[2]]->setStyleSheet("background-color: red");

    }

}

void MainWindow::invalidMove(){
    ui->announce->setText("Invalid move! Try again.");
    qDebug() << "currentGrid: " << currentGrid;
    qDebug() << "nextGrid: " << nextGrid;
}

void MainWindow::on_playAgain_clicked()
{

    //Clear all background colors,

    for(int i=0; i<9; i++){
        for(int j=0;j<9;j++){
            itemButtons[i][j]->setText("");
        }
    }

    for(int i=0; i<9; i++){
        frames[i]->setStyleSheet("background-color: none");
        wonGrids[i] = 0;
        for(int j=0; j<9; j++){
            itemButtons[i][j]->setStyleSheet("background-color: none");
        }
    }

    currentGrid = -1;
    nextGrid = -1;
    game->reset();
    begin(strength);

    ui->playAgain->setVisible(false);

}

void MainWindow::colorBoard(int nextGrid, int move){

    //itemButtons[nextGrid][move]->setStyleSheet("background-color: red");

    for(int i=0; i<9; i++){
        frames[i]->setStyleSheet("background-color: none");
    }

    for(int i=0; i<9; i++){
        for(int j=0; j<9; j++){

            if(wonGrids[i] == EMPTY){
                itemButtons[i][j]->setStyleSheet("background-color: none");
            }
        }
    }


    //Sets computer move's background color
    QPushButton* button = itemButtons[nextGrid][move];
    QPalette pal = button->palette();
    pal.setColor(QPalette::Button, QColor(Qt::blue));
    button->setAutoFillBackground(true);
    button->setPalette(pal);
    button->update();

    itemButtons[nextGrid][move]->setStyleSheet("background-color: yellow");

    if(wonGrids[move] == EMPTY){
        frames[move]->setStyleSheet("background-color: lightgreen");
    }
    else{
        for(int i=0; i<9; i++){
            if(i != move)
                frames[i]->setStyleSheet("background-color: lightblue");
        }
    }



}

void MainWindow::colorBoardWin(int nextGrid,int player){



    if(player == -1){
        itemButtons[nextGrid][game->winningRows[0]]->setStyleSheet("background-color: lightblue");
        itemButtons[nextGrid][game->winningRows[1]]->setStyleSheet("background-color: lightblue");
        itemButtons[nextGrid][game->winningRows[2]]->setStyleSheet("background-color: lightblue");
    }
    else{
        itemButtons[nextGrid][game->winningRows[0]]->setStyleSheet("background-color: pink");
        itemButtons[nextGrid][game->winningRows[1]]->setStyleSheet("background-color: pink");
        itemButtons[nextGrid][game->winningRows[2]]->setStyleSheet("background-color: pink");
    }



}

void MainWindow::colorBoardUltimateWin(int player){

    //for(int i=0; i<)
}

void MainWindow::computer(int grid,int player){

    double time;
    humanTurn = false; //Prevent random clicking
    QTime myTimer;
    myTimer.start();
    //Qtconcurrent::run();
    ui->announce->setText("Thinking...");
    game->CalculateGrid(nextGrid,player);

    time = myTimer.elapsed();

    ui->timer->setText("Move took: " + QString::number(time/1000) + " seconds.");

    humanTurn = true;

    qDebug() << "Returned!!!";

}


void MainWindow::prediction(QString prediction){

    ui->predictor->setText(prediction);

}

tictactoe.cpp

#include "tictactoe.h"

#include <QTextStream>
#include <QProcess>
#include <stdio.h>
#include <QDebug>
#include <ctime>

using std::cout;
using std::endl;


TicTacToe::TicTacToe(QObject *parent) : QObject(parent)
{
    std::srand(std::time(0));
    std::fill(boards.begin(),boards.end(),std::vector<int>(NUM_SQUARES,EMPTY));
}

void TicTacToe::setDepth(int strength){
    DEPTH_LIMIT = strength;
    qDebug() << "DepthLimit: " << DEPTH_LIMIT;
}

void TicTacToe::humanMove(int move,int currentGrid){

    boards[currentGrid][move] = human;

}

bool TicTacToe::isLegal(int move,int currentGrid) const{

    qDebug() << "is Legal?: " << currentGrid;
    qDebug() << "isLegal move?: " << move;
    qDebug() << "boards[currentGrid][move]: " << (boards[currentGrid][move] == EMPTY);

    return (boards[currentGrid][move] == EMPTY);
}

int TicTacToe::winner(const std::vector<int> &board){

    const int WINNING_ROWS[8][3] = {
        {0,1,2},
        {3,4,5},
        {6,7,8},
        {0,3,6},
        {1,4,7},
        {2,5,8},
        {0,4,8},
        {2,4,6} };
    const int TOTAL_ROWS = 8;

    for(int row = 0; row < TOTAL_ROWS; ++row){

        if( (board[WINNING_ROWS[row][0]] != EMPTY) &&                              //Go through the rows of WINNING_ROWS and determine if board[x][y] is winner
                (board[WINNING_ROWS[row][0]] == (board[WINNING_ROWS[row][1]])) &&
                (board[WINNING_ROWS[row][1]] == board[WINNING_ROWS[row][2]]) )
        {

            qDebug() << "Ultimate win rows: " << WINNING_ROWS[row][0];
            qDebug() << "Ultimate win rows: " << WINNING_ROWS[row][1];
            qDebug() << "Ultimate win rows: " << WINNING_ROWS[row][2];
            ultimateWinGrids[0] = WINNING_ROWS[row][0];
            ultimateWinGrids[1] = WINNING_ROWS[row][1];
            ultimateWinGrids[2] = WINNING_ROWS[row][2];

            return board[WINNING_ROWS[row][0]]; //Return the character that has won
        }
    }

    //No one has won yet
    return 0;
}


int TicTacToe::winner(int currentGrid){

    qDebug() << "Checking winner at grid: " << currentGrid;


    const int WINNING_ROWS[8][3] = {
        {0,1,2},
        {3,4,5},
        {6,7,8},
        {0,3,6},
        {1,4,7},
        {2,5,8},
        {0,4,8},
        {2,4,6} };
    const int TOTAL_ROWS = 8;

    for(int row = 0; row < TOTAL_ROWS; ++row){

        if( (boards[currentGrid][WINNING_ROWS[row][0]] != EMPTY) &&                              //Go through the rows of WINNING_ROWS and determine if board[x][y] is winner
                (boards[currentGrid][WINNING_ROWS[row][0]] == boards[currentGrid][WINNING_ROWS[row][1]]) &&
                (boards[currentGrid][WINNING_ROWS[row][1]] == boards[currentGrid][WINNING_ROWS[row][2]]) )
        {
            qDebug() << "Regular win: " <<  WINNING_ROWS[row][0];
            qDebug() << "Regular win: " <<  WINNING_ROWS[row][1];
            qDebug() << "Regular win: " <<  WINNING_ROWS[row][2];

            winningRows[0] = WINNING_ROWS[row][0];
            winningRows[1] = WINNING_ROWS[row][1];
            winningRows[2] = WINNING_ROWS[row][2];
            return boards[currentGrid][WINNING_ROWS[row][0]]; //Return the character that has won
        }
    }

    //Check for tie. If no more empty spots, no more moves
    if(count(boards[currentGrid].begin(), boards[currentGrid].end(), EMPTY) == 0)
        return TIE;
}


void TicTacToe::reset(){
    for(int i=0; i<9; i++)
        gridStates[i] = 0;
    board = std::vector<int>(NUM_SQUARES,EMPTY);
    std::fill(boards.begin(),boards.end(),std::vector<int>(NUM_SQUARES,EMPTY));

}

char TicTacToe::gridChar(int i) {
    switch(i) {
    case -1:
        return 'X';
    case 0:
        return ' ';
    case 1:
        return 'O';
    }

    return 0;
}

//Ultimate Tic-Tac-Toe Functionality
//Calculates which tile to play

int TicTacToe::CalculateGrid(int currentGrid,int player){

    // If a player forces the computer to a score that has already been won...welp.
    // Computer gets to pick whichever grid he wants. 2 guud 4 uuu
    int test;
    if(gridStates[currentGrid] != EMPTY){
        qDebug() << "NOT EMPTY" << endl << endl;
        int move,best,bestscore = -20000;
        for(int i=0; i<9; i++){
            qDebug() << "looping";
            if((i != currentGrid)){
                move = pickMove(i,player,best);
                qDebug() << "best: " << best;
                //Check if move place is empty, and that the move grid has not been won.
                if((best > bestscore) && (boards[i][move] == EMPTY) && (gridStates[i] == EMPTY)){
                    qDebug() << "move: " << move << " i: " << i;
                    boards[i][move] = player;
                    emit computerMove(move,i);
                    return 20;
                }
            }
        }
    }



    int movetomake = pickMove(currentGrid,player,test);

    boards[currentGrid][movetomake] = player;

    emit computerMove(movetomake,currentGrid);
    return 20;
}


void TicTacToe::setGridState(int grid, int winner){

    gridStates[grid] = winner;
}

int TicTacToe::ultWin(){

    int lol = winner(gridStates);
    qDebug() << "ULIMATE WIN: " << lol;
    //return winner(gridStates);
                return lol;
}

void TicTacToe::utility(int currentGrid,int move,int depth){


    bool win = false;

    int triad_sum = 0;
    int score = 0;


    int numElements = BY_SLOT[move][0];

    for(int i=1; i<=numElements; i++){
        triad_sum = 0;
        for(int j=0; j<3; j++){
            triad_sum += boards[currentGrid][WINNING_TRIADS[BY_SLOT[move][i]][j]];
        }



        switch(triad_sum){

        case 3:
        case -3:
        {
            win = 1;
            score = boards[currentGrid][WINNING_TRIADS[BY_SLOT[move][i]][0]] * VERY_LARGE - depth;
            break;
        }
        case 2:{
            score += 3000;
            break;
        }
        case -2:
            score -=3000;
            break;
        case 1:
        case -1:{
            if((boards[currentGrid][WINNING_TRIADS[BY_SLOT[move][i]][0]] & boards[currentGrid][WINNING_TRIADS[BY_SLOT[move][i]][1]] & boards[currentGrid][WINNING_TRIADS[BY_SLOT[move][i]][2]]) != 0){
                score -= triad_sum * 1000;
            }
            break;
        }
        case 0:{
            score += boards[currentGrid][move];
            break;
        }
        default:
            break;
        }

        if(win){
            break;
        }


    }


    if(!win){

        //qDebug() << "BScore: " << score;
        int bonus = 0;
        if(move == 0 || move == 2 || move == 6 || move == 8){
            bonus += 2;//*boards[currentGrid][move];
        }

//        if(move == 4){
//            bonus += 7;//*boards[currentGrid][move];
//        }

        //Strange behaviour documented here.
        if(currentGrid == 0){
            bonus +=7;
        }

        score += boards[currentGrid][move]*bonus;


    }


    rets[0] = score;
    rets[1] = win;

}

int TicTacToe::pickMove(int currentGrid,int player, int& best){

    qDebug() << "Picking move for grid: " << currentGrid;

    int score = 0;
    int bestScore = -VERY_LARGE - DEPTH_LIMIT;
    std::vector<int> my_moves;

    int opponent;
    if(player == 1)
        opponent = -1;
    else
        opponent = 1;


    for(int slot=0; slot<9; slot++){

        if(boards[currentGrid][slot] == EMPTY){
            //boards[currentGrid][slot] = 1;
            boards[currentGrid][slot] = player;


            utility(currentGrid,slot,0);
            score = alphaBeta(boards[currentGrid],slot,opponent,player,(-(VERY_LARGE+DEPTH_LIMIT)-1),(VERY_LARGE+DEPTH_LIMIT+1),1,rets[0],rets[1]);

            boards[currentGrid][slot] = 0;

            if((score > bestScore) && (gridStates[currentGrid] == EMPTY) && (boards[currentGrid][slot] == EMPTY)){
                bestScore = score;
                my_moves.clear();
                my_moves.push_back(slot);

            }
            else if((score == bestScore) && (gridStates[currentGrid] == EMPTY) && (boards[currentGrid][slot] == EMPTY)){
                my_moves.push_back(slot);
            }


        }

        qDebug() << "Move: " << slot << " Score: " << score;    
    }

    int rv = 0;
    if(my_moves.size() < 1){

    }
    else{
        //rv = my_moves.back();
        rv = my_moves[0];

    }

    if(bestScore > THRESHOLD)
        emit prediction(QString("Predict Computer Wins."));
    else if(bestScore < -THRESHOLD)
        emit prediction(QString("Predict Human Wins."));
    else
        emit prediction(QString("Score: " + QString::number(score)));


    qDebug() << "Ultimate move: " << rv << endl;
    qDebug() << "score: " << score;
    best = score;
    return rv;

}

int TicTacToe::alphaBeta(std::vector<int>& board,int last_slot,int player, int next_player,int alpha, int beta, int depth, int score_so_far, int last_move_won){

    if(last_move_won || depth >= DEPTH_LIMIT){
        return score_so_far;
    }



    int score = score_so_far;
    int value;

    for(int i=0; i<9; i++){
        if(boards[last_slot][i] == EMPTY){
            boards[last_slot][i] = player;

            utility(last_slot,i,depth);
            value = alphaBeta(board,i,next_player,player,alpha,beta,depth+1,score_so_far + rets[0],rets[1]);

            boards[last_slot][i] = 0;
            switch(player){
            case 1:
                if(value > alpha)
                    alpha = value;
                break;
            case -1:
                if(value < beta)
                    beta = value;
                break;
            }

                if(beta <= alpha)
                break;
        }

    }

    score = beta;
    if(player == 1)
        score = alpha;   
    return score;   
}
\$\endgroup\$
  • 4
    \$\begingroup\$ perfect game, huh? How about we let one of my bots fight one of your bots and see who plays the most perfect? :) \$\endgroup\$ – Simon Forsberg Oct 31 '14 at 22:33
  • \$\begingroup\$ @SimonAndréForsberg Ha :P. \$\endgroup\$ – Quaxton Hale Oct 31 '14 at 23:44
  • \$\begingroup\$ @SimonAndréForsberg that's actually a really interesting idea... so many possibilities go with that. Program wars. *starts building app* \$\endgroup\$ – Viziionary Nov 1 '14 at 4:31
  • \$\begingroup\$ Updated the code. Now supports playing until three tiles have been won in a row. Current uploaded binary doesn't reflect those changes, so if you want to test, you'll have to compile/build from the source. \$\endgroup\$ – Quaxton Hale Nov 1 '14 at 10:27
  • \$\begingroup\$ Just curious, how much did you try to make this "clean code"? I am no C++ expert, but I see a whole bunch of "easy nitpicks" that you could have improved easily. \$\endgroup\$ – Simon Forsberg Nov 1 '14 at 13:09
11
\$\begingroup\$

This will not be a full review of the code at all since I don't really review how it works, but here are some little things that may be of interest to make it better anyway :)

Qt5 new connection syntax

Qt5 introduces a new syntax for signals and slots which allows you to directly pass function pointers instead of relying on the Qt preprocessor to do the job. Your set of connections could be rewritten as:

connect(options, &Dialog::choosen,
        this, MainWindow::begin);
connect(game, &TicTacToe::humanMoves,
        this, &MainWindow::humanMoves);
connect(game, &TicTacToe::computerMove,
        this, &MainWindow::computerMove);
connect(game, &TicTacToe::prediction,
        this, &MainWindow::prediction);

There are many pros and some cons. You can look at the page I linked above for the details. Basically, the syntax is considered more compliated, but it gets typedefs and namespaces right since it does rely on the C++ type system and not on strings processing anymore. Moreover, the functions passed as slots don't even have to be slots anymmore; you can even pass lambdas as slots.

Use nullptr

Since you are using C++11, you should use nullptr instead of 0 and NULL to represent null pointers. It has the advantage of be being easy to search and to chose the pointer overload when a function is overloaded for integers and pointers:

explicit MainWindow(QWidget *parent = nullptr);

Don't use std::rand

While easy to use, std::rand is not the best tool to generate random numbers. For a simple game like this, it may not matter that much, but the C++11 header <random> provides better alternatives to generate pseudo-random numbers. That's not critical for your application, but you should have a look at it.

Use the free functions std::begin and std::end

Once again it won't change anything for you, but since C++11, it is good practice to use the free functions std::begin and std::end instead of the container's methods. The reason if that they will also work with C-style arrays and std::valarray. This is a good habit to take, because it ensures that your code will still work if you change the container. Of course, the real advantage of these functions is that they make template code more generic.

std::fill(std::begin(boards),std::end(boards),std::vector<int>(NUM_SQUARES,EMPTY));

begin is a half-reserved method name

While it is ok to name a method begin, a regular C++ user will expect that method to return an iterator and also will expect that another method named end, also returning an iterator, exists. There are no rules anywhere saying that this method name is reserved, but it is so commonly used for iteration that in practice it totally feels like a reserved name. To avoid a potential ambiguity, I would change the name begin to start or some equivalent in your class. That may help you or others to avoid some problems in the future.

Reorganize your includes

Whenever possible, incude the heades you need in the implementation file and not in the header. Also, try to remove the headers that are not used:

  • Move <iostream> to tictactoe.cpp.
  • Move <QFuture> to MainWIndow.cpp.
  • Remove <string.h>, you don't need it.
  • I probably forgot some more headers that you could move/delete.
  • Try to see whether you could use forward declarations to reduce the number of headers included in the .h file.
  • C++11 in-class initializers are great, but initializing the variables in the constructors in the .cpp instead may allow you to only need forward declarations in the .h and to subsequently move of the includes to the .cpp files.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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