Discussion :

[f0x888]

Bonjour tout le monde,

J'ai un petit soucis sur mon code. J'ai créé un classe Personnage dans laquelle j'ai implémenté deux constructeurs :
- 1 avec des valeurs par défauts,
- l'autre avec des valeurs définies par l'utilisateur.

Jusqu'ici, je pense que tout va bien. Pour comprendre voici mon code concernant la classe Personnage :

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
 
#include "Personnage.h"
#include <iostream>
#include <string.h>
#include <istream>
#include <vector>
 
using namespace std;
 
Personnage::Personnage(vector<string> nom): m_arme(0), m_armeSac(0), m_vie(100), m_mana(100)
{
    m_arme = new Arme();
    m_armeSac = new Arme();
 
}
 
Personnage::Personnage(vector<string> nom, vector<string> nomArme, vector<int> degatsArme) : m_arme(0), m_armeSac(0), m_vie(100), m_mana(100)
{
    m_arme = new Arme(nomArme, degatsArme);
    m_armeSac = new Arme(nomArme, degatsArme);
}

Dans mon main(), je demande à l'utilisateur de créer le nom de ses perso, de choisir des armes qu'il affecte à chaque perso. Pour cela, j'ai opté pour la création de tableaux en utilisant vector.
J'ai donc :
- un vector<string> avec le nom des Perso entrés par le User,
- un vector<string> avec les armes affectées par le User,
- un vector<int> pour les dégâts correspondant aux armes affectées.

L'idée est de rassembler tous ces éléments dans un objet qui, lui-même, serait un tableau. OUF, j'ai réussi à tout expliquer.

Voici l'ensemble de mon main() =

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
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
int main()
{
 
    int nbrePersoACreer=0;
    int validChoix;
    int num=0;
 
    vector<string> nomPerso;
    vector<string> armePerso;
    vector<int> degatsArme;
 
    // Demander de créer des personnages
 
    while(nbrePersoACreer<1 || nbrePersoACreer>4){
       cout << "Créer vos personnages" << endl << endl;
       cout << "Pour le moment, 4 combattants maximum !!!" << endl;
       cout << "Combien de personnages voulez-vous créer : ";
       cin >> nbrePersoACreer;
    }
 
    do{
    num=0;
    nomPerso.clear(); // efface l'ensemble des données du tableau.
        for (int i=1 ; i <= nbrePersoACreer ; i++){
            num++;
            string nom;
            cout << "Entrez le nom du personnage n° " << num << " : ";
            cin >> nom;
            nomPerso.push_back(nom);
        }
 
        cout << "" << endl;
 
        cout << "Voici les " << nbrePersoACreer << " noms des personnages choisis : " << endl;
        num=0;
 
        for (int i=0; i < nbrePersoACreer ; i++){
            num++;
            cout << "Personnage " << num << " : " << nomPerso[i] << endl;
        }
 
        cout << "" << endl;
        cout << "Voulez-vous conserver les noms de personnages ?" << endl;
        cout << "Choix 1 : c'est ok," << endl;
        cout << "Choix 2 : Finalement, les noms sont merdiques, je change !" << endl;
        cout << "Vas-y mec, choisi : ";
        cin >> validChoix;
        cout << "" << endl;
 
     }while(validChoix != 1 && validChoix == 2);
 
         cout << "" << endl;
 
 
   // Choisir une arme pour chaque personnage
 
    cout << "Choississez une arme pour chaque personnage : " << endl;
    cout << "choix 1 : épée (degats 10)" << endl;
    cout << "choix 2 : sabre (degats 10)" << endl;
    cout << "" << endl;
    num=0;
 
    do{
    num=0;
    armePerso.clear(); // efface l'ensemble des données du tableau.
    degatsArme.clear(); // efface l'ensemble des données du tableau.
 
        for (int i=0; i < nbrePersoACreer ; i++){
            num++;
            int arme;
            cout << "Personnage " << nomPerso[i] << " : ";
            cin >> arme;
                if (arme = 1){
                    armePerso.push_back("épée");
                    degatsArme.push_back(10);
                }
                if (arme = 2){
                    armePerso.push_back("sabre");
                    degatsArme.push_back(10);
                }
        }
 
        cout << "" << endl;
 
        cout << "Voici les armes affectées pour les " << nbrePersoACreer << " personnages : " << endl;
        num=0;
 
        for (int i=0; i < nbrePersoACreer ; i++){
            num++;
            cout << "Personnage " << nomPerso[i] << " : " << armePerso[i] << " (degats " << degatsArme[i] << ")" << endl;
        }
 
        cout << "" << endl;
        cout << "Voulez-vous valider l'affectation des armes ?" << endl;
        cout << "Choix 1 : c'est ok," << endl;
        cout << "Choix 2 : Finalement, je change !" << endl;
        cout << "Vas-y mec, choisi : ";
        cin >> validChoix;
        cout << "" << endl;
 
     }while(validChoix != 1 && validChoix == 2);
 
     /*créer l'objet Personnage en utilisant les valeurs tapées par l'utilisateur
     et stockées dans mes différents tableaux.*/
 
    // Début de piste :
 
    Personnage *tabPerso[nbrePersoACreer];
 
    tabPerso[nbrePersoACreer] = new Personnage(nomPerso, armePerso, degatsArme);
 
    tabPerso[0]->afficheEtat();
 
    return 0;
 
}

