Discussion :

[NainTernaute]

Bonsoir à tous,

Je viens de "finaliser" mon Tetris amateur qui est en quelque sorte mon premier vrai projet en C++, et j'aimerais beaucoup avoir vos avis sur mon code. Ce que je pourrais améliorer, la manière d'utiliser l'objet, etc... Afin de produire un code toujours meilleur.

Le Tetris en lui-même n'est pas beau, et se contente des fonctions basiques mais ce n'est pas ce qui m'intéresse ; je recherche avant tout des remarques à propos de mon code.

Voici le lien pour dl le projet : http://www.jeremy-dutheil.fr/projects/Tetris.rar

Merci d'avance !

[oxyde356]

J'ai pas était hyper minutieux pour regarder ton code mais personnellement je le trouve très bien, il est propre et efficace, facile à lire. Mais je te conseillerais tout de même de rajouter des commentaires, sur-commentes les .h et commentes les algos des cpp. Niveau technique je te conseillerais d'utiliser les smart pointers (auto_ptr ou intrusive_ptr) au lieu de n'utiliser que des raw pointers. A pars ça je te conseille d'essayer un projet plus dur maintenant, par exemple en codant un petit moteur de jeux 2D, avec gestionnaire de ressources, ouverture au script (python ou lua ...) gérer les erreurs, prévoir différentes type de scene, la direct dans ton jeu on rentre dans une scene ou se déroule directement le gameplay, fais en sorte que ton moteur puisse afficher une image d'abords, puis un menu principal, un sous menu, puis allez sur la scene de gameplay (le jeu en soi) puis revenir sur le menu etc ... tout ça devrais te faire poser les bonnes questions pour évoluer ^^

[NainTernaute]

Je ne connaissais en fait pas les smarts pointers, mais depuis que je me suis renseigné ça a en effet l'air sympathique. Je compte bien me lancer dans un projet un peu plus compliqué maintenant, oui !

Merci pour tes commentaires.

[LittleWhite]

Bonjour,

Il n'y a vraiment pas grand chose à dire.
Le code est bien structuré, propre.

Il manque peut être juste que des commentaires ( et des commentaires pour la génération automatique de la doc ).

Sinon, mais c'est parce que je suis pénible... dommage qu'il n'y ai pas de Makefile Linux .

[oxyde356]

Ouai je confirme là c'est parce que t'es pénible

[lagwag]

c'est une horreur ca!...

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
void Piece::rotate() {
	(m_rotationId >= 3) ? m_rotationId = 0 : m_rotationId++;
}

le ?: est tres pratique mais là c'est illisible (modifier les variables à l'interieur)

ecris plutot (si ya 4 possibilités)

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
if ( ++m_rotationId>=4 )
  m_rotationId = 0;

---

dans Graphics.h, tu devrais utiliser des pointeurs ou references car tu perds un temps fou à chaque fois que tu appelles ces fonctions (une copie est effectuée à chaque fois)

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
void draw(Piece piece)
void draw(Board board);
void drawUi(Piece nextPiece);

---

dans Game.cpp

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
float time = Clock.GetElapsedTime();
if (time > 0.5)

il faut éviter d'utiliser des constantes double avec des variables float...
c'est du temps perdu à chaque conversion, pour un jeu ca peut jouer sur le fps. ecris plutot:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

if (time>0.5f)

---

tu ne differencies pas les pointeurs dans tes noms de variables?
m_board est pointeur (il faut le savoir)
alors que si tu le nommes m_pBoard (ou autrement)... aucune confusion possible

---

pour des jeux plus compliqués, il te faudra un Finite State Machine

[NainTernaute]

Merci pour ce long poste riche en enseignement. En effet pour le premier point, je n'y aurai pas pensé.

En ce qui concerne le passage de paramètres par pointeur ou copie, je pensais qu'il était plus "logique" de passer par copie si on n'a pas besoin de modifier la véritable variable (elle est juste utile à la fonction). N'est-ce pas le cas ?

Je ne savais pas non plus que "0.5" était de base considéré comme un double, mais je pensais qu'il l'était comme un float. C'est bon à savoir.

Ok aussi pour les pointeurs.

Et pour le point final, je vais me renseigner sur ce qu'est un Finite State machine.

[Limuath]

En ce qui concerne le passage de paramètres par pointeur ou copie, je pensais qu'il était plus "logique" de passer par copie si on n'a pas besoin de modifier la véritable variable (elle est juste utile à la fonction). N'est-ce pas le cas ?

Sinon tu peux passer le paramètre avec une référence constante. Tu évites ainsi la copie (potentiellement couteuse) tout en garantissant la non-modification du paramètre.

[lagwag]

je ne pige pas un truc dans ton code de la classe Game.
tu crees des instances Piece mais je ne vois nulle part où tu les detruis à part dans le destructeur qui ne detruit que les dernieres instances.
as tu un tableau qui stocke les references de toutes ces instances?

