Discussion :

[Mikechaos]

J'ai tenté d'appliquer le pattern NVI.

Voici le code, bonne lecture! :

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
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
 
/****Character.h****/
#ifndef Character_H
#define Character_H
 
#include <ostream>
#include <string>
#include "Weapon.h"
//#include "Inventory.h"
class Character {
 
    public :
    Character() {}
    Character(std::string name, int life) : _name(name), _lifePoint(life) {
        _alive = true;
        std::cout << "Character -> \'" << _name << "\' created" << std::endl;
    }
 
         //Les wrapper publics permettant d'appeler
        //les fonctions virtuelles des classes filles
        void attack(Character* ennemy) {do_attack(ennemy);}
        void attacked(Character* attacker) {do_attacked(attacker);}
 
        Weapon* getWeapon() {return do_getWeapon();}
        void setWeapon(Weapon* weapon) {do_setWeapon(weapon);}
 
        //Comportement de base de la fonction defini par les deux
        //premieres lignes puis spécialisation appelé par do_dies()
        void dies() {
            std::cout << _name << " is dead" << std::endl;
            _alive = false;
            do_dies();
        }
 
        //Les getter/setter
        std::string getName() {return _name;}
 
        int getLifePoint() {return _lifePoint;}
        void setLifePoint(int lifePoint) {_lifePoint = lifePoint;}
 
        bool getAlive() {return _alive;}
        void setAlive(bool alive) {_alive = alive;}
 
    virtual ~Character() {
    std::cout << "Character ->\'" << _name << "\' Destroyed" << std::endl;}
 
    private :
 
        //Les méthodes abstraites sont la partie customisable de l'interface
        virtual void do_attack(Character *ennemy) = 0;
        virtual void do_attacked(Character* attacker) = 0;
 
        virtual Weapon* do_getWeapon() = 0;
        virtual void do_setWeapon(Weapon* weapon) = 0;
 
        virtual void do_dies() = 0;
 
        //Noter que meme les attributs propre a Character sont private
        //Si j'ai bien compris, cela renforce l'encapsulation et empeche
        //la classe fille de redefinir des comportements faisant partie
        //de la partie constante de l'interface.
        std::string _name;
        int _lifePoint;
        bool _alive;
};
 
class Warrior : public Character {
 
    public :
    Warrior(std::string name, int life) : Character(name, life) {
        _weapon = new Weapon();
        std::cout << "Character created" << std::endl;
    }
 
    //Si je decommente delete _weapon, j'obtiens un segfault parce que
    //l'arme Flame thrower est défruite deux fois.
    //Je ne comprends pas pourquoi
    virtual ~Warrior() {/*delete _weapon;*/}
 
    private :
        virtual void do_attack(Character *ennemy);
        virtual void do_attacked(Character *attacker);
 
        //Et on retrouve ici la specialistion de do_dies
        virtual void do_dies() {_weapon->setUsable(false);}
 
        virtual Weapon* do_getWeapon() {return _weapon;}
        virtual void do_setWeapon(Weapon* weapon) {_weapon = weapon;}
 
        Weapon *_weapon;
 
};
 
#endif

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
 
/****Character.cpp****/
#include <ostream>
#include "Character.h"
 
using namespace std;
 
void Warrior::do_attack(Character *ennemy)
{
    if (_weapon->isUsable())
        ennemy->attacked(this);
    else
        std::cout << getName() << " does not have a weapon" << endl;
}
 
void Warrior::do_attacked(Character *attacker)
{
    int lifePoint = getLifePoint();
    int damage = attacker->getWeapon()->getDamage();
    switch(damage > lifePoint){
        case true :
            damage = lifePoint;
            setLifePoint(0);
            dies();
            break;
        case false :
            setLifePoint(lifePoint - damage);
            break;
    }
    cout << getName() << " lost " << damage << " life point" << endl;
}

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
 
/****Weapon.h****/
#ifndef WEAPON_H
#define WEAPON_H
 
#include <iostream>
#include <string>
 
class Weapon {
    public :
        Weapon () : _usable(false) {}
        Weapon(std::string name, int damage) : _name(name), _damage(damage), _usable(true) {
        std::cout << "Weapon -> \'" << _name << "\' created" << std::endl;}
 
        ~Weapon(){
        std::cout << "Weapon -> \'" << _name <<"\' Destroyed" << std::endl;
        }
        int getDamage() {return _damage;}
        bool isUsable () {return _usable;}
        void setUsable(bool usable) {_usable = usable;}
 
    private:
        std::string _name;
        int _damage;
        bool _usable;
 
};
 
#endif

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
 
/*Et un petit main pour tester*/
 
#include <ostream>
 
#include "Character.h"
#include "Weapon.h"
//#include "Inventory.h"
//#include "Item.h"
 
using namespace std;
 
