Morpion en C++

Débutant

a marqué ce sujet comme résolu.

Bonjour/bonsoir ! J’ai suivi le cours du site sur le C++ et j’ai décidé de coder un morpion 3–3 pour voir ce que j’avais retenu. Je suis pas à mon coup d’essai en C++ (j’avais déjà un peu lu le cours de gbdivers y’a quelques temps maintenant) et en algorithmique également (on fait du python au lycée).

Je voulais vous montrer ce morceau de code pour avoir des retours sur la façon dont auraient pu être organisées les choses ici. J’imagine aussi que ça doit être bourré d’erreurs niveau utilisation du langage ou même en terme de maintenabilité et d’évolutivité.

Pour ceux qui auront le courage de lire, n’hésitez pas à partager vos avis ! Je vous remercie par avance.

Le code :

#include <iostream>
#include <array>
#include <limits>

struct Grid
{
    Grid();
    void update(bool player1);
    void print();

    std::array<char, 9> content;
    std::array<std::array<int, 3>, 8> winPositions;
};

bool gameWon(const Grid & grid);
std::size_t checkEntry(std::size_t & entry, const Grid* grid);
void printResults(const bool player1, int counter, const Grid & grid);

int main()
{
    Grid grid;
    bool player1 { true };;
    int counter { 0 };

    while (!(gameWon(grid)) && counter != 9) {
        grid.update(player1);
        grid.print();
        player1 = !(player1);
        ++counter;
    }

    printResults(!(player1), counter, grid);

    return 0;
}

Grid::Grid()
{
    content.fill('-');

    winPositions[0] = { 0, 1, 2 };
    winPositions[1] = { 3, 4, 5 };
    winPositions[2] = { 6, 7, 8 };
    winPositions[3] = { 0, 3, 6 };
    winPositions[4] = { 1, 4, 7 };
    winPositions[5] = { 2, 5, 8 };
    winPositions[6] = { 0, 4, 8 };
    winPositions[7] = { 2, 4, 6 };
}

void Grid::update(bool player1)
{
    const int playerNumber { (player1 ? 1 : 2) };
    char c { player1 ? 'X' : 'O' };

    std::cout << "Player " << playerNumber << ", pick a position (1-9): ";
    std::size_t pick { checkEntry(pick, this) };

    --pick;
    content[pick] = c;
}

void Grid::print()
{
    for (std::size_t i { 0 }; i < 9; ++i) {
        std::cout << content[i] << ' ';
        if (i == 2 || i == 5 || i == 8)
            std::cout << std::endl;
    }
}

bool gameWon(const Grid & grid)
{
    for (const auto & line : grid.winPositions) {
        if (grid.content[line[0]] == grid.content[line[1]]
                && grid.content[line[1]] == grid.content[line[2]]
                && grid.content[line[2]] != '-')
            return true;
    }

    return false;
}

std::size_t checkEntry(std::size_t & entry, const Grid* grid)
{
    while (!(std::cin >> entry) || entry > 9 || entry < 1 || grid->content[entry-1] != '-') {
        std::cout << "Invalid entry, please try again: ";
        std::cin.clear();
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
    }

    return entry;
}

void printResults(const bool player1, const int counter, const Grid & grid)
{
    const int playerNumber { player1 ? 1 : 2 };

    if (counter == 9 && !(gameWon(grid)))
        std::cout << "It's a draw." << std::endl;
    else
        std::cout << "Player " << playerNumber << " wins." << std::endl;
}

Je pensais que tu aurais des réponses de gens plus compétents que moi, mais comme ça ne vient pas, je vais apporter ma petite contribution : du code sans commentaire, je considère que ce n’est pas du code.

Hormis ça, ça me paraît propre, et surtout, bien structuré.

Globalement, ça m’a l’air plutôt pas mal. Je vais essayer de faire de mon mieux pour citer tout ce qui pourrait être amélioré (en tout cas selon moi, je suis pas un expert).

Concernant le C++ directement
  1. winPositions contient des positions de grille, or ces positions devraient être de types std::size_t, pas int. GCC peut en donner un warning avec -Wsign-conversion.

  2. winPositions contient des valeurs qui peuvent être définies à la compilation et qui ne changeront jamais, on pourrait donc le déclarer constexpr. Et si on veut aller encore plus loin, plutôt que de définir manuellement ses valeurs, on pourrait les calculer avec un algorithme dans une fonction elle aussi constexpr.

  3. checkEntry prend en paramètre un pointeur vers une grille, plutôt qu’une référence comme toutes les autres fonctions, et ce pour aucune raison valable.

  4. On pourrait croire que checkEntry contrôle la validité du paramètre entry, mais ce n’est pas le cas. Cette fonction modifie ce paramètre mais ne lit jamais sa valeur initiale, puis le renvoie. À mon gout, on pourrait parfaitement se passer de ce paramètre, et éventuellement donner nom qui ne contient pas "check" pour éviter la confusion :