si tu les supprimes pas, va yavoir des fuites de memoire. Pour tester, utilise ce code en mode debug seulement

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
#ifdef _DEBUG
#include <crtdbg.h>
#endif
 
_CrtMemState memStateInit;
_CrtMemCheckpoint(&memStateInit);
 
Game* pGame = new Game;
pGame->play();
delete pGame;
pGame = NULL;
 
_CrtMemState memState, memStateDiff;
_CrtMemCheckpoint(&memState);
if ( _CrtMemDifference(&memStateDiff, &memStateInit, &memState) )
	=> pas bon

---

pour board, ca veut dire que le tableau entier sera copié, soit 20*10=>200 int à copier

effectivement Limuath a raison, tu peux rajouter le mot clé const
moi j'ai une preference pour les pointeurs

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

void Draw(const Piece* pPiece)

---

voici d'autres remarques mais là c'est different, chacun fait comme il veut. donc prends ce que tu veux...

j'utilise des defines pour les constants globales de type int, float, double et string.

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
#define BOARD_HEIGHT	20
#define BOARD_DOUBLE	20.0
#define BOARD_FLOAT	20.0f
#define BOARD_STRING	"hello"

Pour les noms des classes, j'utilise une lettre en majuscule avant, ca permet de savoir si c'est une classe: CPiece par exemple

Pour le noms des fonctions/methodes j'utilise une majuscule pour ne pas confondre avec les variables locales: void Draw(CPiece* pPiece) par exemple

Pour les pointeurs, le lettre p.

J'ai mets generalement toujours le mot clé virtual aux destructeurs meme si ca prend 4 octets de plus sur un systeme 32bits... ca evite de s'arracher les cheveux lorsqu'on rajoute une classe derivée par la suite... pour les classes qui ne stockent que des donnees, pas besoin, mais dans ce cas j'aurais plutot une preference pour les structures (question d'habitude)

et je n'utilise pas les smart pointeurs

[Seb2086]

Salut je suis moi même en train de découvrir la SFML et j'ai également pour "projet" de faire un tetris je trouve le tiens très réussi bravo.

J'aurais juste une question à ceux qui savent :

Au niveau du nommage des variables pourquoi on écrit m_var à quoi correspond le m.

Y'a t'il une nomenclature "standard" à respecter ?

[lagwag]

m pour member... variable membre de la classe

chacun choisit sa convention
certains n'utilisent que le underscore (sans le m), c'est le cas de microsoft

le principal c'est de s'y retrouver

[Seb2086]

merci beaucoup et encore désolé pour le hors sujet

[NainTernaute]

Merci pour toutes ces remarques lagwag, en effet j'ai encore un peu de mal à penser "libération de mémoire". ^^
En ce qui concerne les define vs const, j'ai lu pas mal de trucs là-dessus et préfère utiliser les const.

Ne vous étonnez pas de ne pas voir évoluer ce projet, j'avais posté pour avoir des remarques (sur un autre forum, ça a été déplacé) dont je pourrai tenir compte dans un futur projet plus important.

[lagwag]

no prob

bon courage pour la suite

[Ivelios]

je pensais qu'il était plus "logique" de passer par copie si on n'a pas besoin de modifier la véritable variable (elle est juste utile à la fonction). N'est-ce pas le cas ?

Si tu a une méthode qui prend un string en paramètre. Il ne faut pas passer le string par copie.
Imagine que string = bible ou coran....
Je ne te raconte pas le temps que tu perd pour copier alors que ça ne sert à rien.

Désolé, petit HS pour éclaircir la situation et compléter la réponse de Limuath

[Kalith]

c'est une horreur ca!...

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
void Piece::rotate() {
	(m_rotationId >= 3) ? m_rotationId = 0 : m_rotationId++;
}

le ?: est tres pratique mais là c'est illisible (modifier les variables à l'interieur)

ecris plutot (si ya 4 possibilités)

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
if ( ++m_rotationId>=4 )
  m_rotationId = 0;

Juste mon point de vue, mais le code que tu proposes est pire encore.
Ecrire quelques lignes de plus ne tue personne :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
++m_rotationId;
if (m_rotationId >= 4)
    m_rotationId = 0;

Si l'on est vraiment avare en ligne, on écrira plutôt (mais c'est toujours moins lisible) :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

m_rotationId = (m_rotationId + 1) % 4;

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
float time = Clock.GetElapsedTime();
if (time > 0.5)

il faut éviter d'utiliser des constantes double avec des variables float...
c'est du temps perdu à chaque conversion, pour un jeu ca peut jouer sur le fps. ecris plutot:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

if (time>0.5f)

Là je pense que le compilo optimise tout ça pour toi. Mais j'ai la même habitude : ça a l'avantage d'insister sur le fait qu'on travaille en float et pas en double.


Mais bon, tout ça c'est du pinaillage.