int main()
{
    cout << "\n\\=*=*=*=*=*=*=*=*=*Creation=*=*=*=*=*=*=*=*=*=/\n" << endl;
    Warrior a("Mike", 50);
    Warrior b("Gob", 20);
    Weapon sword("Sword", 25);
    Weapon flameThrower("Flame Thrower", 45);
 
    a.setWeapon(&sword);
    b.setWeapon(&flameThrower);
 
    cout << "\n\\=*=*=*=*=*=*=*=*=*Operation=*=*=*=*=*=*=*=*=*=/\n" << endl;
 
    a.attack(&b);
 
    cout << "\n\\=*=*=*=*=*=*=*=*Destructions=*=*=*=*=*=*=*=*=/\n" << endl;
    return 0;
}

Mes principales questions sont intégrées en commentaires dans le code.
(principalement, le destructeur de Warrior me pose problème...)

Sinon, j'attends bien évidemment des critiques (constructives!) sur mon implémentation de ce pattern et sur mon code ne général.
Ce code n'est qu'un code d'exercice, il se veut donc volontairement simplifier.

Merci d'avance à tous ceux qui vont prendre de leur précieux temps pour lire mon code et rédiger une réponse me permettant, ultimement, d'améliorer mes connaissances du C++.

[zul]

Le problème avec ta classe Warrior est ta méthode do_setWeapon. Dans le ctor, tu alloue un weapon. Dans do_setWeapon, tu écrase ce pointeur par un pointeur venant de l'extérieur (donc tu leak). Si tu appelle delete sur ce nouveau pointeur, 1/ c'est invalide parce que l'objet n'a pas été "crée" via un new 2/ l'objet est déja détruit en fin du scope main.

La solution la plus simple au vu du code est de stocker une arme dans Warrior, et pas un pointeur vers une arme (sauf si tu as réellement besoin de cette sémantique plus tard).

[3DArchi]

sauf si tu as réellement besoin de cette sémantique plus tard.

En ce cas, si tu as besoin d'un pointeur, sois pointeur intelligent

Pour le reste, il y a nul part de const. Je n'ai pas tout lu en détail, mais c'est certainement une étourderie.

Les get/set brisent l'encapsulation. Cela signifie que tu n'as probablement pas le bon niveau d'abstraction. Il devrait au pire être protected et non public.

Le passage des paramètres est souvent par pointeur. Personnellement, j'utilise un passage par pointeur lorsque la fonction peut recevoir NULL (c'est une précondition valide). Si ce n'est pas le cas (le paramètre ne peut jamais être nul), alors autant passer une référence (si besoin constante).

Puisque tu en es au pattern NVI, un peu de lecture : Les fonctions virtuelles en C++. Un chapitre y est consacré.

[Mikechaos]

Premièrement, merci beaucoup d'avoir pris le temps de regarder le code!

Zul -> Oui, j'ai besoin de cette architecture parce que Weapon est en fait un classe abstraite.

3dArchi ->
pour le leak ->Effectivement, j'aurais du y penser! Il est temps d'installer boost!

Pour lest const, je les rajoute tout de suite! Je n'étais pas concentré la-dessus. Merci!

Effectivement, je peux mettre les getter/setter en protected. Je n'y avais absolument pas pensé! C'est effectivement beaucoup plus logique! L'utilisateur n'a pas besoin d'avoir accès aux attributs de Character. Je n'avais même pas remarqué l'incohérence! Merci

<édit>Pour les pointeurs, confusions de ma part, à cause du polymorphisme (j'avais en tête que j'étais obligé d'utilisé des pointeurs)</édit>


Merci beaucoup pour ces conseils! Je vais de ce pas modifier mon code!
Sinon, est-ce qu'il y aurait autre chose, dans mon architecture autant que dans mon code, qui foire?

Merci d'avance à tous et bonne journée!

[Mikechaos]

Pour la méthode setWeapon, si je ne me trompe pas, je n'ai pas le choix d'utiliser un pointeur à la place d'une référence?
Puisque _weapon est un pointeur?

<reédit> Voir post plus bas </reédit>
<édit>
Bon, j'ai appliqué les modifications suggérées. Dernière étape, me familiariser avec Boost.Smartptr... Et je bloque!
Deux problèmes se posent.
Le premier, dans le constructeur de Warrior. Je veux créer une arme par défaut (les poings du guerrier), mais le compilo me crache dessus :

Ensuite, si je commente le constructeur, j'obtiens de nouveau un segfault avec ce code (destruction de 3 armes alors qu'il y en a seulement deux).

Merci d'avance, de nouveau!
</édit>

[poukill]

J'imagine que tu Warrior peut changer d'arme !
Du coup, c'est bien un pointeur que tu veux, une référence étant fixé lors de la construction de ta classe.

[3DArchi]

