Question conception contructeur

Version imprimable

Bonjour à tous, j'aimerais juste savoir si ce code vous paraît correct même si il fonctionne :?

Code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
 
Spell::Spell( std::string const &name, std::vector<DmgRoll> const &dmg, std::vector<DmgRoll> const &dmgCrit ) : spell_name{name}, spell_dmg{dmg}, spell_dmg_crit{dmgCrit}, spell_noCrit{false}
{
    if(spell_dmg.empty())
        e("Spell " + spell_name + " vide");
}
 
Spell::Spell( std::string const &name, std::vector<DmgRoll> const &dmg ) : spell_name{name}, spell_dmg{dmg}, spell_noCrit{true}
{
    if(spell_dmg.empty())
        e("Spell " + spell_name + " vide");
}
 
Spell::Spell( std::string const &name, DmgRoll const &dmg, DmgRoll const &dmgCrit ) : spell_name{name}, spell_dmg{dmg}, spell_dmg_crit{dmgCrit}, spell_noCrit{false}
{
    if(spell_dmg.empty())
        e("Spell " + spell_name + " vide");
}
 
Spell::Spell( std::string const &name, DmgRoll const &dmg ) : spell_name{name}, spell_dmg{dmg}, spell_noCrit{true}
{
    if(spell_dmg.empty())
        e("Spell " + spell_name + " vide");
}

Le fait qu'un vecteur en l’occurrence le spell_dm_crit ne soit pas initialisé est grave ? ( même si il y a un test pour voir si il est valide ailleurs dans les codes qui l'utilise ).
En faite je trouve ce code pas très sécurisé et moche donc peut-être que vous pouvez me dire ce qui ne vas pas ?

Merci ;)

Bonjour,

Si un constructeur ne fait pas référence à un objet de la classe, l'objet est initialisé par son constructeur par défaut (sauf pour les scalaires c-a-d les pointeurs, les enum et les arithmétiques). Donc le spell_dm_crit est bien initialisé comme un vecteur vide.
Ces constructeurs me paraissent cleans.

Mes interrogations :
n'y a-t-il pas trop de constructeurs ?
les vecteurs qui sont passés au constructeurs ont été créé avant, et sont copiés lors de la construction. On en a donc 2 de chaque à cet instant. Cela est-il vraiment nécessaire ?
les paramètres sont passés par référence constantes, c'est une excellente manière de faire. Cependant quand par exemple le paramètre est copié, un passage par valeur est souvent plus optimal.
Code:

1 2 3 4 5 Spell::Spell( std::string name , std::vector<DmgRoll> dmg ) : spell_name{std::move(name)}, spell_dmg{std::move(dmg)}, spell_noCrit{true} { ... } Spell spl{ "nom"s , std::vector<DmgRoll>{} }; // va construire directement ces paramètres dans l'objet spl

31/07/2016, 00h00
Disixlis

