Discussion :

[devdeb91]

bonjour

alors voila enfin un premier jet du projet rpg en c++ aprés quelque semaine que je vous bassine avec mes questions. les sources sont téléchargeable.

si vous avez des commentaires ou reflexion ou idée ou autre je suis ouvert a tout.

bug constater au niveau de la suppression des monstre et raison trouver mais pas encore corriger.

voila je vous laisse apprécier.

[LittleWhite]

Bonjour,

Je ne comprend pas la raison de la présence de deux .h dans le dossier "doxygen". En théorie, doxygen doit lire les fichiers de code et non des fichiers à part.

Personnellement, j'aime bien mettre les fichiers sources (.c et .h) dans un dossier "src" afin qu'ils ne soient pas mélangés avec les fichiers du projet ou les fichiers de données. Les fichiers de données, j'aime bien qu'ils soient dans un dossier "data". Mais ce ne sont que des avis personnels.

Attention, "Personnage.h" possède une majuscule alors qu'il est inclut sans majuscule dans les autres fichiers. Sachant que tous les autres fichiers sont sans majuscule, je propose de renommer le fichier en "personnage.h". La raison de cette remarque est que sur un système stricte qui respecte correctement la casse (comme Linux), la compilation renvoie des erreurs (comme actuellement sur mon système). Sous Windows, cela n'est certes pas le cas, mais autant faire bien (bien ==> que ça marche chez moi ).
Autre problème avec Windows VS Linux. Vous avez fait une inclusion de windows.h (dans main.h) qui est un fichier spécifique à Windows. Mais bon, il est nécessaire vu que vous utilisez les fonctions de la WinAPI je ne pourrais donc pas tester .
Il semble aussi qu'il reste quelques avertissements (warnings) dans le code. Il faudrait réellement songé à les corriger.