Je me demande s'il ne serait pas plutôt intéressant que le personnage aie une liste d'armes (par pointeur intelligent bien sur (les armes, pas la liste)) avec des méthodes 'prendre_arme' pour rajouter une arme à la liste et 'donner_arme' pour en sortir une de la liste.
Ma remarque pointeur/référence était plus axée sur le passage de paramètre en fonction.
Autre chose, weapon et character sont probablement des classes de base utilisées pour un héritage. Elles devraient être non copiable (cf F.A.Q. sur la sémantique de copie et sémantique d'entité).

[Mikechaos]

Poukill -> Merci

3DArchi -> J'ai édité mon précédent message.
<édit> J'ai réédité. Voir la fin de ce post! (Je commence à compliqué les choses à force d'édité </édit>

Sinon, la liste d'arme va se retrouver dans la classe Inventory, que je n'ai pas encore implémenté.

Pour la copie -> Effectivement! Je règle cela tout de suite.

<édit (je n'en fini plus avec les édits.. >
Bon, j'ai essayé avec la méthode reset de shared_ptr (après réflexion, je crois que c'est plutôt scoped_ptr qui devrait être utilisé). Je sens que j'y suis presque :

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
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
#ifndef Character_H
#define Character_H
 
#include <ostream>
#include <string>
#include "Weapon.h"
//#include "Inventory.h"
#include <boost/scoped_ptr.hpp>
class Character {
 
    public :
    Character() {}
    Character(std::string name, int life) : _name(name), _lifePoint(life) {
        _alive = true;
        std::cout << "Character -> \'"
        << _name << "\' created" << std::endl;
    }
 
         //Les wrapper publics permettant d'appeler
        //les fonctions virtuelles des classes filles
        void attack(Character& ennemy) {do_attack(ennemy);}
        void attacked(Character& attacker) {do_attacked(attacker);}
 
        Weapon& getWeapon() const {return do_getWeapon();}
        void setWeapon(Weapon* weapon) {do_setWeapon(weapon);}
 
        //Comportement de base de la fonction defini par les deux
        //premieres lignes puis spécialisation appelé par do_dies()
        void dies() {
            std::cout << _name << " is dead" << std::endl;
            _alive = false;
            do_dies();
        }
 
        //Les getter/setter
        protected :
        std::string getName() const {return _name;}
 
        int getLifePoint() const {return _lifePoint;}
        void setLifePoint(int lifePoint) {_lifePoint = lifePoint;}
 
        bool getAlive() const {return _alive;}
        void setAlive(bool alive) {_alive = alive;}
 
        ~Character() {
            std::cout << "Character ->\'" 
            << _name << "\' Destroyed" << std::endl;
        }
 
    private :
        //Classe non-copiable
        Character(Character const & copy);
        Character& operator=(Character const & copy);
 
        //Les méthodes abstraites sont la partie customisable de l'interface
        virtual void do_attack(Character& ennemy) = 0;
        virtual void do_attacked(Character& attacker) = 0;
 
        virtual Weapon& do_getWeapon() const = 0;
        virtual void do_setWeapon(Weapon* weapon) = 0;
 
        virtual void do_dies() = 0;
 
        //Attributs en private pour garder l'interface constante
        std::string _name;
        int _lifePoint;
        bool _alive;
};
 
class Warrior : public Character {
 
    public :
 
    Warrior(std::string name, int life) : Character(name, life) {
 
        //On cree une arme par defaut
        _weapon.reset(new Weapon("Hands", 5));
 
        std::cout << "Character created" << std::endl;
    }
    ~Warrior(){_weapon.reset();}
 
    private :
        virtual void do_attack(Character& ennemy);
        virtual void do_attacked(Character& attacker);
 
        //Et on retrouve ici la specialistion de do_dies
        virtual void do_dies() {_weapon->setUsable(false);}
 
        virtual Weapon& do_getWeapon() const {return *_weapon;}
 
        //J'ai donc utilise reset et cela semble fonctionner
        virtual void do_setWeapon(Weapon* weapon) {_weapon.reset(weapon);}
 
        boost::scoped_ptr<Weapon> _weapon;
 
};
 
#endif

Mais le segfault persiste et je ne vois vraiment pas d'où il peut provenir (il me dit encore que FlameThrower est détruite deux fois)
</édit>

Pour simplifier les choses, j'ai réécris une version épurée du code :

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
#include <string>
#include <boost/scoped_ptr.hpp>
#include <iostream>
 
class Character;
 
class Weapon {
    public :
        Weapon (){std::cout << "Weapon -> Created" << std::endl;}
 
        ~Weapon(){std::cout << "Weapon -> Destroyed" << std::endl;}
};
 
class Character {
 
    public :
    Character() {std::cout << "Character -> Created" << std::endl;}
 
    Weapon& getWeapon() const {return *_weapon;}
    void setWeapon(Weapon* weapon) {_weapon.reset(weapon);}
 
    ~Character() {std::cout << "Character -> Destroyed" << std::endl;}
 
    private :
        boost::scoped_ptr<Weapon> _weapon;
};
 
int main()
{
    Character a;
    Weapon b;
    a.setWeapon(&b);
    return 0;
}

<édit>Le problème a été cerné. Il vient de la méthode setWeapon.

Et voilà!

Code :

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

1
2
3
4
    void setWeapon(Weapon& weapon) {
        std::cout << "Changing Weapon" << std::endl;
        _weapon.reset(new Weapon(weapon));
    }

Merci à tous!