J'espere que mes explications sont suffisamment claires pour trouver une personne qui pourra m'aider. Attention, je ne suis pas un developpeur chevronné ou averti. Donc je suis preneur d'explications abordables pour un néophyte.

Ai-je fait le bon choix pour le stockage de mes valeurs ? N'y a-t-il pas plus simple pour stocker les valeurs entrées par le User et ensuite les affecter à mon Constructeur Personnage ?

Bref, je suis fan de tous les conseils qui pourront me faire avancer.

PS : Ne faites pas attention à la sécurisation de mon code car je n'ai pas encore travaillé dessus.

Mille Mercis

[JolyLoic]

Il y a quelques points dans ton code que je ne comprends pas (et peut-être que toi non plus ). Donc voici une liste de remarques questions :
- Pourquoi passer un vector<string> comme nom d'un personnage ? Il n'a qu'un nom, n'est-ce pas ?
- Même question pour l'arme : Il semblerait qu'il n'en a qu'une, pourquoi passer des vectors ?
- Plutôt que de passer des string + dégâts au constructeur de personnage, vu que tu as visiblement une classe arme quelque part, pourquoi ne pas lui passer directement une arme préexistante ?
- Pour désigner un pointeur nul, nullptr est plus clair que 0.
- Tu utilises des vector, c'est très bien, mais pourquoi ne pas faire de même pour tabPerso ?
- Ton code de saisie des arme code en dur deux armes existantes. Le jour où tu voudras en ajouter (soit dans le code, soit par fichier de configuration), tu devras modifier de code qui va vite devenir très répétitif. Je te propose de partir vers l'approche suivante :

Code :

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

1
2
3
4
5
6
vector<Arme> armesMagasin {{"Epée", 10}, {"Sabre", 10}, {"Nunchaku", 5}}; // On pourrait le charger dans un fichier
for (Arme : armesMagasin)
{
  // Affiche l'arme, précédée d'un nombre, d'une lettre ou de tout autre identifieur
}
// Demande à l'utilisateur l'arme

Il y a je pense plein d'autres choses à améliorer dans le code, mais je crois que ça constitue déjà une première base d'amélioration.

PS : Je n'ai pas compris ce que tu appelles sécurisation du code. Tu veux dire la protection contre de mauvaise entrées de la part de l'utilisateur ?

[f0x888]

Voici mes réponses à la suite de tes questions/remarques :
- Pourquoi passer un vector<string> comme nom d'un personnage ? Il n'a qu'un nom, n'est-ce pas ? -> Non, en fait, si tu regardes en détail, je propose à l'utilisateur de créer entre 1 à 4 perso. C'est pour cela que j'ai décidé de choisir vector pour stocker les différents noms choisis par le User.
Voici l'extrait du code :

Code :

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

1
2
3
4
5
6
7
for (int i=1 ; i <= nbrePersoACreer ; i++){
            num++;
            string nom;
            cout << "Entrez le nom du personnage n° " << num << " : ";
            cin >> nom;
            nomPerso.push_back(nom);
        }

- Même question pour l'arme : Il semblerait qu'il n'en a qu'une, pourquoi passer des vectors ? -> Alors pour l'arme, je suis d'accord avec toi sur le fait que pour le moment, je pourrai probablement me passer d'un vector étant donné que les valeurs (épée,...) sont en dures. Cela dit, l'idée était de pouvoir, par la suite, proposer à l'utilisateur de créer ses propres armes. Donc du coup, le choix du vector me permettrait de stocker ses créations d'arme dans un tableau.