std::size_t promptPosition(const Grid & grid);

// exemple d'utilisation :
auto position = promptPosition(grid);
  1. Ligne 22, un point-virgule en trop.

  2. Ligne 28, parenthèses inutiles.

  3. On pourrait remplacer la ligne 67 par if (i % 3 == 2) (on veut un retour à la ligne toutes les trois cases).

Concernant l’architecture

Ici n’y a pas de vraie bonne manière de faire, du coup je passe rapidement sur les premiers trucs qui me sont venus en tête :

  1. On a plusieurs fonctions qui manipulent la grille, certaines sont des fonctions membres et d’autres des fonctions libres. Je ne vois pas vraiment ici l’intérêt d’une structure avec fonctions membres, car de toute manière contentet winPosition sont d’accessibilité publique. À mon gout, on pourrait faire de update et print des fonctions libres et de Grid un simple alias de tableau.

  2. Les valeurs de la grille sont des caractères qui seront affichés. Et si, par exemple, on prenait soudainement la décision de changer l’apparence des cellules vides ? On serait un peu embêté car on devrait modifier toutes les occurrences de '-' dans le code (il y en a trois). C’est pourquoi on a intérêt à faire abstraction de ce à quoi les cellules vont ressembler. On pourrait par exemple profiter d’une énumération.

Pour résumer, on pourrait avoir :

enum struct Cell { empty, cross, circle };

using Grid = std::array<Cell, 9>;

void update(Grid & grid, bool player1);
void print(const Grid & grid);
// ...
// le reste est à adapter
Concernant les bonnes habitudes
  1. Comme déjà dit plus haut, il faut prendre l’habitude de commenter son code, surtout si on le partage. Pour moi, le plus important est de documenter les déclarations de fonctions (expliquer ce qu’elles font, les paramètres, etc.) Ici, j’ai eu du mal à comprendre ce que faisait checkEntry et j’ai été obligé de lire son implémentation pour m’en faire une idée.

  2. Selon l’ambition du projet, il est important de prévoir qu’il y aura de futures modifications (pas besoin de prévoir nécessairement lesquelles). Pour cela, il suffit de prendre certaines habitudes, comme ne pas écrire plusieurs fois les mêmes valeurs brutes dans le code. Ici, la valeur 9 (la taille du tableau) apparait 6 fois, ce qui va être ennuyeux si on veut par exemple changer la taille de la grille.

  3. Ça rejoint un peu le point d’avant, mais il est également important de bien choisir les noms des fonctions et objets (et parfois c’est assez difficile), afin de pouvoir lire du code et le comprendre immédiatement. Ici, je trouve que les fonctions Grid::update et checkEntry ne reflète pas bien leur comportement, et il n’est pas évident de savoir à quoi sert counter.

Salut et merci d’avoir répondu !

Je pensais que tu aurais des réponses de gens plus compétents que moi, mais comme ça ne vient pas, je vais apporter ma petite contribution : du code sans commentaire, je considère que ce n’est pas du code.

Hormis ça, ça me paraît propre, et surtout, bien structuré.

elegance

En effet, je me suis fais la remarque après-coup, ça doit être illisible pour des gens qui ne l’on pas écrit.. je compte revenir et vous livrer une version mieux commentée.

Globalement, ça m’a l’air plutôt pas mal. Je vais essayer de faire de mon mieux pour citer tout ce qui pourrait être amélioré (en tout cas selon moi, je suis pas un expert).

Concernant le C++ directement
  1. winPositions contient des positions de grille, or ces positions devraient être de types std::size_t, pas int. GCC peut en donner un warning avec -Wsign-conversion.

  2. winPositions contient des valeurs qui peuvent être définies à la compilation et qui ne changeront jamais, on pourrait donc le déclarer constexpr. Et si on veut aller encore plus loin, plutôt que de définir manuellement ses valeurs, on pourrait les calculer avec un algorithme dans une fonction elle aussi constexpr.

  3. checkEntry prend en paramètre un pointeur vers une grille, plutôt qu’une référence comme toutes les autres fonctions, et ce pour aucune raison valable.

  4. On pourrait croire que checkEntry contrôle la validité du paramètre entry, mais ce n’est pas le cas. Cette fonction modifie ce paramètre mais ne lit jamais sa valeur initiale, puis le renvoie. À mon gout, on pourrait parfaitement se passer de ce paramètre, et éventuellement donner nom qui ne contient pas "check" pour éviter la confusion :

