Premier jeu en C++ et 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 , 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 : https://github.com/BizohxDev/OXO
Télécharger le jeu compilé (Windows) : ICI

Ce serait plus simple si le code était disponible directement ici ou à la limite sur un truc genre github.

En effet c'est sans doute mieux, j'ai ajouté le lien GitHub

Bonjour,

Ah ! Amusant, votre dossier 'src', contient les ressources (images, sons...). Généralement, le dossier 'src', contient, les sources :D. Les médias, ça va dans un dossier "res" ou "resources".
Chose que l'on peut améliorer : mettre en ligne le fichier du projet/Makefile, ou encore mieux, un fichier d'un générateur de projets (CMakeLists.txt d'un CMake ou autre).

Dans le main:

Code:

std::vector<Screen*> screens;

Pourquoi des pointeurs bruts ? Ou même, pourquoi ds pointeurs. Je pense que l'on peut les éviter.

Dans le dossier view, j'aurais fait un sous dossier screens, du coup.

Code:

ScreenHomepage::ScreenHomepage() = default;

Lorsque l'on utilise le default, on le déclare ainsi, dans le .h, directement.

Chaqu'un de vos Screen implémentent le chargement des ressources et une boucle principale. Il y a plusieurs solutions possibles, mais :

le chargement des ressources pourrait être dans une fonction dédiée (pour un peu réduire la taille de run()), mais aussi, car on aimerait peut être faire un chargement d'une bonne partie (voire toute) les ressources du jeu, au démarrage de celui-ci.
on aimerait n'avoir qu'une seule boucle principale (dans le main()), qui va exécuter le code qu'il faut de l'écran. D'ailleurs, c'est pas mal expliqué ici : https://alexandre-laurent.developpez...boucle-de-jeu/

Ceci dit, j'aime bien l'idée d'avoir fait des Screen et une machine à états.
Code:

1 2 const int Size_textNamePlayer(2); sf::Text textNamePlayer[Size_textNamePlayer];
Je verrais bien des std::array et des for(auto var : array) (je crois que cela s'appelle range-for :aie:).
Code:

1 2 3 // Event queue processing while (app.pollEvent(event)){ switch(event.type){
C'est un design que je n'aime pas :aie:. Note : c'est un avis personnel. Je préfère récupérer les événement au besoin, et non pas dépilé les événements reçus moi même. Notamment, je peux bloquer votre application en déplaçant la fenêtre (le déplacement provoque souvent un envoi d'événement de déplacement de fenêtre pour chaque pixel. Bref, ça spam à mort. Pareil lorsque l'on fait bouger la souris.

Vous avez un modèle MVP (d'après le nom des dossiers). Ce n'est pas un design que l'on fait pour les jeux vidéo :aie:.

Cette variable dans le main:

Code:

const int Size_playersNames(2);

doit avoir la même valeur que les variables dans les screens, non ? Dans un tel cas, on a souvent un fichier de nombre magique, appelé globals.h :aie:. Cela peut se faire aussi s'implémenter en variable constante statique de classe. Bref, avoir un endroit où la valeur est définie et tout le monde se réfère à cette valeur.
Ah ! vous avez un fichier GlobalConstants. Bah, c'est la même idée. Vous pouvez ajouter des namespace pour faire un peu d'ordre dans ce fichier, notamment, pour regroupper tout ce qui concerne l'UI. Et comme c'est de l'UI, faire un fichier dédié peut être une bonne idée, à placer dans le dossier view.
Aussi, vous avez un petit effet comique, c'est que votre fonction run, accepte deux variables, défini à l'extérieur (dans le main). Elles le sont, car vous en avez besoin dans plusieurs écrans. Je pense que j'aurai opté pour une classe pour contenir ces informations.

Dans MRandom, je ne sais plus si cela à un coup de recréer à chaque fois votre device/mt/distribution. La classe devrait être en mesure de conserver tout cela, afin d'éviter de recréer à chaque génération d'un nombre.
Code:

1 2 3 /** * * * * * * ** * GETTER * ** * * * * * * **/
Inutile.
Cette information, je l'ai juste en lisant le nom de la fonction (ici getText). Pas besoin d'alourdir le fichier. D'ailleurs, pour le MFile::getText(), l'implémentation aurait simplement pu être dans le fichier .h
Pareil pour le setText. Les fonctions d'une ligne, on peut se permettre de les mettre dans le .h directement. Pareil pour les constructeur vide.

La taille de la grille est aussi en nombre magique (3). En réalité, ce n'est pas très grave, car on peut se dire simplement, la taille ne changera jamais. C'est vrai et il n'y a pas trop de mal à ça et je pense qu'il faut se donner certaines limites quant à la modularité que l'on veut atteindre. Toutefois, je crois qu'il aurait été plus simple d'avoir une classe dédiée à la grille, et peut être avoir implémenté un tableau plat. D'autant plus que grâce à la méthode getCell(), la différence serait minime pour le reste du code. Et avec une méthode getCell, vous pouvez ajouter des gardes fous pour repérer les erreurs de programmation au plus tôt (des assertions, notamment).
Pourquoi un tableau plat:

beaucoup plus facile à parcourir (notamment pour voir si toutes les cases ont été jouées) ;
le getCell permet d'avoir une interface propre (utilisation d'un système de coordonnées ligne/colonnes), peu importe la structure contenant les données derrière ;
permet la taille que l'on veut ;
permet une utilisation des fonctionnalités standard (algorithm)
les tableaux 2D, bof bof (et encore, le votre est d'une taille fixe, sans allocation dynamique)

Ce code :
Code:

1 2 3 4 do { row = randomInt(0, 2); column = randomInt(0, 2); } while (m_grid[row][column] != ' ');
En cas de malchance interplanétaire, cela fait freeze votre programme :aie:. On aurait pu simplement scanner les cases libres, piocher dans l'une d'entre elle et hop, on a une case aléatoire.

La grille est composé de char. Vous auriez très bien pu utiliser un enum, rendant le code plus clair.

Chaque écran charge certaine même ressources (les polices) :triste:
Chaque écran affiche une liste de Drawable, pourtant, chaque écran possède un code du style :
Code:

1 2 3 4 5 6 7 8 app.clear(); app.draw(bgSprite); app.draw(title); app.draw(subtitle); app.draw(creditsText); app.draw(buttonBack); app.display();
Un écran pourrait très bien juste renvoyer/remplir une liste de choses à afficher, tout simplement. Et un code commun ferait le clear/draw/display.

Un Screen va renvoyer l'index d'un autre screen. Cela aurait pu être un enum et l'autre problème, c'est que c'est dépendant de l'ordre d'insertion des Screen fait dans le main.

Voilà. Il y a peut être d'autres choses à dire, mais il faut retenir les points suivants :

vous avez fait un jeu qui fonctionne. C'est déjà un très grand succès pour vous même et vous pouvez en être fier ;
les retours que j'ai fait, viennent de l'expérience. Il est normal que son premier jeu soit codé comme vous l'avez fait (je pense que le mien était même pire :aie:). C'est en programmant que l'on devient programmeur (et que l'on acquiert l'expérience). Si cela vous tente, faites une branche et tentez de modifier le code suivant mes retours. Si cela ne vous tente pas, faites un autre projet et tentez quelques solutions que j'ai proposé.
j'ai remarqué notamment que le code était plus proche du C, que du C++. Essayez d'utiliser quelques fonctionnalités de base, notamment : algorithm, std::array/std::vector, les ranged for et même std::optional (pour le selectedPage, par exemple).

Encore une fois, félicitations.