- Plutôt que de passer des string + dégâts au constructeur de personnage, vu que tu as visiblement une classe arme quelque part, pourquoi ne pas lui passer directement une arme préexistante ? -> Il existe déjà une arme préexistante dans ma classe Arme. Mais je voulais plutôt utiliser le deuxième constructeur Arme, qui n'utilise pas les valeurs par défaut.
extrait du code de la classe Arme :

Code :

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

1
2
3
4
5
6
7
8
9
10
11
12
13
14
 
Arme::Arme(){
 
    m_nom = "pierre";
    m_degats = 2;
 
}
 
Arme::Arme(string nom, int degats){
 
    m_nom = nom;
    m_degats = degats;
 
}

- Pour désigner un pointeur nul, nullptr est plus clair que 0 -> je ne comprends pas trop ta proposition car pour le moment, dans mon main, je n'ai pas encore utilisé de pointeur. J'ai tenté de créer un pointeur *tabPerso qui rassemble les valeurs créer par le User (nomPerso, armePerso, DegatsArme), mais le problème, c'est que je n'arrive pas à utiliser mon tabPerso.
Exemple (probablement faux) :
Personnage tabPerso[0](qui rassemble mon vector<string>nomPerso[0], mon vector<string>armePerso[0], et mon vector<int> degatsArme).
Ensuite, j'utilise ma méthode pour afficher tout cela :
tabPerso[0].afficherEtat().
Voilà en gros l'objectif.

- Tu utilises des vector, c'est très bien, mais pourquoi ne pas faire de même pour tabPerso ? -> La réponse est donnée en grande partir ci-dessus.

PS : Je n'ai pas compris ce que tu appelles sécurisation du code. Tu veux dire la protection contre de mauvaise entrées de la part de l'utilisateur ? -> Oui, c'est ça.

[JolyLoic]

Voici mes réponses à la suite de tes questions/remarques :
- Pourquoi passer un vector<string> comme nom d'un personnage ? Il n'a qu'un nom, n'est-ce pas ? -> Non, en fait, si tu regardes en détail, je propose à l'utilisateur de créer entre 1 à 4 perso. C'est pour cela que j'ai décidé de choisir vector pour stocker les différents noms choisis par le User.
Voici l'extrait du code :

Code :

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

1
2
3
4
5
6
7
for (int i=1 ; i <= nbrePersoACreer ; i++){
            num++;
            string nom;
            cout << "Entrez le nom du personnage n° " << num << " : ";
            cin >> nom;
            nomPerso.push_back(nom);
        }

Oui, j'ai bien vu que tu as 4 personnages, mais chaque personnage n'a qu'un nom. Tu dois donc avoir un vector<Personnage>, mais le Personnage n'a besoin en paramètre du constructeur que d'une string, pas d'un vector<string>.

- Même question pour l'arme : Il semblerait qu'il n'en a qu'une, pourquoi passer des vectors ? -> Alors pour l'arme, je suis d'accord avec toi sur le fait que pour le moment, je pourrai probablement me passer d'un vector étant donné que les valeurs (épée,...) sont en dures. Cela dit, l'idée était de pouvoir, par la suite, proposer à l'utilisateur de créer ses propres armes. Donc du coup, le choix du vector me permettrait de stocker ses créations d'arme dans un tableau.

Là encore, j'ai peur qu'il y ait confusion entre les armes existant globalement, et les armes possédées par un personnage en particulier.

- Plutôt que de passer des string + dégâts au constructeur de personnage, vu que tu as visiblement une classe arme quelque part, pourquoi ne pas lui passer directement une arme préexistante ? -> Il existe déjà une arme préexistante dans ma classe Arme. Mais je voulais plutôt utiliser le deuxième constructeur Arme, qui n'utilise pas les valeurs par défaut.
extrait du code de la classe Arme :

Code :

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

1
2
3
4
5
6
7
8
9
10
11
12
13
14
 
Arme::Arme(){
 
    m_nom = "pierre";
    m_degats = 2;
 
}
 
Arme::Arme(string nom, int degats){
 
    m_nom = nom;
    m_degats = degats;
 
}

Ça n'empêche pas. Là, pour créer un personnage, tu fais :

Code :

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

Personnage conan("Conan", "Epée en acier de Crom", 100); // L'arme est crée à l’intérieur du constructeur de personnage

Je te propose de remplacer ça par :

Code :

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

1
2
Arme epeeCrom("Epée en acier de Crom", 100);
Personnage conan("Conan", epeeCrom);

- Pour désigner un pointeur nul, nullptr est plus clair que 0 -> je ne comprends pas trop ta proposition car pour le moment, dans mon main, je n'ai pas encore utilisé de pointeur.

