Premier jeu en C++ (avec SFML)

Bonjour,

J’ai commencé à apprendre le C++ il y a un mois (j’avais déjà des bases en C et dans d’autres langages). J’ai développé mon premier jeu, "OXO", qui est un simple Morpion. Le but était simplement d’apprendre le C++ et à utiliser la librairie SFML.

Ayant tout appris sur internet (OpenClassrooms, forums…), je n’ai aucune idée de la qualité de mon code.

C’est pourquoi je m’en remets à vous. Y a-t-il des erreurs de débutant que j’ai commises ? Des optimisations sont-elles possibles ?

Merci pour votre temps passé et vos éventuels conseils !

Code source ici : github.com/BizohxDev/OXO Télécharger le jeu compilé (Windows) : ICI

Salut,

Bravo d’avoir réalisé ce petit jeu.
ça m’interesse et je ferais certainement une relecture, en donnant si je peux quelques conseils de bonnes pratiques et des pistes de design pattern qui aurait pu t’être utile. Pour les optimisations, si c’est lié à une bonne pratique je l’indiquerai mais sinon optimiser n’a de sens qu’avec des mesures et des objectifs (il y a souvent des choix à faire entre la RAM consommé et le temps de calcul par exemple). Et sur un petit projet comme ça, d’autant plus à vocation d’apprentissage, ça n’a pas vraiment de sens.

Je vais voir quand j’aurais le temps de rentrer dans les détails mais pour moi ce ne sera probablement pas cette semaine désolé. Par contre en jetant juste un oeil au dépôt, je pense qu’il y a un petit travail d’organisation des sources.

Afin que tout le monde puisse compiler tes sources, je te conseille d’ajouter un système de build. Le standard de facto, celui qui s’est largement imposé dans la communauté C++, c’est CMake. Tu peux jeter un oeil aussi à XMake, SCons, ou Meson qui ont le bon goût d’utiliser un vrai langage de script pour piloter le build (De plus XMake sert aussi de gestionnaire de package de lib, ce qui est plutôt agréable). Sinon il y a les traditionnels Makefile, ou un peu au dessus Automake, pas mal de projets tournent encore avec mais je ne le recommanderait pas.
Fait un choix parmi ces systèmes puis intègre le fichier au dépôt. Comment l’as tu compilé jusqu’à présent d’ailleurs ?
Puis tu peux mettre les instructions de compilation dans le README ou dans un COMPILING.md, indiquant quelles sont les dépendances à installer et quels sont les commandes à exécuter pour réussir à compiler correctement ton projet.

Ensuite, le dossier src contient conventionnellement plutôt le code source, que tu as mis à la racine. Ainsi à la racine, tu vas pouvoir mettre quelques doc comme le readme et la licence, ou encore des fichiers de configuration d’intégration continue, sans les mélanger au code source. Et ce que tu as mis dans ton dossier src, on le désigne généralement plutôt comme "ressources" ou "assets".

Je reviendrai pour parler de ce qui t’intéresse vraiment, la qualité de code, une autre fois.

Merci pour ta réponse !

1. Pour le système de build :

En fait, au début j’utilisais Code::Blocks comme IDE, et je ne sais pas s’il en utilise un ou pas, mais c’était complètement transparent. Je suis passé ensuite sur CLion, qui lui utilise CMake. Cependant je n’y comprends pas grand chose, il faudrait que je me penche dessus un peu plus sérieusement…

2. Organisation des dossiers/fichiers

Merci pour ces infos, déjà rien que ça, ça m’aide. J’ai essayé d’utiliser le pattern MVC mais je suis même pas sûr de l’avoir bien implémenté.

Il utilise sont propre système, paramétré par les différent menu de l’IDE et décrit dans le fichier projet .cbp. je ne sais pas s’il est possible de mettre ce fichier projet dans le dépôt ou s’il contient des chemins absolus. Même si c’est possible, c’est un peu dommage car il n’y a que code::blocks qui utilise ce format, ça peut être une contrainte projet défendable mais avec les alternatives qui existent c’est un peu dommage. à l’inverse, avec un CMake, tu peux tout à fait générer un projet code::blocks pour travailler avec s’il s’agit de ton éditeur favori.

le pattern MVC est plutôt adapté à des IHM. Puisque le morpion peut s’assimiler à une IHM c’est peut-être pertinent, même si c’est pas le cas de tout les jeux vidéos.
D’ailleurs j’en profite pour dire, puisque je ne l’ai pas encore fait, si le but est de faire des jeux vidéos, alors il vaut mieux utiliser un moteur de jeu, si le but est d’apprendre C++ au travers des projets jouets, pas de soucis.

Memes remarques que romantik pour le systeme de build et l’organisation.

