Manipulation de classes en c++

Une classe CoffreFort contient des pierres précieuses. L’accès à ce coffre se fait par code(pour l’ouvrir et retirer des pierres). Cependant, il est possible d’ajouter des pierres sans ouvrir le coffre. La capacité globale du coffre est limitée (pour simplifier, en nombre de pierres). Un certain nombre de fonctionnalités sont implémentées : ouverture et fermeture du coffre, vérification que le coffre n’est pas plein, ajout d’une pierre ,retrait d’une pierre , valeur, poids et volume du coffre.

Pour télécharger le programme : http://julien.marmin.free.fr/perso/index.htm

Il est vide ton zip !
Tu aurais pu t'en rendre compte, avec si peu d'octets

Oups ! lol

Je modifierai ça quand j'aurais le temps... (mais c'est pas pour quelques lignes de codes qu'il va être lourd le .zip !)

non mais bon... un octet c'est un caractère...
Je ne vois pas comment coder une classe entière avec moins de 200 caractères ^^

C'est bon c'est modifié ^^
220Ko :p

Je vais me permettre de te faire toutes les remarques qui me passent par la tête. Visiblement c'est du bon code (à première vue) cependant je crois que quelques conventions de "bonne pratique" peuvent t'être utiles

1. Le nom de fichiers source.
Remarque pour toutes les librairies existantes, les noms de fichiers commencent généralement par une minuscule, sauf si le nom commence par une abréviation. Ainsi "coffre_fort.h" et "pierre.h" auraient été mieux.

2. Les '#define'
Afin de ne pas interférer avec les autres define défini avec pour but de fournir des valeurs, il est préférable de les noter en majuscules, avec le même nom que le fichier, les différents mots étants séparer par des underscore, et avec le h de la fin. Ce sui aurait donné par exemple
#ifndef COFFRE_FORT_H
#define COFFRE_FORT_H

(note#1: pour ce qui est de la syntaxe, tu utilises la même que moi, que je tire de je ne sais plus où comme convention, et qui est très efficace. A savoir
NomDeClasse
fonctionDeClasse
variable_de_classe
C'est très facile de voir ce qui est quoi n'importe où dans le code.)

(note#2: j'aime aussi le fait que tu ne mette pas de variables en paramètres dans les .h, c'est une erreur à ne pas faire, les types suffisent.)

3. Les noms de variables
Dans ce cas ci je ne sais pas si c'était imposé ou non, mais une fonction qui s'appelle getValeur() moi ça me perturbe. Soit tout en français (erk les accents) soit tout en anglais (c'est mieux), mais les deux ensembles c'est boiteux. La seule raison pour laquelle j'utilise parfois des mots français c'est quand je veux absolument appeler ma variable d'une certaine façon, mais que ce mot est un mot-clef du langage, qui ne peut donc être utilisé. getValue(), getWeight(),... ça aurait été mieux (et rien ne t'empêche de faire des commentaires en française autours de variables en anglais

(note#3: Utiliser les unsigned quand la valeur ne peut être que positive, c'est très bien )

4.GROSSE ERREUR ! avec l'aléatoire.
srand(time(NULL)); ne doit jamais se placer dans un constructeur ! Il doit être appelé une seule fois au début du code et c'est tout.
Pourquoi ?
Pour comprendre cela, il faut connaitre un minimum les générateurs de nombres aléatoires. Le principe est simple: l'algorithme de nombres aléatoire est réglé avec des valeurs (bien choisies) a b et m. A partir d'un nombre x < m, on crée un autre nombre (ax+b)%m. Souvent, a et b sont grand, ce qui fait que le nouveau nombre a toutes les chances d'avoir été tronqué par le modulo, et ce genre de comportement est très peu prévisible, donc pseudo aléatoire. Oui, mais il faut bien un nombre de départ ! Si on partait tout le temps de 1, on aurait tout le temps la même séquence (a,b,et m ne changent pas). Il faut donc choisir ce premier nombre avec un nombre lui aussi à peu près aléatoire. Ce nombre, c'est time(NULL). Il s'agit en fait du nombre de secondes (ou de millièmes je ne sais plus, mais c'est pas important) qui se sont écoulées depuis le premier janvier 1970. Ce nombre variant tout le temps et assez rapidement, les séquences seront toujours différentes.
Et c'est là que le problème intervient. Immagine que tu créer 1000 objets dans la même seconde (ou dans le même millième de seconde (c'est rapide un ordi)), a chaque fois, tu réinitialise ta séquence aléatoire SUR LE MÊME NOMBRE, et donc toutes tes pierres seront identiques ! par contre, si tu le fais une seule fois au début du programme, il poursuivra sur sa séquence avec ses nombres pseudo aléatoires. De plus, il faut toujours initialiser tes variables à 0 dans un constructeur que tu voudrais sans action. Bien que les compilateurs mettent naturellement les variables à 0 quand ils les déclarent, ce n'est pas une obligation, et ne fait pas partie de la stricte norme de c++.

5. Erreur de code
Ta fonction Pierre::Pierre(unsigned int,unsigned int,unsigned int) est déclarée dans le .h mais non définie dans le .cpp


