Discussion :

[devdeb91]

Ca pourrait être intéressant de virer les fichiers texte qui ne sont pas utilisés, et de réorganiser tout ça (les fichiers de donnée dans un répertoire data, par exemple ; les fichiers h et cpp dans un répertoire src ; ...) histoire de simplifier la compréhension du total.

Et puis, tu n'as pas besoin de mettre le répertoire obj

pour obj il se met automatiquement. pour le reste sa serais fait .

Profite en pour virer tout le code commenté. Il n'est pas utile, ralonge les fichiers, et rend la lecture du tout plus complexe.

sa aussi c'est fait mais hier j'ai joins la version avec les commentaires.

Maintenant, la critique du code :

* "using namespace std" ne doit jamais être utilisé (l'enlever permet de bien mettre en évidence l'utilisation de la librairie standard, vu que chaque utilisation est préfixée de std:: (5 caractères, c'est pas la mort)).

j'y penser aussi.

* main.h ? non, très mauvaise idée. Chaque .cpp doit inclure les headers dont il se sert, et ce de manière indépendante des headers qui sont inclus par le jeu des dépendances. Un court exemple :

Code :

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

1
2
3
4
5
6
7
8
9
 
// classe1.h 
include "classe2.h"
...
 
// classe3.cpp
#include "classe1.h"
...
classe2 un_objet;

Alors classe3.cpp doit include classe2.h, et ce même si ce fichier est automatiquement inclus du fait de l'inclusion de classe1.h. La raison derrière ce point : les dépendances peuvent changer au gré des refactoring. Un refactoring étant par essence local, il ne doit pas avoir d'impact global. Si classe1.h n'a plus besoin de classe2.h et ne l'inclus plus, alors classe3.cpp doit quand même compiler.

Bien évidemment, si classe3.cpp ne traite pas directement d'objets de type classe2, alors il n'inclus pas classe2.h (l'inclusion étant alors un détail d'implémentation de classe1.h).

En outre, le fait d'inclure dans un .cpp tous les headers dont il a besoin.

je vais voir cela car je comprend sans trop comprendre.

* il n'y a pas assez d'encapsulation dans ce code. En fait, il y a trop peu de classes - certaines fonctions mériterait d'être regroupées dans une classe (ne me demande pas lesquelles ; tu a fait le découpage en fichiers, donc tu sais grosso-modo ce qui peut être mis dans une classe, n'est-ce pas ? )

la seul class que je verais en plus serais sans doute MAP s'appuyant sur SMap

* certaines fonctions sont trop complexes. Des outils comme SourceMonitor (http://www.campwoodsw.com/) peuvent identifier les fonctions qui sont trop complexes (en utilisant une métrique connue sous le nom de "complexité cyclomatique de McCabe" (http://en.wikipedia.org/wiki/Cyclomatic_complexity)).

je vais tester cette outils.

* Une petite passe avec un analyseur statique (cppcheck, avec les options --enable=all --platform=win32A) me donne les petites erreurs suivantes :

Code :

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

1
2
3
4
5
6
7
 
[enemy.cpp:3]: (warning) Member variable 'Enemy::m_restonnerre' is not initialized in the constructor.
[enemy.cpp:3]: (warning) Member variable 'Enemy::m_resglace' is not initialized in the constructor.
[enemy.cpp:3]: (warning) Member variable 'Enemy::m_resfeu' is not initialized in the constructor.
[enemy.cpp:3]: (warning) Member variable 'Enemy::m_reseau' is not initialized in the constructor.
[enemy.cpp:3]: (warning) Member variable 'Enemy::m_ressacre' is not initialized in the constructor.
[smap.h:79]: (style) The function 'hasTeleportAt' is never used

(pas d'autres erreurs apparentes ; c'est plutôt un bon point !)

erreurs normal car c'est des attribut mis en place pour plus tard.

* Le constructeur de Personnage prend trop de paramètres (12). Idem pour Player (8). Idem pour SMonstre. L'idéal est un nombre réduit de paramètres (<=4).

je trouve aussi mais malheuresement j'ai besoin de tout ces parametre.

* on ne passe jamais les std::string par valeur ; on les passe par référence constante (const std::string&). Au niveau du code à écrire, ça ne change rien d'autre - et au niveau de l'exécution, on passe un pointeur plutôt qu'une copie d'un objet (qui impose l'utilisation du copy ctor).

* au niveau architecture logicielle, je ne sais pas si un player est un personnage. Un player a un avatar, et cet avatar est une créature, mais de la à faire le lien direct d'héritage entre player et personnage, c'est peut-être exagéré. Je mettrais plutôt les choses sous cette forme :

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
 
a player
  has an avatar
  has a position
  has an equipment
  evolves
  ...
 
an avatar
  has characteristics
  evolves (exemple: void player::levelup() { m_avatar->levelup(); })
  ...
 
an enemy
  has an avatar
  has a position
  has an equipment
  ...

player et enemy sont donc très proche - sauf que l’ennemi n'évolue pas (sauf cas très, très particulier ; auquel cas on peut rajouter une classe evolving_enemy...).

a voir car je ne m'y connais pas non plus des masses.

Ceci dit, c'est une opinion personnelle. En tout cas, SMonstre n'est certainement pas une structure.

Le prefixe S pour structure n'est vraiment pas utile.

Je reviendrais plus tard avec d'autres infos.

pourquoi elle ne serais pas une structure ?

ok bah j'attendrais ces nouvelles infos merci

[PilloBuenaGente]

la seul class que je verais en plus serais sans doute MAP s'appuyant sur SMap

Tu peux aller jusqu’à créer des classes pour "position" par exemple, elle aurait comme membres simplement "int X;" et "int Y;".

[PilloBuenaGente]

(Désolé, appuyé sur le bouton envoyé et c'est partit...)

a voir car je ne m'y connais pas non plus des masses.

C'est encore une histoire de polymorphisme, met le en relation avec le code que ta envoyé Koala01.

pourquoi elle ne serais pas une structure ?

Car ça doit être une classe ! Une structure est une classe aussi, mais sans "protection". Tout ces membres sont public. Et la encore pour pouvoir "polymorphiser !

[ternel]

Une structure est une donnée exposée.
Une classe est un ensemble cohérent ayant son comportement propre et défini.

Tu ne devrais pas avoir une structure si tu as des fonctions dédiées à sa manipulation.
Je passe personnellement à une classe aussitôt qu'une des conditions suivantes est remplie:

• la structure n'est pas exposée
• il existe plus de trois fonctions dédiées.
• il existe un libérateur/destructeur
• il existe une fonction de vérification
• il y a une relation entre les valeurs des champs

Les seules structures réellement utiles sont des utilitaires pour les retours de fonction et les rares API utilisant un argument void*.

[devdeb91]

(Désolé, appuyé sur le bouton envoyé et c'est partit...)
C'est encore une histoire de polymorphisme, met le en relation avec le code que ta envoyé Koala01.

Car ça doit être une classe ! Une structure est une classe aussi, mais sans "protection". Tout ces membres sont public. Et la encore pour pouvoir "polymorphiser !

mais la structure Smonstre et la pour appuyer la class enemy justement donc voila je vois pourquoi ce ne serais pas une structure

[devdeb91]

Tu peux aller jusqu’à créer des classes pour "position" par exemple, elle aurait comme membres simplement "int X;" et "int Y;".

la par contre je vois vraiment pas ou tu veut en venir

[germinolegrand]

Là encore c'est purement conventionnel et selon le goût, mais la distinction que je fais personnellement entre structure et classe est la suivante :
* une structure expose des données (que ce soit au travers de fonctions ne change rien)
* une classe expose des services

Rien qu'en faisant cette différence on donne au code une vraie sémantique.

la structure n'est pas exposée
il existe plus de trois fonctions dédiées.
il existe un libérateur/destructeur
il existe une fonction de vérification
il y a une relation entre les valeurs des champs

Pour une fois, ce n'est pas moi qu'on va traiter de matérialiste .
Dès l'instant où l'on parle de champs il s'agit d'exposer des données, et par conséquent il s'agit ni plus ni moins d'une structure.

[germinolegrand]

Pour la stucture position dont il parle :

Code :

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

1
2
3
4
5
 
struct Position
{
    int x, y;
};

On peut bien entendu templater, rajouter des operator à tout va, il n'en reste pas moins que la base sémantique est celle-ci.

[devdeb91]

Mais alors pourquoi faire une structure position alors que on a une class position deja existante

[germinolegrand]

(je m'excuse je n'ai pas lu le code)
Si elle existe déjà inutile de la créer

[devdeb91]

Pas de soucis. donc pillobuenagente il faudra que tu m'explique ce que tu voulais dire par la

[Emmanuel Deloget]

mais la structure Smonstre et la pour appuyer la class enemy justement donc voila je vois pourquoi ce ne serais pas une structure

Les structures doivent être réservées aux objets n'ayant ni invariant (=ensemble de valeurs valides), ni comportement (=méthode) - uniquement des propriétés. Dès que tu as des méthodes et des propriétés, et que les méthodes agissent sur les propriétés, alors il est plus que préférable de faire une classe. L'idée est que les propriétés sont des détails d'implémentation qui te permettent de fournir les services dont tu as besoin. Si tu permet à un utilisateur (au sens large) de modifier les propriétés sans passer par l'interface de la classe, alors tu ne peux plus contrôler la validité de l'objet (son invariant, donc), et tu ne peux pas être sûr que les services que tu rends se comportement correctement.

Un exemple un peu limite :

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
 
class logarithme
{
public:
  float m_base;
 
  logarithme(float base) : m_base(base)
  {
    if (m_base <= 0)
       throw std::domain_error("log base shall be positive"); 
  }
 
  float get(float x)
  {
     if (x > 0)
        return ::logf(x) / ::logf(m_base);
     throw std::domain_error("cannot compute log_a(x) for x < 0");
  }
};

Maintenant, le fait que m_base soit public permet d'écrire :

Code :

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

1
2
3
4
 
logarithme my_log(100);
m_log.m_base = -1;
float v = my_log.get(100);

On ne récupère aucune exception, sauf que le résultat obtenu par l'appel à get() ne va pas être valide du tout. Dans ce cas, l'invariant de la classe (qui est : "(m_base > 0) est vrai") n'est pas respecté.

En mettant m_base en privé, on s'empêche ce genre de problème.

Bien évidemment, je ne fais qu'énoncé quelque chose de très, très simple, et tu le sais déjà certainement. Sauf que c'est la même erreur que tu fais sur SMonstre, SWrap, SMap (si pour une raison ou une autre tu modifies la taille de la map sans modifier la map elle même, tu va rencontrer des problèmes...), etc.

Du point de vue de l'architecte logiciel, chaque membre public d'une classe introduit une dépendance - que cette dépendance soit effective (c'est à dire que le membre est utilisé) ou non. La raison est simple : si une classe A utilise une classe B, alors A dépends de toutes les interfaces de B.

Si les dépendances se multiplient, alors le couplage entre les classes augmente. Hors, le couplage n'est jamais bon : il rends les softs plus difficilement maintenables en plus de les rendre fragiles (une modification quelque part peut avoir un impact insoupçonné autre part), visqueux (pour éviter des impacts insoupçonnés, on effectue des modifications qui ne sont pas correctes d'un point de vue architectural, ce qui rends le soft encore plus fragile), etc.

L'idée est donc (toujours du point de vue de l'architecte) de diminuer au maximum la surface d'exposition des différentes classes. En termes clair : on ne rend public que ce qui doit l'être.

Ca a un autre avantage au niveau du debuggage. Dès lors que des problèmes se posent, on peut identifier plus rapidement les classes contenant des valeurs mauvaises (parce qu'on connait leur invariant) et le fait que seul un petit nombre de fonctions permet d'influer sur l'état de la classe, on va déterminer la fonction coupable plus rapidement.

Bref, ça fait pas mal de raisons de préférer les classes aux structures dès lors qu'on fait quelque chose qui n'est pas trivial (selon le design, un point3d peut être considéré comme un objet trivial (membres x, y, z publics dans un structure) ou non (méthodes permettant le déplacement, le dédoublementl, la projection sur un axe...)).

[r0d]

"using namespace std" ne doit jamais être utilisé

Je ne suis pas trop d'accord sur ce point. En fait, c'est le "jamais" que je n'aime pas trop.
En général, j'opte pour une charte de nommage "à la java": MaClasse::MaFonction() et MaFonctionLibre(). Et justement, une des raisons pour lesquelles j'aime bien la majuscule sur la fonction c'est que c'est une façon de différentier mes fonctions à celles des libs que j'utilise. Je trouve qu'un code est plus lisible sans les "std::" et "boost::" partout.
En revanche, il ne faut pas inclure namespace std dans un .h, j'ai déjà eu des soucis avec ça.

[ternel]

De mon coté, j'ai plus tendance à faire un using namespace (généralement std) et des alis (using bfs = boost::filesystemCela dit, je place la grande majorité de mes propres codes dans des namespace, ce qui réduit ce risque

[germinolegrand]

Moi je suis d'accord avec le jamais de using namespace. Quand on voit un std:: un sf:: ou un eode:: (namespace perso) on voit immédiatement qu'on utilise des ressources d'un module extérieur, ce qui aide à démêler les appels internes et externes.

[PilloBuenaGente]

"il faudra que tu m'explique ce que tu voulais dire par la"

Exemple de la classe position. Ce n'est pas que tu dois l'utiliser, c'est à titre d'exemple :

fichier position.h

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
 
#ifndef POSITION_H
#define POSITION_H
 
class Postition {
public:
    Postition( const int x, const int y );
    const int PosX() const { return m_x; }
    const int PosY() const { return m_y; }
private:
    const int m_x;
    const int m_y;
};
 
#endif

fichier position.cpp

Code :

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

1
2
3
4
5
6
 
#include "Position.h"
 
Position::Position( const int x, const int y ) : m_x( x ), m_y( y ) {
 
}

Tu vois bien que l'on peut créer une classe pour tout ! Après le compliqué est le faire judicieusement.

[Médinoc]

Il y a un inconvénient à déclarer les variables membres elles-même const: Tu ne peux pas écraser complètement l'objet par un autre:

Code C :

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

1
2
Position a = Position(1, 1);
a = Position(2, 42); //Nécessite que les variables soient non-const.

Ce problème est résolu en C#, mais en C++ il faut le contourner en déclarant les variables non-const. Les accesseurs ne changent pas, par contre.

[PilloBuenaGente]

Autant pour moi
Également si on ne met que des fonctions constantes, les membres ne pourront être affecté de l’extérieur, ce qui les rends les membres plus ou moins constant non ?! Et la classe reste copiable.

[gbdivers]

Ce problème est résolu en C#, mais en C++ il faut le contourner en déclarant les variables non-const. Les accesseurs ne changent pas, par contre.

Cela m'étonne comme remarque. C'est pas un problème, c'est un choix du concepteur de la classe qui dit "je veux pas qu'on puisse modifier l'objet". C'est l'inverse (modifier un objet constant) qui m'étonne et qui me semble poser problème
Ou alors, j'ai pas compris le besoin

[PilloBuenaGente]

Pour l'utilisation des vecteurs par exemple, ou alors il faut alors réserver l'espace. (Perd du dynamisme)