Pour le MVC, je pense que le C(ontroller) n’a pas trop d’usage dans un jeu. D’ailleurs, ce que tu as mit dans le dossier controller n’est pas des controllers. Et si on garde que M et V, on est plus sur une architecture 2 couches assez classiques. Si cela t’intéresse, voici un exemple d’architecture multi-couches pour les jeux (du livre "Game Engine Architecture") :

Mais ce sont des choses qui s’apprennent avec l’expérience. C’est normal que tu n’as pas encore une idée claire de comment architecturer un projet, si tu n’as jamais bossé sur des projets réels.

Quelques details.

https://github.com/BizohxDev/OXO/blob/main/main.cpp#L55 C’est vraiment du détail, mais ne pas être consistant dans le code (espace entre la parenthèse et le crochet), c’est le genre de choses qui montre en général un dev débutant. Un autre exemple : https://github.com/BizohxDev/OXO/blob/main/model/GlobalConstants.hpp#L7. Pourquoi définir certaines constante avec #define et d’autres avec const ?

Pour les constantes, utilise static constexpr.

https://github.com/BizohxDev/OXO/blob/main/model/MFile.cpp#L8 Les blocs de commentaires comme ça, c’est lourd a la lecture. Et ca sert pas a grand chose en pratique.

https://github.com/BizohxDev/OXO/blob/main/model/MLogs.hpp#L4 Utilise des forward declarations.

https://github.com/BizohxDev/OXO/blob/main/main.cpp#L28 C’est le genre de chose que tu peux déclarer en une ligne

const std::vector<Screen*> screens = { &homepage, &credit, ... };

https://github.com/BizohxDev/OXO/blob/main/controller/Game.cpp#L11 Tu peux simplifier ton code en utilisant les algos de la lib standard (par exemple std::fill) ou des range-for loop.

https://github.com/BizohxDev/OXO/blob/main/controller/Game.cpp#L67 Un autre classique des devs débutants : la qualité du code. Tu n’as pas de tests, pas de vérification des contrats, etc. Regarde la programmation par contrat, les préconditions, les postconditions, assert. Imagine que quand tu écris une classe, c’est quelqu’un d’autre qui va l’utiliser et tu veux lui signaler quand il fait une erreur. Par exemple, si les valeurs de row, column et symbol sont valides. Par exemple :