Alors, non il n'y pas trop de constructeur car certains sort on 1 ou plusieurs jets et je ne vais pas créer de vecteur contenant 1 seul jet x) donc pour ça, c'est normal :)
"les vecteurs qui sont passés au constructeurs ont été créé avant, et sont copiés lors de la construction" je comprend pas vraiment, j'ai pas le choix je suis obligé de faire une copie non ? du moins si je créer le vecteur dans le constructeur quand je crée l'objet, ça fait de toute manière une copie non ? ( #l'oublie )
Et donc du coup ma référence sert pas à grand chose vu que ce sera toujours copié :/... . . je crois que je suis embrouillé xD je vais relire les cours
31/07/2016, 01h53
SkyZoThreaD

Je suis d’accord avec dalfab et j'ajouterai: Est-il vraiment nécessaire de stoker les valeurs des vecteurs dans la classe ou est-ce que la ref ne suffirait pas? Ça éviterait une copie.

Citation:

Envoyé par dalfab

les paramètres sont passés par référence constantes, c'est une excellente manière de faire. Cependant quand par exemple le paramètre est copié, un passage par valeur est souvent plus optimal.[/LIST]

Code:

1
2
3
4
5
Spell::Spell( std::string name , std::vector<DmgRoll> dmg )
 : spell_name{std::move(name)}, spell_dmg{std::move(dmg)}, spell_noCrit{true}
{ ... }
 
   Spell spl{ "nom"s , std::vector<DmgRoll>{} };   // va construire directement ces paramètres dans l'objet spl

Parfois plus optimal, mais d'autres fois moins.
Je viens de tester ce code sur le site Coliru :

Code:

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
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
#include <iostream>
#include <string>
 
struct ObjetSimple
{
    std::string m_data;
 
    ObjetSimple(const std::string& data) : m_data(data)
    {
        std::cout << "ObjetSimple  : constructeur\n";
    }
    ObjetSimple(const ObjetSimple& other) : m_data(other.m_data)
    {
        std::cout << "ObjetSimple  : constructeur de recopie\n";
    }
    ObjetSimple(ObjetSimple&& other) : m_data(std::move(other.m_data))
    {
        std::cout << "ObjetSimple  : constructeur de mouvement\n";
    }
    ObjetSimple& operator=(const ObjetSimple& other)
    {
        m_data = other.m_data;
        std::cout << "ObjetSimple  : affectation de recopie\n";
        return *this;
    }
    ObjetSimple& operator=(ObjetSimple&& other)
    {
        m_data = std::move(other.m_data);
        std::cout << "ObjetSimple  : affectation de mouvement\n";
        return *this;
    }
    ~ObjetSimple()
    {
        std::cout << "ObjetSimple  : destructeur\n";
    }
};
 
struct ParReference
{
    ObjetSimple m_objet;
 
    ParReference(const ObjetSimple& objet) : m_objet(objet) // PAR REFERENCE CONSTANTE
    {
        std::cout << "ParReference : constructeur\n";
    }
    ParReference(const ParReference& other) : m_objet(other.m_objet)
    {
        std::cout << "ParReference : constructeur de recopie\n";
    }
    ParReference(ParReference&& other) : m_objet(std::move(other.m_objet))
    {
        std::cout << "ParReference : constructeur de mouvement\n";
    }
    ParReference& operator=(const ParReference& other)
    {
        m_objet = other.m_objet;
        std::cout << "ParReference : affectation de recopie\n";
        return *this;
    }
    ParReference& operator=(ParReference&& other)
    {
        m_objet = std::move(other.m_objet);
        std::cout << "ParReference : affectation de mouvement\n";
        return *this;
    }
    ~ParReference()
    {
        std::cout << "ParReference : destructeur\n";
    }
};
 
struct ParValeur
{
    ObjetSimple m_objet;
 
    ParValeur(ObjetSimple objet) : m_objet(std::move(objet)) // PAR VALEUR
    {
        std::cout << "ParValeur    : constructeur\n";
    }
    ParValeur(const ParValeur& other) : m_objet(other.m_objet)
    {
        std::cout << "ParValeur    : constructeur de recopie\n";
    }
    ParValeur(ParValeur&& other) : m_objet(std::move(other.m_objet))
    {
        std::cout << "ParValeur    : constructeur de mouvement\n";
    }
    ParValeur& operator=(const ParValeur& other)
    {
        m_objet = other.m_objet;
        std::cout << "ParValeur    : affectation de recopie\n";
        return *this;
    }
    ParValeur& operator=(ParValeur&& other)
    {
        m_objet = std::move(other.m_objet);
        std::cout << "ParValeur    : affectation de mouvement\n";
        return *this;
    }
    ~ParValeur()
    {
        std::cout << "ParValeur    : destructeur\n";
    }
};
 
static ParReference fooParReference() {ObjetSimple obj("toto"); return ParReference(obj);}
static ParValeur    fooParValeur()    {ObjetSimple obj("toto"); return ParValeur   (obj);}
static ParReference barParReference() {return ParReference(ObjetSimple("toto"));}
static ParValeur    barParValeur()    {return ParValeur   (ObjetSimple("toto"));}
 
int main()
{
    std::cout << "Version de g++ : " __VERSION__ << "\n\n";
    std::cout << "Avec les fonctions foo, c'est mieux par référence :\n\n";
    fooParReference();
    std::cout << "\n";
    fooParValeur();
    std::cout << "\n";
    std::cout << "Avec les fonctions bar, c'est mieux par valeur :\n\n";
    barParReference();
    std::cout << "\n";
    barParValeur();
	return 0;
}

Options de compilation :
g++ -std=c++14 -O2 -Wall -pedantic -pthread main.cpp && ./a.out

Sortie :

Code:

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
Version de g++ : 6.1.0
 
Avec les fonctions foo, c'est mieux par référence :
 
ObjetSimple  : constructeur
ObjetSimple  : constructeur de recopie
ParReference : constructeur
ObjetSimple  : destructeur
ParReference : destructeur
ObjetSimple  : destructeur
 
ObjetSimple  : constructeur
ObjetSimple  : constructeur de recopie
ObjetSimple  : constructeur de mouvement
ParValeur    : constructeur
ObjetSimple  : destructeur
ObjetSimple  : destructeur
ParValeur    : destructeur
ObjetSimple  : destructeur
 
Avec les fonctions bar, c'est mieux par valeur :
 
ObjetSimple  : constructeur
ObjetSimple  : constructeur de recopie
ParReference : constructeur
ObjetSimple  : destructeur
ParReference : destructeur
ObjetSimple  : destructeur
 
ObjetSimple  : constructeur
ObjetSimple  : constructeur de mouvement
ParValeur    : constructeur
ObjetSimple  : destructeur
ParValeur    : destructeur
ObjetSimple  : destructeur

Citation:

Envoyé par Disixlis

"les vecteurs qui sont passés au constructeurs ont été créé avant, et sont copiés lors de la construction" je comprend pas vraiment, j'ai pas le choix je suis obligé de faire une copie non ?

Pour éviter une copie, il y a deux solutions simples :
1) La classe Spell contient des pointeurs ou des références vers des std::vector<DmgRoll> au lieu de contenir directement les std::vector<DmgRoll>. Cela permet d'éviter de copier les objets std::vector<DmgRoll>. Il faudra cependant faire attention à ce que l'objet Spell soit détruit avant les objets std::vector<DmgRoll>. Sinon, l'objet Spell aura des pointeurs ou des références vers des std::vector<DmgRoll> qui n'existent plus.
2) La classe Spell contient des std::vector<DmgRoll> initialement vides. Tu les remplis ensuite progressivement via des fonctions de Spell qui appellent std::vector<DmgRoll>::push_back(). Cette solution a un sens si les éléments ajoutés au fur et à mesure ne sont pas déjà tous dans un std::vector<DmgRoll> avant la création de l'objet Spell.