std::size_t promptPosition(const Grid & grid);

// exemple d'utilisation :
auto position = promptPosition(grid);
  1. Ligne 22, un point-virgule en trop.

  2. Ligne 28, parenthèses inutiles.

  3. On pourrait remplacer la ligne 67 par if (i % 3 == 2) (on veut un retour à la ligne toutes les trois cases).

Concernant l’architecture

Ici n’y a pas de vraie bonne manière de faire, du coup je passe rapidement sur les premiers trucs qui me sont venus en tête :

  1. On a plusieurs fonctions qui manipulent la grille, certaines sont des fonctions membres et d’autres des fonctions libres. Je ne vois pas vraiment ici l’intérêt d’une structure avec fonctions membres, car de toute manière contentet winPosition sont d’accessibilité publique. À mon gout, on pourrait faire de update et print des fonctions libres et de Grid un simple alias de tableau.

  2. Les valeurs de la grille sont des caractères qui seront affichés. Et si, par exemple, on prenait soudainement la décision de changer l’apparence des cellules vides ? On serait un peu embêté car on devrait modifier toutes les occurrences de '-' dans le code (il y en a trois). C’est pourquoi on a intérêt à faire abstraction de ce à quoi les cellules vont ressembler. On pourrait par exemple profiter d’une énumération.

Pour résumer, on pourrait avoir :

enum struct Cell { empty, cross, circle };

using Grid = std::array<Cell, 9>;

void update(Grid & grid, bool player1);
void print(const Grid & grid);
// ...
// le reste est à adapter
Concernant les bonnes habitudes
  1. Comme déjà dit plus haut, il faut prendre l’habitude de commenter son code, surtout si on le partage. Pour moi, le plus important est de documenter les déclarations de fonctions (expliquer ce qu’elles font, les paramètres, etc.) Ici, j’ai eu du mal à comprendre ce que faisait checkEntry et j’ai été obligé de lire son implémentation pour m’en faire une idée.

  2. Selon l’ambition du projet, il est important de prévoir qu’il y aura de futures modifications (pas besoin de prévoir nécessairement lesquelles). Pour cela, il suffit de prendre certaines habitudes, comme ne pas écrire plusieurs fois les mêmes valeurs brutes dans le code. Ici, la valeur 9 (la taille du tableau) apparait 6 fois, ce qui va être ennuyeux si on veut par exemple changer la taille de la grille.

  3. Ça rejoint un peu le point d’avant, mais il est également important de bien choisir les noms des fonctions et objets (et parfois c’est assez difficile), afin de pouvoir lire du code et le comprendre immédiatement. Ici, je trouve que les fonctions Grid::update et checkEntry ne reflète pas bien leur comportement, et il n’est pas évident de savoir à quoi sert counter.

Olybri

Tout d’abord encore merci pour cette réponse super détaillée ! J’ai bien pris en compte toutes tes remarques et j’ai réécrit le code en conséquence suivant tes conseils. Je me suis heurté sur le coup à l’utilisation de l’énumération dans le code (j’ai surchargé <<). J’ai aussi foutu des #define GRID_SIZE maValeur et ça je ne sais pas si c’est bien ou pas ! J’ai cherché brièvement si on utilisait encore les define en C++ mais je n’ai pas trouvé je l’ai donc quand même fait.. J’ai donc complètement restructuré le code et j’ai intégré les morpions en n’importe quelle taille mais du coup je ne sais pas si j’ai vraiment profité des modifications de structure pour apporter une réelle évolutivité au code, surtout concernant l’affichage..

Voilà la nouvelle version :

// main.cpp
// Le morpion

#include <iostream>
#include <vector>
#include <array>
#include <limits>
#include <cmath>

// On part du principe que l'utilisateur utilise une valeur qui est un carré parfait
// (carré d'un entier)
#define GRID_SIZE 9
#define GRID_SIZE_SQUARED static_cast<size_t>(sqrt(GRID_SIZE))

enum struct Cell { empty, cross, circle };
using Grid = std::array<Cell, GRID_SIZE>;
using Lines = std::vector<std::vector<std::size_t>>;