L'essentiel, comme il a été dit, c'est d'utiliser le passage d'argument par références constantes plutôt que par copie : c'est tout bénef'. Tu peux utiliser les pointeurs, mais tu perds les garanties qu'offrent les références, au profit d'une plus grande flexibilité.

Pour la gestion de la mémoire, si tu es rigoureux tu peux t'en occuper toi même. Mais ça implique de bien réfléchir à la durée de vie de chaque objet.
La voie de la facilité et de la sûreté, c'est les smart pointers.

Dernière remarque, au niveau des #includes.
Beaucoup d'entre eux devraient se trouver dans les fichiers .cpp plutôt que dans les .h.
Tu peux en supprimer la majeure partie en utilisant les déclarations anticipées (forward declarations).

Exemple avec Game.h : tu peux virer

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
#include "Board.h"
#include "Piece.h"
#include "Graphics.h"

... en rajoutant :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
class Board;
class Piece;
class Graphics;

L'avantage immédiat, c'est qu'on a pas à recompiler tous les fichiers qui utilisent "Game.h" quand on modifie "Piece.h", "Board.h" ou "Graphics.h".

[lagwag]

si j'ecris ca, je me prends un warning dans la gueule

FLOAT f = 10.111;

warning C4305: 'initializing' : truncation from 'double' to 'FLOAT'

c'est pas qu'une question de rapidité, ca pose aussi un probleme d'arrondi

----

pour m_rotationId, ton code est juste plus lent que le mien
franchement pour une condition aussi simpliste, ca n'a aucun interet de rajouter une instruction supplementaire
yaurait plusieurs comparaisons dans le meme if, ca serait different, je le ferai pas

et le modulo c'est n'importe quoi
c'est moins lisible et c'est encore plus lent

[Kalith]

si j'ecris ca, je me prends un warning dans la gueule

FLOAT f = 10.111;

warning C4305: 'initializing' : truncation from 'double' to 'FLOAT'

c'est pas qu'une question de rapidité, ca pose aussi un probleme d'arrondi

Pas de warning sous GCC, pourtant le bougre est bavard.
Les conversions sont certes couteuses, mais si l'on passe d'une variable à une autre. Au niveau des constantes c'est géré à la compilation, vérifie le code assembleur généré si tu veux.

pour m_rotationId, ton code est juste plus lent que le mien
franchement pour une condition aussi simpliste, ca n'a aucun interet de rajouter une instruction supplementaire
yaurait plusieurs comparaisons dans le meme if, ca serait different, je le ferai pas

et le modulo c'est n'importe quoi
c'est moins lisible et c'est encore plus lent

Comparer la rapidité de deux lignes de code comme celles là n'a aucun sens dans cette situation. Ce n'est clairement pas une partie critique du programme, alors j'aurai tendance à écrire le code le plus clair possible et laisser le compilo faire son boulot.
Après, chacun dispose de sa définition du "code clair". La mienne est peut être trop sévère.

[oxyde356]

En effet sauf quand on est obligé il ne faut pas utiliser de double dans un jeu, c'est vraiment vraiment très gourmand, que ça soit pour calculer ou pour caster en autre chose.
Perso des 3 méthodes autour du rotationId je préfère celle avec le modulo, mais c'est peut-être parce que j'ai l'habitude. J'ai fais un petit bench et la méthode la plus performante et celle qui utilise le if, s'en suit pas loin après la méthode avec le modulo et en gros dernier loin derrière le ?:
Perso un truc comme ça avec un modulo et un commentaire juste avant j'aime bien mais bon si la méthode du if te parait claire hésite pas c'est la plus rapide.

Voici les résultat du bench si ça intéresse quelqu'un (pour 10^9 itérations) :
Temps avec if = 1992ms
Temps avec modulo = 2298ms
Temps avec ?: = 3650ms

(tests effectués sous Windows 7 - 64bits et Visual Studio 2008 Pro en mode debug (optimisation désactivées)

En release avec toutes les optimisations activées la méthode avec le modulo prend 0ms (avec le même nombre d'itérations), ce qui veut dire que l'IDE à optimiser la boucle j'imagine en la faisant à la compilation, car si dans le code je met un "cin >> rotationId" juste avant le temps redevient normal (un peu plus long qu'avec if), visual studio n'ayant pu déterminer le résultat de la boucle à la compilation vu que celui-ci dépend de l'entrée utilisateur. Bref il ne faut pas en tenir compte ^^
A propos de ce bench c'est intéressant alors je le partage (c'est toujours utiles des benchs bas niveaux ^^) mais c'est lourdement H.S.

[lagwag]

un autre probleme par rapport au type de la constante:

float+1.0

convertira le float en double car le compilateur ne peut pas connaitre l'intention du programmeur

donc meme si sur 1 cas sur 10, le compilateur convertit correctement le double en float, il vaut mieux TOUJOURS preciser explicitement le type pour les constantes