31/07/2016, 12h55
Disixlis

Donc pour résumé, je veux que mon jeu puisse évolué sans devoir (ou du moins rarement) recompiler mon projet. Ce qui veux dire que tout ce qui est DmgRoll est lu dans un fichier, copié dans un spell puis un vecteur de spell est gardé dans le main pour que toutes les classes nécessitant un sort puisse y avoir accès sans copies. Donc du coup comme de toute manière DmgRoll est copié dans un Spell puis détruit, mes références ne servent à rien, donc autant copié ? ou je devrai faire autrement ?
01/08/2016, 14h34
Disixlis

Finalement je vais abandonné les références, par contre je vais utiliser std::move qui est bien mieux que la copie :)
01/08/2016, 15h18
dalfab

Dans ce cas ça n'est pas bien important, nous avons proposé d'autres méthodes mais ta solution aussi est acceptable.
Ce qui est important c'est de comprendre les avantages/inconvénients des diverses possibilités, et il existera d'autres cas où il faudra faire le bon dès la conception.

Citation:

Envoyé par Pyramidev

Parfois plus optimal, mais d'autres fois moins.
Je viens de tester ce code sur le site Coliru :

Code:

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
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
#include <iostream>
#include <string>
 
struct ObjetSimple
{
    std::string m_data;
 
    ObjetSimple(const std::string& data) : m_data(data)
    {
        std::cout << "ObjetSimple  : constructeur\n";
    }
    ObjetSimple(const ObjetSimple& other) : m_data(other.m_data)
    {
        std::cout << "ObjetSimple  : constructeur de recopie\n";
    }
    ObjetSimple(ObjetSimple&& other) : m_data(std::move(other.m_data))
    {
        std::cout << "ObjetSimple  : constructeur de mouvement\n";
    }
    ObjetSimple& operator=(const ObjetSimple& other)
    {
        m_data = other.m_data;
        std::cout << "ObjetSimple  : affectation de recopie\n";
        return *this;
    }
    ObjetSimple& operator=(ObjetSimple&& other)
    {
        m_data = std::move(other.m_data);
        std::cout << "ObjetSimple  : affectation de mouvement\n";
        return *this;
    }
    ~ObjetSimple()
    {
        std::cout << "ObjetSimple  : destructeur\n";
    }
};
 