Dans ton main, non, mais dans ton constructeur de personnage, quand tu écris m_arme(0).

J'ai tenté de créer un pointeur *tabPerso qui rassemble les valeurs créer par le User (nomPerso, armePerso, DegatsArme), mais le problème, c'est que je n'arrive pas à utiliser mon tabPerso.
Exemple (probablement faux) :
Personnage tabPerso[0](qui rassemble mon vector<string>nomPerso[0], mon vector<string>armePerso[0], et mon vector<int> degatsArme).
Ensuite, j'utilise ma méthode pour afficher tout cela :
tabPerso[0].afficherEtat().
Voilà en gros l'objectif.

- Tu utilises des vector, c'est très bien, mais pourquoi ne pas faire de même pour tabPerso ? -> La réponse est donnée en grande partir ci-dessus.

Par moment, on a l'impression que ta classe Personnage décrit un personnage, mais par d'autres moments, on a l'impression qu'elle devrait les décrire tous. Par exemple, dans ta phrase précéente, tu dis que tabPerso[0] rassemble vector<string>nomPerso[0]], c'est à dire un élément du vecteur, une simple string, pas le vecteur complet ! Ce qui rejoint le premier point.

[f0x888]

Ok, je te remercie d'avoir pris le temps de consulter mon code. Je vais prendre en compte l'ensemble des tes recommandations afin de simplifier mon code et voir ce que je peux faire pour éviter d'avoir à gérer tous mes vector...

Par conséquent, je place le topic en résolu, et si j'ai des difficultés, je reviendrai à la charge...

En tout cas, merci de ta réactivité et bon dimanche.

[koala01]

Salut,

Pour compléter (et peut être t'aider à mieux comprendre) la réaction de JolyLoic.

Tu es d'accord que chaque personnage a un et un seul nom et une et une seule arme. Que tu crées 1, 5, 10 ou ... 150 personnages, cela restera le cas.

Dés lors, tu peux demander à l'utilisateur de créer autant de personnage que tu le souhaites (attention, il faudra peut être voir à éviter les doublons au niveau du nom du personnage, car, si tu en as deux qui s'appelle MickeyMouse, tu auras sans doute du mal à choisir lequel est attaqué ou lequel boit sa potion de mana ), et l'une des solutions, pendant l'étape de création peut effectivement passer par le fait de placer les informations introduites par l'utilisateur dans un tableau.

Mais l'idée, une fois que l'utilisateur a créé tous les personnages qu'il voulait, c'est de créer tous les personnages en une fois. Et donc, tu pourrais très bien avoir un code proche de

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
 
// j'aime énormément vérifier que la logique qui a mené à un certain point du programme
// soit vérifiée, de manière à traquer les erreurs dues à mon inattention
// il faut donc avoir le même nombre d'armes que de personnages
assert(nomPerso.size() == armePerso.size() && "nombre d'armes inconsistant");
// et il faut avoir introduit des dégats pour chaque arme
assert(armePerso.size() == degatsArme.size() && "nombre de degats inconsistant");
// Maintenant que l'on sait que chaque personnage a une arme et que chaque arme dispose d'un nombre de dégats
// on peut créer les différents personnages sous une forme proche de
std::vector<Personnage> tousLesPersos; // on le nomme comme on veut, du moment qu'on sait à quoi il sert 
for(size_t i=0; i<nomPerso.size(); ++1){  // j'aurais pu choisir armePerso.size() ou degatsArme.size() vu qu'on sait que 
                                                                 // les trois tableau ont la même taille
    Personnage tmp{nomPerso[i],armePerso[i], degatsArmes[i]); // pour chaque perso créé, il ne faut que l'arme et les dégats qui lui correspondent
    tousLesPersos.push_back(tmp); // on le rajoute dans la liste des personnages existants
}

ou, pour que le constructeur ne prenant que le nom de ton personnage puisse servir, nous pourrions imposer le fait qu'une chaine vide et un nombre de dégats égal à 0 provoquera la création d'une arme "par défaut". Nous pourrions alors modifier ce code pour lui donner une forme proche de

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
 