J'ai remarqué que vous avez de nombreux include dans main.h alors que ceux-ci pourrait être effectué dans les fichiers qui en ont réellement besoin (main.cpp, par exemple). En effet, il faut minimiser le nombre d'inclusion qui sont effectuer dans les fichier .h (afin de la compilation soit plus rapide, entre autre, mais aussi que le code soit plus modulaire (en effet, à cause des inclusions dans les .h ont a tendance à ne pas les faire dans les .cpp faisant que si vous réutiliser votre code, il ne va pas trouver les fichiers nécessaires)

main.h contient des structures qui pourraient être déplacé dans un fichier qui leur est propre (je déteste les fichiers qui centralisent tout, du genre "common"). Par exemple, un fichier SMap.h/SMap.c pour la gestion de SMap. De plus, SMap est définit comme une structure laissant tous les membres à l'air libre, mais possède aussi des fonction du genre tileAt(). En fait, il faudrait transformer SMap en classe, rendre les membres privés afin que personne (aucune classe ou développeur externe) ne puisse modifier le contenu de la map sans en avoir autorisation par la Map elle même. En effet, la Map doit être géré que par la map. Si un élément externe modifie la Map, il peut arriver que celui-ci fasse n'importe quoi (et puis sa violerai certains principe d'encapsulation et autre que koala01 ne se gênerait pas de rappeler ).

Un autre détail :

Code :

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

1
2
SMonstre(std::string nom, int hp, int mp, std::string type, int exp, int gold):
            nom(nom), hp(hp), mp(mp), type(type), exp(exp), gold(gold){}

À chaque appel à ce constructeur, la std::string passée sera copier (la copie d'un objet est une opération qui peut être lente). Il est donc conseiller de définir les paramètres nom et type comme des const std::string& afin d'éviter la copie. À l'utilisation, cela ne change rien, mais la copie sera bel est bien éviter. (voir la FAQ : http://cpp.developpez.com/faq/cpp/?page=references pour quelques informations supplémentaires).
Cette remarque s'applique pour tout le code et non que pour ce constructeur.

Code :

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

1
2
3
std::vector<unsigned char> tiles;
	std::map<Position, SWrap> wraps;
	std::map<Position, SCoffre> coffres;

Essayez de choisir un seul langage lorsque vous nommez vos variables. Ici, on voit un mélange de français et d'anglais.

Code :

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

1
2
3
4
void tilegamer(Joueur & joueur)
       {
           tiles[joueur.y()*cols + joueur.x()] = '@';
       }

Ici, la référence (qui évite bien la copie) devrait être constant, car le joueur n'est pas modifié. Le problème (j'imagine, car je n'ai pas vérifié) c'est qu'en le faisant, le compilateur va raler en disant que vous utilisez des fonctions non constantes (y() et x()). Comme les fonction y() et x() ne devrait pas non plus modifié le joueur (car ce sont des getters/accesseurs), vous pouvez les définir comme fonctions constantes.

Code :

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

1
2
size_t x() const{return position_.x();}
         size_t y() const{return position_.y();}

super, elles le sont déjà :clap:

Essayez de respecter une convention de nommage sur tout le projet. Je vois des classes qui ont des noms qui commence avec une majuscule (ce qui est "bien"), mais je vois aussi personnage et enemy sans majuscule (ce qui n'est pas normalisé avec le reste du code).

Le destructeur de personnage doit être virtuel, car la classe est hérité. Explication : http://cpp.developpez.com/faq/cpp/?p...UCTEUR_virtuel

Code :

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

1
2
3
4
5
Deplacement(int x, int y):distanceX_(x),distanceY_(y)
        {
             assert(x>=-1 && x<=1);
             assert(y>=-1 && y<=1);
        }

Ce choix devrait être documenté (doxygen) ou commenté. Il peut y avoir une autre solution, c'est de tronqué la valeur lorsqu'elle est plus grande (

Code :

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

1
2
 (if x > 1) x=1; 
// ...

)

Sachant que l'assertion ne sera pas présente lorsque le code est compilé en release. Mais soit, c'est un choix qui se défend.

Code :

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

1
2
3
4
5
void enemy::afficherinfo()
{
    std::cout << m_nom << " " << m_hp << " " << m_mp << " " << m_type << " "
        << m_xp << " " << m_argent << std::endl;
}

Il était aussi possible de surcharger l'opérateur << pour la classe. Mais c'est aussi un choix.

Code :

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

1
2
char *etat = "proteger";
    if(strcmp(etat, m_etat) != 0)

Ouch, vous êtes en C++ (11) et vous avez encore des strcmp/char*
etat peut être défini comme un std::string et pour la comparaison il suffira d'utiliser l'opérateur ==.

Ouch * 2 m_etat est aussi un char* alors qu'il pourrait être un std::string.

Code :

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

m_etat = "normal";

Et même, pour faire du C correct, il faudrait que le pointeur soit constant (const char*). Mais bon, la question ne se pose pas lorsque l'on utilise des std::string.

Code :

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

m_etat("normal")

Comme vous utilisez plusieurs fois "normal", on peut imaginer une table de string, ou une énumération au lieu d'avoir une répétition de la clé en dur. Ce qui évitera des erreurs. On peut aussi imaginer des choses plus complexes mais cela dépend du besoin (qui a parlé de classe de politique, de design pattern de stratégie ? (mais nous verrons cela un peu plus tard )).

Code :

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

1
2
3
4
5
6
7
void player::levelup()
{
    m_level = m_level + 1;
    m_nextlevel = m_nextlevel * 1.5;
    m_maxhp = m_maxhp * 3/4;
    m_maxmp = m_maxmp * 3/4;
}

Beaucoup de nombres magiques que je mettrais en variable statique/constante membre de la classe.

Code :

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

void afficherinfo();

afficherinfo() et tout autre fonction d'affichage peuvent être des méthodes constantes.

Je trouve le main imposant. Il doit y avoir des possibilités de le décomposer. De plus, il faudra tot ou tard instauré une boucle de jeu (game loop) afin que la gestion du jeu soit bien structurée. (exemple simple : http://fearyourself.developpez.com/t...l/sdl/morpion/ ou http://fearyourself.developpez.com/tutoriel/sdl/pong/ ; exemple plus complexe : http://khayyam.developpez.com/articl.../architecture/ )

J'ai remarqué que le main possède une grosse partie de copier coller, ou seul la Direction change (copier coller pour chaque touche appuyée). Cela est fortement simplifiable/factorisable . La création de nouvelle méthode (et de part se fait, de classe) permettra de rendre le code plus clair, plus simple et moins sujet à des oublies ou des bogues, car le copier coller (ou la duplication de code) c'est généralement le mal !

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
17
18
 
bool operator <(Position const & first, Position const & second)
{
    return first.x() < second.x() ||
              (first.x() == second.x() && first.y() < second.y() );
}
/* et, tant qu'à faire, te l'opérateur de comparaison == (pour d'autres
 * usages ultérieurs, très certainement ;)
 */
bool operator ==(Position const & first, Position const & second)
{
    return ( first.x() == second.x() && first.y() == second.y() );
}
 
bool operator !=(Position const & first, Position const & second)
{
    return  !(first == second);
}

Cela trouvera une meilleure place dans le fichier position.h/position.c car c'est lié à la classe Position.

En théorie, dans main.cpp il ne devrait y avoir que la fonction main (tout autre fonction devrait se trouver dans une classe). De plus, le main est généralement très court (initialisation + lancement de la boucle principale et hop, c'est fini ).

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
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
Deplacement computeDeplacement(size_t xOrigin, size_t yOrigin, SMap const & Map, TOUCHE touche)
{
     /* on va considérer que touche peut valoir "up","down","left" et "right"
      * sous la forme de valeurs énumérées ;)
       */
     switch(touche)
     {
         case haut:
         /* si on rencontre un "#" en haut, on est bloqué :aie */
         if (Map.tileAt(xOrigin, yOrigin-1) == '#')
         {
             return Deplacement(0,0);
         }
         else if(Map.tileAt(xOrigin, yOrigin-1) == 'O')
         {
             return Deplacement(0,0);
         }
         return Deplacement(0,-1);
 
		 case bas:
         /* si on rencontre un "#" en bas, on est bloqué :aie */
         if (Map.tileAt(xOrigin, yOrigin+1) == '#')
         {
             return Deplacement(0,0);
         }
         else if(Map.tileAt(xOrigin, yOrigin+1) == 'O')
         {
             return Deplacement(0,0);
         }
         return Deplacement(0,1);
 
         case gauche:
         /* si on rencontre un "#" à gauche, on est bloqué :aie */
         if (Map.tileAt(xOrigin-1, yOrigin) == '#')
         {
             return Deplacement(0,0);
         }
         else if(Map.tileAt(xOrigin-1, yOrigin) == 'O')
         {
             return Deplacement(0,0);
         }
         return Deplacement(-1,0);
 
		 case droite:
         /* si on rencontre un "#" à droite, on est bloqué :aie */
         if (Map.tileAt(xOrigin+1, yOrigin) == '#' )
         {
             return Deplacement(0,0);
         }
         else if(Map.tileAt(xOrigin+1, yOrigin) == 'O')
         {
             return Deplacement(0,0);
         }
         return Deplacement(1,0);
     }
     /* on ne devrait jamais arriver ici, mais ca évite un avertissement du
       * compilo, et puis, sait on jamais ;)
       * Pour toutes les autres touches, on ne bouge pas :D
       */
    return Deplacement(0,0);
}

Je crois (disons je dois en être même certain) qu'il est possible de remplacer les Deplacement(0,0) (ou autre valeurs) par des variables ayant des nombres plus clair (HAUT/BAS/GAUCHE/DROITE). Rendant le code beaucoup plus clair (je pense). Pour ce faire, on pourrait utiliser des macros, mais les macros sont sujets aux erreurs. On peut aussi imaginer utiliser des variables constantes (statiques ?) par exemple :

Code :

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

static const Deplacement HAUT = Deplacement(0,1);

qui serait membre public de la classe Deplacement.

Mais bon, je chipote peut être un peu de trop.

Ce que j'attends vraiment (car c'est un vrai point à améliorer) c'est la refactorisation du main, qui est trop massif et qui contient trop de duplication du code.

Voilà

[devdeb91]

bon bah alors je vais voir tout ça je te remercie d'avoir pris le temps de me souligner tout ces problemes.

pour doxygene a vrai dire je ne savais pas à quoi il servait et les fichiers qui sont à l'intérieur sont des anciens fichiers inutiles.

pour les classes sans majuscule je comprend cela provient du fait qu'il on été fait à la va vite.

pour la version Linux elle sera la aussi la j'ai mis sur Windows car j'utilisais sur Windows mais la version sera portable.

et pour le reste je vais essayer de palier à ça et faire des retours prochain.

pour tout autre commentaire ou autres je les attend tout est bon à prendre

[Médinoc]

Pour le franglais, je ne suis pas sûr: Est-ce qu'on utilise un autre mot que l'anglais "tiles" sur la scène française?
Il y a des anglicismes dont la traduction n'est pas évidente, ou "sonne" maladroit.

[JolyLoic]

Pour le franglais, je ne suis pas sûr: Est-ce qu'on utilise un autre mot que l'anglais "tiles" sur la scène française?

J'ai vu utiliser le terme tuiles. Mais ça ne m'avait pas paru très naturel.

[Luc Hermitte]

J'ai vu utiliser le terme tuiles. Mais ça ne m'avait pas paru très naturel.

On dit bien "tuile", et on écrit "tile" dans le code et les commentaires.

[Iradrille]

Pour le franglais, je ne suis pas sûr: Est-ce qu'on utilise un autre mot que l'anglais "tiles" sur la scène française?

Je dirais "case", mais c'est pas un mot pratique (inutilisable en minuscule).

[Kaamui]

Le problème c'est que tiles nous paraisse cohérent pour ce qu'on essaie d'exprimer, alors que c'est comme tuile en fin de compte, mais comme en anglais ça sonne mieux...

Je penses que le mot tile vient du besoin d'exprimer qu'il s'agit d'un fragment ayant des caractéristiques communes et étant rattaché à un organe plus grand (une map)...

Du coup pour un mot français intéressant, il faudrait plutôt chercher un mot qui traduit cette notion, comme la tuile le fait, mais qui sonne mieux que ce dernier (personnellement j'ai plein de mots mais aucun ne va)

Bon courage DevDeb91 pour ton jeu, et passe à la SFML ( 2° fois que je te le dis et même LittleWhite te l'as dit... )! Sinon une fois les défis techniques terminés, tu vas vite te décourager avec un programme console, et tu voudras tout refaire avant même d'avoir scripter le moindre morceau d'histoire (traduction pour toi et moi : avant même d'avoir atteint le village de Besaid), des mois vont s'écouler, et tu va voir que tout ce temps pourrait te servir à faire une version 2D vachement plus jolie, surtout avec la motivation que tu as, et la soif d'apprendre que tu as.