struct ParReference
{
    ObjetSimple m_objet;
 
    ParReference(const ObjetSimple& objet) : m_objet(objet) // PAR REFERENCE CONSTANTE
    {
        std::cout << "ParReference : constructeur\n";
    }
    ParReference(const ParReference& other) : m_objet(other.m_objet)
    {
        std::cout << "ParReference : constructeur de recopie\n";
    }
    ParReference(ParReference&& other) : m_objet(std::move(other.m_objet))
    {
        std::cout << "ParReference : constructeur de mouvement\n";
    }
    ParReference& operator=(const ParReference& other)
    {
        m_objet = other.m_objet;
        std::cout << "ParReference : affectation de recopie\n";
        return *this;
    }
    ParReference& operator=(ParReference&& other)
    {
        m_objet = std::move(other.m_objet);
        std::cout << "ParReference : affectation de mouvement\n";
        return *this;
    }
    ~ParReference()
    {
        std::cout << "ParReference : destructeur\n";
    }
};
 
struct ParValeur
{
    ObjetSimple m_objet;
 
    ParValeur(ObjetSimple objet) : m_objet(std::move(objet)) // PAR VALEUR
    {
        std::cout << "ParValeur    : constructeur\n";
    }
    ParValeur(const ParValeur& other) : m_objet(other.m_objet)
    {
        std::cout << "ParValeur    : constructeur de recopie\n";
    }
    ParValeur(ParValeur&& other) : m_objet(std::move(other.m_objet))
    {
        std::cout << "ParValeur    : constructeur de mouvement\n";
    }
    ParValeur& operator=(const ParValeur& other)
    {
        m_objet = other.m_objet;
        std::cout << "ParValeur    : affectation de recopie\n";
        return *this;
    }
    ParValeur& operator=(ParValeur&& other)
    {
        m_objet = std::move(other.m_objet);
        std::cout << "ParValeur    : affectation de mouvement\n";
        return *this;
    }
    ~ParValeur()
    {
        std::cout << "ParValeur    : destructeur\n";
    }
};
 
static ParReference fooParReference() {ObjetSimple obj("toto"); return ParReference(obj);}
static ParValeur    fooParValeur()    {ObjetSimple obj("toto"); return ParValeur   (obj);}
static ParReference barParReference() {return ParReference(ObjetSimple("toto"));}
static ParValeur    barParValeur()    {return ParValeur   (ObjetSimple("toto"));}
 
int main()
{
    std::cout << "Version de g++ : " __VERSION__ << "\n\n";
    std::cout << "Avec les fonctions foo, c'est mieux par référence :\n\n";
    fooParReference();
    std::cout << "\n";
    fooParValeur();
    std::cout << "\n";
    std::cout << "Avec les fonctions bar, c'est mieux par valeur :\n\n";
    barParReference();
    std::cout << "\n";
    barParValeur();
	return 0;
}

Options de compilation :
g++ -std=c++14 -O2 -Wall -pedantic -pthread main.cpp && ./a.out

Sortie :

Code:

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
Version de g++ : 6.1.0
 
Avec les fonctions foo, c'est mieux par référence :
 
ObjetSimple  : constructeur
ObjetSimple  : constructeur de recopie
ParReference : constructeur
ObjetSimple  : destructeur
ParReference : destructeur
ObjetSimple  : destructeur
 
ObjetSimple  : constructeur
ObjetSimple  : constructeur de recopie
ObjetSimple  : constructeur de mouvement
ParValeur    : constructeur
ObjetSimple  : destructeur
ObjetSimple  : destructeur
ParValeur    : destructeur
ObjetSimple  : destructeur
 
Avec les fonctions bar, c'est mieux par valeur :
 
ObjetSimple  : constructeur
ObjetSimple  : constructeur de recopie
ParReference : constructeur
ObjetSimple  : destructeur
ParReference : destructeur
ObjetSimple  : destructeur
 