6. Simple question
Pourquoi ne pas avoir appeler les fonctions set dans le constructeur par défaut de Pierre ?
Ca éviterait d'avoir un objet incohérent (poids nul) à la sortie d'un constructeur. Imagine que quelque part dans tes calculs, tu collectionne des pierres, et puis tu décide de calculer le poids relatif d'une pierre, et imagine qu'un utilisateur a déclaré tes pierres sans appliquer les set. Il fera alors une division par 0, ce qui fera planter ton code à l'intérieur de TA classe qui lui ne pourra pas visiter pour corriger (en théorie). Il faut toujours éviter qu'un objet, une fois créé, soit incohérent. C'est un fondement de l'orienté objet.

7. Remarque sur les types
Comme je te l'ai dit plus haut en bien, ça aurait été pas mal d'utiliser des unsigned int pour la taille et le nombre de pierres dans le coffre, qui sont des valeurs obligatoirement positives.

8. Erreur sur les pointeurs
Pierre[16] contiendra 16 pierre (indices de 0 à 15) et non 17 !!!

9. Erreur de déclaration et fuite mémoire
Tu as un gros problème dans ta classe CoffreFort au niveau du constructeur. Tu déclare dans la classe un tableau Pierre tab[16]; et d'autre part dans le constructeur Pierre* tab = new Pierre[16];
Ta seconde déclaration va compter pour du beurre ! Tu es au sein d'une fonction, ce qui signifie que c++ va accepter que tu déclares une variable qui a le même nom qu'une variable qui est en dehors de cette fonction, et temporairement, tab indiquera la nouvelle fonction et non l'ancienne. De plus, ce pointeur Pierre* tag sera perdu à la sortie du constructeur, et ce qu'il pointe (les 16 pierres dans le vecteur) ne seront jamais détruites de la mémoire avant la fin du programme. Cela signifie que tu as une fuite mémoire. Tu peux par exemple faire le test suivant:
while(true)
CoffreFort cf;
Et regarde ta RAM dans les options de windows, elle va vite exploser, déborder sur ton disque dur, et crasher le programme.
De plus, erreur supplémentaire, pour éviter des fuites mémoires, lorsqu'un constructeur déclare des choses avec new, il faut OBLIGATOIREMENT un destructeur pour les effacer ! Tu es donc doublement fautif sur ce coup ^^
Ce que tu aurais du faire, c'est soit conserver ta déclaration initiale Pierre tab[16]; et ne pas créer de vecteur dans le constructeur, soit déclarer un pointeur Pierre* tab; dans ton fichier .h, puis faire un tab = new Pierre[16]; dans le constructeur et un if(tab != NULL) delete [] tab; dans un destructeur.

10. Bête question
Pourquoi n'as tu pas une variable dans la classe CoffreFort qui spécifie si le coffre est ouvert ou fermé ? Car pour le moment, la fonction CoffreFort::ouvertureCoffre() pourrait être mise en "const", elle n'affecte absolument pas le coffre, elle ne fait que spécifier si le code introduit est correct ou non.
De plus, un telle variable pourrait autoriser ou refuser l'utilisations d'autres fonctions qui ne seraient possible que si le coffre est ouvert ou fermé.

11. Erreur de segmentation en vue !
Une erreur de segmentation c'est aller écrire en dehors de ton vecteur, ça fait des comportements bizarres ou ça fait planter le programme. Comme je l'ai dit, impossible d'avoir 17 pierres dans ton coffre, donc ton test <18 dans ta fonction CoffreFort::ajoutPierre() n'est pas correcte.

12. Librairie inutile
Il est inutile d'appeler dans main.cpp puisque tu l'a fait dans main.h

13. Les commentaires
En principe, il faut peu de commentaire. Un bon code doit se suffire à lui-même par les noms de variables. De plus, on n'écrit pas un commentaire en supposant que le lecteur ne connait par le langage.
Par exemple il est totalement inutile (cela surcharge même les choses) d'écrire
unsigned int poids; /* poids de la pierre */
la variable se suffit à elle-même
De même, un commentaire comme celui-ci est totalement inutile:
CoffreFort(); /* constructeur par défaut de la classe CoffreFort */
Les commentaires doivent se contenter de justifier ce qui peut ne pas être immédiat pour le lecteur, éclaircir le code quand il y a des long if-else (par exemple rappeler de quel if il s'agit à côté du else), justifier ou expliquer des calculs, etc...






Voila mon avis général sur ce code, qui est dans l'ensemble très bien, même si j'y ai trouvé beaucoup de choses à dire

Sinon j'aime beaucoup le nouveau design de ta page perso :p

Et COMING SOON (si ça vous intéresse) une classe qui permet de lire ou de créer des fichiers bmp et d'extraire les pixels dans une matrice.

Wahou ! Ca fait plaisir !!! (je parle pour le design de mon site et de ta critique pour le coffre-fort)

Je vais prendre en compte ce que tu m'as dit plus tard et le modifier pour améliorer ce coffre, là je vais dormir !

Merci pour toutes tes remarques !