// j'aime énormément vérifier que la logique qui a mené à un certain point du programme
// soit vérifiée, de manière à traquer les erreurs dues à mon inattention
// il faut donc avoir le même nombre d'armes que de personnages
assert(nomPerso.size() == armePerso.size() && "nombre d'armes inconsistant");
// et il faut avoir introduit des dégats pour chaque arme
assert(armePerso.size() == degatsArme.size() && "nombre de degats inconsistant");
// Maintenant que l'on sait que chaque personnage a une arme et que chaque arme dispose d'un nombre de dégats
// on peut créer les différents personnages sous une forme proche de
std::vector<Personnage> tousLesPersos; // on le nomme comme on veut, du moment qu'on sait à quoi il sert 
for(size_t i=0; i<nomPerso.size(); ++1){  // j'aurais pu choisir armePerso.size() ou degatsArme.size() vu qu'on sait que 
                                                                 // les trois tableau ont la même taille
    if(armePerso[i].empty()){ // si la chaine de caractères représentant l'arme est vide
        // ma logique a-t-elle bien fait attention au fait que, si la chaine de caractères représentant l'arme est vide,
        // le nombre de dégats DOIT est égal à 0 ???
        assert(degatsArme[i]== 0 && "valeur de degats inconsistante avec une l'arme par defaut");
        Personnage tmp{nomPerso[i]); // pour chaque perso créé, il ne faut que l'arme et les dégats qui lui correspondent
        tousLesPersos.push_back(tmp); // on le rajoute dans la liste des personnages existants
    }else{
        Personnage tmp{nomPerso[i],armePerso[i], degatsArmes[i]); // pour chaque perso créé, il ne faut que l'arme et les dégats qui lui correspondent
        tousLesPersos.push_back(tmp); // on le rajoute dans la liste des personnages existants
   }
}

Maintenant, les quelques remarques habituelles :

• using namespace std; ne devrait pas être utilisé dans un code récent. Cette possibilité avait été donnée à l'époque de la standardisation pour permettre à la base de code qui existait à l'époque de continuer à fonctionner "sans trop de modification" quand la bibliothèque standard a décidé de faire passer tout son contenu dans l'espace de noms std... Mais cela date de près de 18 ans, maintenant!!!
• Tu as de magnifiques fuites mémoires, parce que tu recours à l'allocation dynamique de la mémoire à de nombreuses occasions, et que tu en manipule le résultat avec des pointeurs nus
• Pourquoi n'afficher l'état que du premier personnage, et non celui de tous les autres (ligne 112)
• On ne déclare pas toutes les variables au début de la fonction : on les déclare le plus près possible de la première utilisation qui en est faite
• On utilise de préférence la liste d'initialisaiton dans les constructeurs
• L'allocation dynamique de la mémoire n'est vraiment utile que dans certaines circonstances (besoin de profiter du polymorphisme d'inclusion), qui ne sont pas (encore) d'application pour ton projet-->elle n'a donc aucun intérêt pour l'instant, en plus de provoquer des catastrophes au vu de la manière dont tu l'utilise
• Une fonction de 120 lignes (et plus), c'est une fonction qui en fait beaucoup trop. La règle est simple concernant les fonction : pour chaque objectif, il faut une fonction qui ne s'occupe que de ca. Ici, tu as les objectifs suivants:
  
  1. demander à l'utilisateur le nombre de personnages qu'il veut créer (lignes 14 à 19)
  2. demander à l'utilisateur le nom des personnages qu'il veut créer (lignes 24 à 30)
  3. afficher les noms des personnage (lignes 34 à 40)
  4. proposer de changer le nom des personnages (lignes 42 à 48)
  5. permettre à l'utilisateur de choisir l'arme de chaque personnage (ligne 57 à 82)
  6. afficher les armes qui seront affectées à chaque personnage (ligne 83 à 91)
  7. proposer de changer les armes des personnages (lignes 91 à 99)
  8. créer les personnages et afficher leur états (devraient normalement être considéré comme deux responsabilité distinctes ) le reste du code
  9. faire fonctionner l'application (l'ensemble de la fonction main)

Si j'ai bien compté, cela fait donc que ta fonction main s'occupe de dix choses différentes, prend dix responsabilités différentes. C'est 9 de trop. Tu devrais donc avoir 9 fonctions de plus qui s'occupe chacune d'un aspect bien particulier

[f0x888]

Bon bien me voilà bien armé, sans mauvais jeu de mots, pour repartir sur de bonnes bases. Par ailleurs, j'ai eu l'occasion d'échanger avec quelques personnes qui ont pris le temps de m'expliquer certains points sur lesquels je dois travailler (notamment sur l'utilisation des classes de façon efficace).
La route est encore longue avant de réussir à sortir un code simple.
En tout cas, je remercie les contributeurs d'avoir pris le temps de "déchiffrer" mon code.
Très bonne soirée.