Lines victoryLinesGenerator();
void update(Grid & grid, bool player1);
std::ostream& operator<<(std::ostream& os, Cell cell);
void print(const Grid & grid);
bool playerVictory(const Grid & grid, const Lines & victoryLines);
std::size_t positionPrompt(std::size_t & entry, const Grid & grid);
void printResults(const bool player1, const int filledCells, const Grid & grid, const Lines & victoryLines);

int main()
{
    Grid grid;
    grid.fill(Cell::empty);
    bool player1 { true };
    int filledCells { 0 };
    Lines victoryLines { victoryLinesGenerator() };

    while (!playerVictory(grid, victoryLines) && filledCells != GRID_SIZE) {
        update(grid, player1);
        print(grid);
        player1 = !player1;
        ++filledCells;
    }

    printResults(!player1, filledCells, grid, victoryLines);

    return 0;
}

Lines victoryLinesGenerator()
{
    Lines victoryLines;
    std::vector<std::size_t> horizontal;
    std::vector<std::size_t> vertical;
    std::vector<std::size_t> diagonal;

    // on génère les horizontales et les verticales

    for (std::size_t i { 0 }; i < GRID_SIZE_SQUARED; ++i) {
        for (std::size_t j { 0 }; j < GRID_SIZE_SQUARED; ++j) {
            horizontal.push_back(GRID_SIZE_SQUARED*i + j);
            vertical.push_back(i + GRID_SIZE_SQUARED*j);
        }

        victoryLines.push_back(horizontal);
        victoryLines.push_back(vertical);

        horizontal.clear();
        vertical.clear();
    }

    // on génère les lignes diagonales (formules trouvées au crayon...)

    for (std::size_t i { 0 }; i < GRID_SIZE_SQUARED; ++i) {
        diagonal.push_back(i + GRID_SIZE_SQUARED*i);
    }

    victoryLines.push_back(diagonal);
    diagonal.clear();

    for (std::size_t i { 0 }; i < GRID_SIZE_SQUARED; ++i) {
        diagonal.push_back(GRID_SIZE_SQUARED-1 + GRID_SIZE_SQUARED*i - i);
    }

    victoryLines.push_back(diagonal);

    return victoryLines;
}

void update(Grid & grid, bool player1)
{
    const int playerNumber { (player1 ? 1 : 2) };
    // le joueur 1 représente les croix, le joueur 2 les cercles
    Cell c { player1 ? Cell::cross : Cell::circle };

    std::cout << "Player " << playerNumber << ", pick a position (1-" << GRID_SIZE << "): ";
    std::size_t pick { positionPrompt(pick, grid) };

    --pick;
    grid[pick] = c;
}

std::ostream& operator<<(std::ostream& os, Cell cell)
{
    char symbol { 0 };

    // peut-être dans la définition de Cell remplacer cross par player1
    // et circle par player2 puis décider de comment les afficher ici ?

    switch (cell) {
    case Cell::empty:
        symbol = '-';
        break;
    case Cell::cross:
        symbol = 'X';
        break;
    case Cell::circle:
        symbol = 'O';
        break;
    }

    return os << symbol;
}

void print(const Grid & grid)
{
    for (std::size_t i { 0 }; i < GRID_SIZE; ++i) {
        std::cout << grid[i] << ' ';
        // l'affichage donne un carré
        if (i % GRID_SIZE_SQUARED == GRID_SIZE_SQUARED-1)
            std::cout << std::endl;
    }
}

bool playerVictory(const Grid & grid, const Lines & victoryLines)
{
    unsigned int alignment { 0 };

    // ici, on regarde si deux cases avec le même symbole s'enchaînent sur une ligne
    // si oui, on incrémente alignment, le but c'est d'enchaîner les symboles sur une ligne
    // soit alignment = taille de la grille - 1 (on commence à 0)
    // alignment == GRID_SIZE_SQUARED-1 est notre condition de victoire

    for (const auto & line : victoryLines) {
        for (std::size_t i { 0 }; i < GRID_SIZE_SQUARED-1; ++i)
            if (grid[line[i+1]] == grid[line[i]] && grid[line[i]] != Cell::empty)
                ++alignment;

        // le joueur a gagné
        if (alignment == GRID_SIZE_SQUARED-1)
            return true;

        // sinon , on remet le compteur à 0 et on passe à la ligne suivante
        alignment = 0;
    }

    return false;
}

std::size_t positionPrompt(std::size_t & entry, const Grid & grid)
{
    // on vérifie l'entrée de l'utilisateur (si la case est déjà remplie ou non,
    // si l'entrée est correcte...

    while (!(std::cin >> entry) || entry > GRID_SIZE || entry < 1 || grid[entry-1] != Cell::empty) {
        std::cout << "Invalid entry, please try again: ";
        std::cin.clear();
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
    }

    return entry;
}