ObjetSimple  : constructeur
ObjetSimple  : constructeur de mouvement
ParValeur    : constructeur
ObjetSimple  : destructeur
ParValeur    : destructeur
ObjetSimple  : destructeur

Personnellement je n'aime pas particulièrement les passages par valeur, soit je fais une surchage en proposant un passage par référence constante et un autre par rvalue reference, soit si la situation s'y prête bien, je préfère même un passage par universal reference. Avec ton code ça devient :

Code:

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
struct ParUniversalReference
{
    ObjetSimple m_objet;
 
    template<typename Objet>
    ParUniversalReference(Objet&& objet) : m_objet(std::forward<Objet>(objet)) 
    {
        static_assert(std::is_same<typename std::remove_reference<Objet>::type, ObjetSimple>::value, "pas un objet simple");
        std::cout << "ParUniversalReference : constructeur\n";
    }
    ParUniversalReference(const ParUniversalReference& other) : m_objet(other.m_objet)
    {
        std::cout << "ParUniversalReference : constructeur de recopie\n";
    }
    ParUniversalReference(ParUniversalReference&& other) : m_objet(std::move(other.m_objet))
    {
        std::cout << "ParUniversalReference : constructeur de mouvement\n";
    }
    ParUniversalReference& operator=(const ParUniversalReference& other)
    {
        m_objet = other.m_objet;
        std::cout << "ParUniversalReference : affectation de recopie\n";
        return *this;
    }
    ParUniversalReference& operator=(ParUniversalReference&& other)
    {
        m_objet = std::move(other.m_objet);
        std::cout << "ParUniversalReference : affectation de mouvement\n";
        return *this;
    }
    ~ParUniversalReference()
    {
        std::cout << "ParUniversalReference : destructeur\n";
    }
};
 
static ParUniversalReference    fuuParUniversalReference()    {ObjetSimple obj("toto"); return ParUniversalReference(obj);}
static ParUniversalReference    booParUniversalReference()    {return ParUniversalReference(ObjetSimple("toto"));}
 
int main()
{
    std::cout << std::endl;
    std::cout << "Avec les fonctions fuu, c'est mieux par universal reference : \n\n";
    fuuParUniversalReference();
    std::cout << std::endl;
    std::cout << "Avec les fonctions boo, c'est mieux par universal reference : \n\n";
    booParUniversalReference();
    return 0;
}

Code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
 
Avec les fonctions fuu, c'est mieux par universal reference : 
 
ObjetSimple  			: constructeur
ObjetSimple  			: constructeur de recopie
ParUniversalReference 	        : constructeur
ObjetSimple  			: destructeur
ParUniversalReference 	        : destructeur
ObjetSimple  			: destructeur
 
 
Avec les fonctions boo, c'est mieux par universal reference : 
 
ObjetSimple  			: constructeur
ObjetSimple  			: constructeur de mouvement
ParUniversalReference 	        : constructeur
ObjetSimple  			: destructeur
ParUniversalReference 	        : destructeur
ObjetSimple  			: destructeur

Citation:
Envoyé par Danny-k
soit si la situation s'y prête bien, je préfère même un passage par universal reference. Avec ton code ça devient :
Code:

1 2 3 4 5 6 7 8 9 10 struct ParUniversalReference { ObjetSimple m_objet; template<typename Objet> ParUniversalReference(Objet&& objet) : m_objet(std::forward<Objet>(objet)) { static_assert(std::is_same<typename std::remove_reference<Objet>::type, ObjetSimple>::value, "pas un objet simple"); std::cout << "ParUniversalReference : constructeur\n"; }
J'aime bien le principe. Quand on a un constructeur dont N paramètres peuvent chacun peut être passés soit par référence constante, soit par rvalue référence, cela évite de coder 2 puissance N constructeurs.

Tu as juste oublié un std::remove_const :

Code:

static_assert(std::is_same<typename std::remove_const<typename std::remove_reference<Objet>::type>::type, ObjetSimple>::value, "pas un objet simple");

Autrement, quand on passe en paramètre une lvalue de type const ObjetSimple, on a une erreur de compilation :
Code:

1 2 const ObjetSimple obj("toto"); ParUniversalReference paf(obj); // erreur de compilation si pas de std::remove_const