void Game::playCell(int row, int column, char symbol) {
    assert(row >=0 && row < 3 && column >= 0 && column <3
           && (symbol == ' ' || symbol == 'O' || symbol == 'X')); 
    m_grid[row][column] = symbol;
    ...

Je sais pas quelles valeurs sont valides dans ce cas, mais c’est l’idée.

D’ailleurs, probablement utiliser un enum plutôt que char pour les symbols. Ca permet de donner une sémantique (char est un caractère, pas un symbole).

Tes classes Screen ont une seule fonction, de plusieurs centaines de lignes. Il faudrait mieux découper.

Si tu as lu les cours de base, je te conseille Tour of C++ puis Professional C++ ensuite.

Alors désolé par avance @Bizohx, je n’ai pas regardé ton code, je voulais poser une question à @gbdivers sur sa précédente intervention.

https://github.com/BizohxDev/OXO/blob/main/model/MLogs.hpp#L4 Utilise des forward declarations.

Je vois un #include <string> à cette ligne, comment fait-on une déclaration anticipée de std::string ?
Je pose la question puisque je n’ai jamais vu quelqu’un le faire, et j’ai cru comprendre ici que ce n’était pas vraiment possible / conseillé.

Oui, tu as raison, j’ai confondu avec un autre forward de std. (EDIT : cela dit, j’ai pas fait attention au reste du code, c’est une remarque générale, qui s’applique peut etre quand meme)

Merci beaucoup pour ces retours et conseils ! J’en prends note et tâcherai de les mettre en pratique lors de mon prochain projet

Hello,
Je rejoins les avis de romantik et gbdivers. Et j’ajouterai aussi qu’en bonus, pour ce qui est de la qualité du code, tu peux déjà commencer par augmenter le niveau de warning de ton compilateur (si tu ne l’as pas déjà fait). Il sera tout à fait capable de te remonter des comportements dangereux (variable non initialisée par exemple) ou bien des choses plus trivial comme des variables non utilisées.

En complément tu peux pousser l’analyse encore plus loin avec des analyseurs static tel que cppcheck ou clang-tidy qui pourront aller encore plus loin (étant donné qu’ils n’ont pas la contrainte du temps de compilation).

Tu ne vas pas corriger celui ci ?

Salut,

Quelques remarques

Screens

Découper par écran n’est pas une mauvaise idée, c’est ce qui s’apparente à des "Scenes" dans les moteurs de jeux. Cependant tes classes Screen sont de fausses classes, ce ne sont que des fonctions déguisées, tu pourrais faire la même chose en donnant l’index dans les arguments plutôt qu’utiliser les méthodes virtuelles. Et dans cette fonction, elles gèrent chacune l’initialisation puis la boucle. Je te propose plutôt de concevoir les Screen un peu comme tu as conçu les boutons. Ce sont des classes qui devrait pouvoir se dessiner, et réagir à un évènement. Ta boucle d’évènement se situerait au niveau du main et se propage dans l’écran courant, ça t’éviterai de dupliquer du code comme les évènements de la fenêtre (fermeture), la limite de rafraichissement etc…
Si tu as cette approche, il faut aussi que tu aie conscience que tu vas charger tes éléments vraisemblablement dans le constructeur, et donc le fait d’instancier tout tes écrans au début va tout mettre en RAM. Là que tu navigues entre 5 écrans c’est trois fois rien mais sur un jeu qui multiplie les tableaux genre un plateformer 2D, t’as pas envie de tout charger alors que tu te sers d’un seul tableau.

MVC

Effectivement, c’est pas du tout un MVC, désolé ^^'
Ce qui s’approche le plus d’un model dans ton archi est Game, qui contient la grille et est appelée par la vue ScreenGame pour être mis en forme. Dans cet échange, le modèle n’a pas à se préoccuper de la mise en forme, or ici il le mets sous forme de wstring pour faire plaisir à la vue. ça va rejoindre un point un peu plus bas, mais dans cette grille, chaque case n’a que trois états, vide/joueur1/joueur2, pourquoi représenter ces trois états par un wstring ou un char qui peuvent prendre bien plus de valeurs, qu’il faudra gérer ensuite comme des cas d’erreur, un enum me semble tout indiqué ici.
Game a aussi tout un tas de logique du jeu, je suppose que c’est pour ça qu’il a fini dans controller. Mais il ne fait pas le rôle de controller qui test la validité de l’action de la vue, la vue se charge elle même de savoir qu’est-ce qui est déjà joué, à quel joueur c’est etc…
Enfin hésites pas à plus découper ta vue en composants comme le bouton (et en fait c’est assez valable sur tout le code qui regroupe généralement trop de responsabilité aux mêmes endroits).

Inclusion

Inclut les headers qui ne viennent pas de ton code, tel que la lib sfml, avec les chevrons <>. Ainsi on peut modifier les chemins de recherche pour pouvoir utiliser par exemple un gestionnaire de paquet.
Evite de naviguer dans l’arborescence de fichier "../model/machin.h", c’est à ta configuration d’indiquer à partir d’où chercher des headers et ainsi tu peux inclure "model/machin.h".

Déclaration de variable

Effectue la déclaration d’une variable au plus proche de son utilisation, avec une initialisation. ça te permettra d’avoir une initialisation qui a du sens, et une portée ainsi qu’une durée de vie minimale.

Linéarisation de matrice

Tu peux représenter un tableau de tableau [NB_ROW][NB_COLUMN] par un seul tableau [NB_ROW*NB_COLUMN] et accéder à [i][j] par [i*NB_COLUMN+j] quand ton nombre de colonne est fixe. Préférer les classes C++ pour les collections std::array, std::vector …

Ajouter de la sémantique (enums et constantes à la place de magic numbers)

Tu dois toujours de poser la question de quelle est la meilleure représentation de ton information, celle qui a le plus de sens. Tu as déjà eu le réflexe de mettre un tas de constante, mais tu as encore plein d’endroit où des nombres se baladent. Et il y a plusieurs données qui pourraient facilement devenir des enum pour des statuts ou l’index des écrans par exemples…
Pour les constante ça a déjà été dit mais préfère static constexpr qui type ta donnée.

Et cadeau, je te laisse le fichier que je me suis rajouté pour compiler ton projet. Il faudrait mieux gérer les ressources, je l’ai écrit juste pour pouvoir faire un xmake run, mais au moins tu vois que ce n’est pas très compliqué.

add_rules("mode.debug", "mode.release")

set_languages("cxxlatest")
set_warnings("allextra")

add_requires("sfml")

target("OXO")
    set_kind("binary")
    add_packages("sfml")
    add_includedirs(".")
    add_files(
        "main.cpp",
        "model/*.cpp",
        "view/*.cpp",
        "controller/*.cpp"
    )
    add_extrafiles("src/**/*")
    after_build(function (target)
        local resdir = path.join(target:targetdir(), "src")
        if not os.exists(resdir) then
            os.cp("src", resdir)
        end
    end)
    on_clean(function (target)
        local resdir = path.join(target:targetdir(), "src")
        os.rm(resdir)
    end)