void printResults(const bool player1, const int filledCells, const Grid & grid, const Lines & victoryLines)
{
    const int playerNumber { player1 ? 1 : 2 };

    if (filledCells == GRID_SIZE && !(playerVictory(grid, victoryLines)))
        std::cout << "It's a draw." << std::endl;
    else
        std::cout << "Player " << playerNumber << " wins." << std::endl;
}

Tout d’abord, c’est cool de voir du code moderne! Ca fait vraiment plaisir.

Côté commentaires, je suis moins choqué que mes petits camarades. Certains veulent commenter toutes les lignes, d’autres considèrent qu’un code clair n’a pas besoin d’être commenté. Les fonctions sont courtes, effectivement, les noms peuvent être améliorés, et du coup c’est là qu’il manque de commentaires pour compenser les noms pas forcements clairs. Pour les commentaires de fonctions, le format de Doxygen s’est imposé. Il est intéressant de commenter les fonctions, ce qu’elles attendent de leurs paramètres (préconditions), ce qu’elles garantissent en sortie (post-conditions), et si jamais il y a des astuces mises en oeuvre, des choix qui ont été faits, il est bon de les commenter. Bref.

Mes petits pinaillages, sur le dernier code.

a- Pourquoi un mélange de vector et d’array? Pourquoi cette hétérogénéité? Le mélange à plat, et bi-dimensionnel me perturbe aussi. Sois homogène.

b- Oubli #define, surtout pour définir des constantes. Et privilégie constexpr pour déclarer des constantes.

c- bool représente vrai ou faux. Joueur 1 ou Joueur 2, cela n’a rien de vrai ou de faux. Personnellement, j’avais utilisé un énuméré (https://github.com/LucHermitte/tictactoe/)

d- playerVictory ne fait pas un bon nom de fonction booléenne. has_won, does_player_win… sont de meilleur noms: ils portent cet aspect d’état.

e- Dans le même genre filledCells pourrait désigner les cases remplies plutôt que le nombre de cases remplies.

Tout d’abord, c’est cool de voir du code moderne! Ca fait vraiment plaisir.

Côté commentaires, je suis moins choqué que mes petits camarades. Certains veulent commenter toutes les lignes, d’autres considèrent qu’un code clair n’a pas besoin d’être commenté. Les fonctions sont courtes, effectivement, les noms peuvent être améliorés, et du coup c’est là qu’il manque de commentaires pour compenser les noms pas forcements clairs. Pour les commentaires de fonctions, le format de Doxygen s’est imposé. Il est intéressant de commenter les fonctions, ce qu’elles attendent de leurs paramètres (préconditions), ce qu’elles garantissent en sortie (post-conditions), et si jamais il y a des astuces mises en oeuvre, des choix qui ont été faits, il est bon de les commenter. Bref.

Mes petits pinaillages, sur le dernier code.

a- Pourquoi un mélange de vector et d’array? Pourquoi cette hétérogénéité? Le mélange à plat, et bi-dimensionnel me perturbe aussi. Sois homogène.

b- Oubli #define, surtout pour définir des constantes. Et privilégie constexpr pour déclarer des constantes.

c- bool représente vrai ou faux. Joueur 1 ou Joueur 2, cela n’a rien de vrai ou de faux. Personnellement, j’avais utilisé un énuméré (https://github.com/LucHermitte/tictactoe/)

d- playerVictory ne fait pas un bon nom de fonction booléenne. has_won, does_player_win… sont de meilleur noms: ils portent cet aspect d’état.

e- Dans le même genre filledCells pourrait désigner les cases remplies plutôt que le nombre de cases remplies.

lmghs

Merci pour ces conseils ! Je n’ai pas encore pu voir et mettre en place constexpr, je vais chercher ça. Je vais aussi regarder pour les commentaires, tous vos renseignements me sont très utiles.

Je reviendrai ici si j’apporte un peu de nouveauté au programme, mais vu mes capacités maintenant j’en doute. J’ai regardé votre projet de morpion sur github, et ça va très loin. Mon but ça serait de foutre tout ça à travers une interface graphique, avec Qt pourquoi pas…

Bien à vous,

Connectez-vous pour pouvoir poster un message.
Connexion

Pas encore membre ?

Créez un compte en une minute pour profiter pleinement de toutes les fonctionnalités de Zeste de Savoir. Ici, tout est gratuit et sans publicité.
Créer un compte