[amélioration] Critique de mon programme

**ChipsterJulien** · 16/01/2010, 12h28

Bonjour
J'ai fait un petit programme et j'aimerai avoir vos avis et vos critiques sur ce qui est bien et sur ce qui pourrait être amélioré vu que je suis débutant

Voici les codes sources :

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
import java.util.ArrayList;
import java.util.*;
 
public class Classe {
	private ArrayList<Eleve> tab = null;
 
	public Classe() {
		tab = new ArrayList<Eleve>();
	}
 
	public void addEleve(Eleve eleve) {
		// Vérifier que l'élève n'existe pas déjà
		Boolean test = false;
		for(int i = 0; i < tab.size() && test == false; ++i) {
			if((tab.get(i)).compare(eleve) == true) {
				test = true;
			}
		}
		if(!test) {
			tab.add(eleve);
		}
	}
 
	public void delEleve(Eleve eleve) {
		// Vérifier que l'élève existe
		Boolean trouver = false;
		for(int i = 0; i < tab.size() && trouver == false; ++i) {
			if((tab.get(i)).compare(eleve) == true) {
				trouver = true;
				tab.remove(i);
			}
		}
	}
 
	public void delEleve(int emplacement) {
		tab.remove(emplacement);
	}
 
	// Trouver un eleve
	public int findEleve(Eleve eleve) {
		Boolean trouver = false;
		for(int i = 0; i < tab.size() && trouver == false; ++i) {
			if((tab.get(i)).compare(eleve) == true) {
				trouver = true;
				return i;
			}
		}
		return -1;
	}
 
	public Eleve getEleve(int i) {
		return tab.get(i);
	}
 
	public int getNbEleve() {
		if(tab == null)
			return 0;
		else
			return tab.size();
	}
	public void sortList() {
		Collections.sort(tab);
	}
}

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
import java.io.*;
 
public class EcrireObjet {
	private String nomFichier           = "";
	private ObjectOutputStream ecriture = null;
 
	public EcrireObjet(String nom) {
		setNomFichier(nom);
		try {
			ecriture = new ObjectOutputStream(new FileOutputStream(nom));
		}
		catch(FileNotFoundException e) {
			System.out.println("Fichier introuvable");
			System.exit(1);
		}
		catch(IOException e) {
			System.out.println("Erreur d'E/S");
			System.exit(1);
		}
	}
 
	public void write(Eleve Objet) {
		try {
			ecriture.writeObject(Objet);
		}
		catch(IOException e) {
			System.out.println("Erreur d'E/S");
			System.exit(1);
		}
	}
 
	public void close() {
		if(ecriture != null) {
			try {
				ecriture.flush();
				ecriture.close();
			}
			catch(IOException e) {
				System.out.println("Erreur d'E/S");
				System.exit(1);
			}
		}
	}
 
	public String getNomFichier() {
		return this.nomFichier;
	}
 
	public void setNomFichier(String nomFichier) {
		this.nomFichier = nomFichier;
	}
}

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
import java.util.ArrayList;
import java.io.Serializable;
import java.util.*;
 
public class Eleve implements Serializable, Comparable {
	private String nom      = "";
	private String prenom   = "";
	ArrayList<Double> note  = new ArrayList<Double>();
	double moyenne          = -1;
 
	public Eleve(String nom, String prenom) {
		setNom(nom);
		setPrenom(prenom);
	}
	public Eleve(String nom, String prenom, double note[]) {
		setNom(nom);
		setPrenom(prenom);
		for(int i = 0; i < note.length; ++i) {
			addNote(note[i]);
		}
	}
 
	public void calculMoyenne() {
		if(note.size() > 0) {
			double somme = 0;
 
			for(int i = 0; i < note.size(); ++i) {
				somme += note.get(i);
			}
			this.moyenne = (somme / note.size());
		}
	}
 
	public Boolean compare(Eleve eleve) {
		if(this.nom.equals(eleve.getNom()) && this.prenom.equals(eleve.getPrenom()))
			return true;
		else
			return false;
	}
 
	public int compareTo(Object eleve) {
		String nomEleve1 = nom + prenom;
		String nomEleve2 = ((Eleve)eleve).getNom() + ((Eleve)eleve).getPrenom();
		int comparaison = nomEleve1.compareTo(nomEleve2);
		if(comparaison > 0)
			return 1;
		else if(comparaison == 0)
			return 0;
		else
			return -1;
	}
 
	public String getNom() {
		return nom;
	}
	public String getPrenom() {
		return prenom;
	}
	public double getMoyenne() {
		return moyenne;
	}
	public ArrayList<Double> getNote() {
		return note;
	}
 
	public void setNom(String nom) {
		this.nom = nom;
	}
	public void setPrenom(String prenom) {
		this.prenom = prenom;
	}
	public void setMoyenne(double moyenne) {
		this.moyenne = moyenne;
	}
	public void addNote(double note) {
		this.note.add(note);
	}
	public String toString() {
		calculMoyenne();
		String str = nom + " " + prenom;
		if(moyenne > -1) {
			str += " |" + String.valueOf(((double)(int)(moyenne*100))/100);
			str += "| ";
		}
		else {
			str += " ";
		}
		for(int i = 0; i < note.size(); ++i) {
			str += String.valueOf(note.get(i)) + " ";
		}
		return str;
	}
}

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
import java.io.*;
 
public class LireObjet {
	private String nomFichier         = "";
	private ObjectInputStream lecture = null;
 
	public LireObjet(String nom) {
		setNomFichier(nom);
		try {
			lecture = new ObjectInputStream(new FileInputStream(nomFichier));
		}
		catch(IOException e) {
			System.out.println("Erreur d'E/S");
			System.exit(1);
		}
	}
 
	public void close() {
		try {
			lecture.close();
		}
		catch(IOException e) {
			System.out.println("Erreur d'E/S");
			System.exit(1);
		}
	}
 
	public Eleve getObjet() {
		Eleve personne = null;
		try {
			personne = (Eleve)lecture.readObject();
		}
		catch(EOFException e) {
			return null;
		}
		catch(IOException e) {
			System.out.println("Erreur d'E/S");
			System.exit(1);
		}
		catch(ClassNotFoundException e) {
			System.out.println("Objet inconnu");
			System.exit(1);
		}
		return personne;
	}
 
	public void setNomFichier(String nomFichier) {
		this.nomFichier = nomFichier;
	}
}

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
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
import java.io.*;
import java.util.*;
 
public class zEcole {
	public static void main(String[] args) {
		LireObjet source         = null;
		EcrireObjet cible        = null;
 
		// Teste si le fichier existe
		// S'il n'existe pas on le crée
		try {
			if(!new File(args[0]).exists()) {
				cible = new EcrireObjet(args[0]);
				cible.close();
				cible = null;
			}
		}
		catch(ArrayIndexOutOfBoundsException e) {
			System.out.println("Nombre d'argument incorrect");
			help();
		}
		//Chargement du fichier
		try {
			source = new LireObjet(args[0]);
		}
		catch(ArrayIndexOutOfBoundsException e) {
			System.out.println("Nombre d'argument incorrect");
			help();
			System.exit(1);
		}
 
		// Charger les objets dans le tableau de la classe
		Classe classe = new Classe();
		Eleve eleve = null;
		while((eleve = source.getObjet()) != null) {
			classe.addEleve(eleve);
		}
		// On ferme le fichier
		source.close();
 
		String option = "";
		try {
			option = args[1];
		}
		catch(ArrayIndexOutOfBoundsException e) {
			System.out.println("Nombre d'arguments incorrect");
			help();
		}
		if(option.equals("-l")) {
			classe.sortList();
			for(int i = 0; i < classe.getNbEleve(); ++i) {
				System.out.println(classe.getEleve(i));
			}
		}
		else if(option.equals("-a")) {
			eleve = null;
			try {
				eleve = new Eleve(args[2], args[3]);
			}
			catch(ArrayIndexOutOfBoundsException e) {
				System.out.println("Nombre d'arguments incorrect");
				help();
			}
			classe.addEleve(eleve);
		}
		else if(option.equals("-d")) {
			eleve = null;
			try {
				eleve = new Eleve(args[2], args[3]);
			}
			catch(ArrayIndexOutOfBoundsException e) {
				System.out.println("Nombre d'arguments incorrect");
				help();
			}
			classe.delEleve(eleve);
		}
		else if(option.equals("-n")) {
			int nbArgument = args.length - 4;
			if(nbArgument <= 0) {
				help();
			}
			eleve = null;
			try {
				eleve = new Eleve(args[2], args[3]);
			}
			catch(ArrayIndexOutOfBoundsException e) {
				System.out.println("Nombre d'arguments incorrect");
				help();
			}
 
			int retour = classe.findEleve(eleve);
			if(retour == -1) {
				for(int i = 4; i < args.length; ++i) {
					eleve.addNote(Double.valueOf(args[i]));
				}
				classe.addEleve(eleve);
				eleve = null;
			}
			else {
				eleve = classe.getEleve(retour);
				for(int i = 4; i < args.length; ++i) {
					eleve.addNote(Double.valueOf(args[i]));
				}
				classe.delEleve(retour);
				classe.addEleve(eleve);
				eleve = null;
			}
		}
		else {
			help();
		}
 
		cible = new EcrireObjet(args[0]);
		// On écrit l'intégralité de la classe
		for(int i = 0; i < classe.getNbEleve(); ++i) {
			cible.write(classe.getEleve(i));
		}
		//On ferme le fichier
		cible.close();
	}
 
	// On défini l'aide du programme
	public static void help() {
		System.out.println("Usage : zEcole file <option>");
		System.out.println("  <option> :");
		System.out.println("             -a nom prénom               : Ajoute un élève\n"
			               + "             -d nom prénom               : Supprime un élève\n"
								 		 + "             -l                          : Liste les élèves\n"
							 			 + "             -n nom prénom note note ... : Ajoute des notes");
		System.exit(0);
	}
}

Voilà

**polymorphisme** · 16/01/2010, 15h47

Bonjour,

Code :

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

1
2
import java.util.ArrayList;
import java.util.*;

La première instruction permet de charger la classe ArrayList du paquetage java.util.
La seconde instruction permet de charger toutes les classes du paquetage java.util.
Tu as donc redondance.

Après, on peut tjrs faire des améliorations ...

**ChipsterJulien** · 16/01/2010, 15h57

C'est vrai que j'ai mis ça pour raccourcir. Il faudrait faire un joli

Code :

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

import java.util.Collections;

pour que ça soit mieux
Ça a une grosse conséquence pour la suite de mettre des

Code :

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

 import java.util.*;

où ça ne joue que sur les temps de compilation ?

Autrement, d'autres remarques ?

**Auteur** · 16/01/2010, 19h38

bonjour,

étant moi même un débutant

, je vais te donner également mon avis.

Personnellement j'ai une sainte horreur des déclarations de variables qui se baladent au milieu du code. J'ai la vieille (

) habitude (bonne ou mauvaise ?) de déclarer toutes mes variables au début de chaque méthode comme ça quand je relis mon code (surtout s'il est long), je n'ai pas à chercher sa déclaration.

En ce qui concerne ta méthode main (et cela appuiera ma remarque précédente) :

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
		String option = "";
		try {
			option = args[1];
		}
		catch(ArrayIndexOutOfBoundsException e) {
			System.out.println("Nombre d'arguments incorrect");
			help();
		}
		if(option.equals("-l")) {
			classe.sortList();
			for(int i = 0; i < classe.getNbEleve(); ++i) {
				System.out.println(classe.getEleve(i));
			}
		}
		else if(option.equals("-a")) {
			eleve = null;
			try {
				eleve = new Eleve(args[2], args[3]);
			}
			catch(ArrayIndexOutOfBoundsException e) {
				System.out.println("Nombre d'arguments incorrect");
				help();
			}
			classe.addEleve(eleve);
		}
		else if(option.equals("-d")) {
			eleve = null;
			try {
				eleve = new Eleve(args[2], args[3]);
			}
			catch(ArrayIndexOutOfBoundsException e) {
				System.out.println("Nombre d'arguments incorrect");
				help();
			}
			classe.delEleve(eleve);
		}
		else if(option.equals("-n")) {
			int nbArgument = args.length - 4;
			if(nbArgument <= 0) {
				help();
			}
			eleve = null;
			try {
				eleve = new Eleve(args[2], args[3]);
			}
			catch(ArrayIndexOutOfBoundsException e) {
				System.out.println("Nombre d'arguments incorrect");
				help();
			}
 
			int retour = classe.findEleve(eleve);
			if(retour == -1) {
				for(int i = 4; i < args.length; ++i) {
					eleve.addNote(Double.valueOf(args[i]));
				}
				classe.addEleve(eleve);
				eleve = null;
			}
			else {
				eleve = classe.getEleve(retour);
				for(int i = 4; i < args.length; ++i) {
					eleve.addNote(Double.valueOf(args[i]));
				}
				classe.delEleve(retour);
				classe.addEleve(eleve);
				eleve = null;
			}
		}
		else {
			help();
		}
 
		cible = new EcrireObjet(args[0]);
		// On écrit l'intégralité de la classe
		for(int i = 0; i < classe.getNbEleve(); ++i) {
			cible.write(classe.getEleve(i));
		}
		//On ferme le fichier
		cible.close();

Pourquoi ne pas déclarer int i au début de ta méthode ? Ça t'éviterai ces

Code :

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

for(int i = 0; i < classe.getNbEleve(); ++i)

Même remarque pour

Code :

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

eleve = new Eleve(args[2], args[3]);

**Ivelios** · 16/01/2010, 20h29

Plusieurs petites choses à modifier selon moi :

Classe.addEleve -> ajout d'un break pour ne pas parcourir TOUT l'array si ce n'est pas nécessaire.
Classe.delEleve -> Vérifier que "int emplacement" est compris entre 0 et tab.size();
Eleve.compare-> tu peux directement faire

Code :

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

return this.nom.equals(eleve.getNom()) && this.prenom.equals(eleve.getPrenom())

pourquoi faire compare() alors qu'il y a compareTo()?

Pour toutes les classes :
Mettre les attributs des classes en private ( tu en as oublié )
Dans les constructeurs, mettre this.nom = nom au lieu de setNom(nom) ... ( ça dépend des gouts

)
Initialiser les attributs des classes dans les constructeurs et pas avant ( ça dépend aussi des gouts )

Tous ce qu'il y a au dessus c'est du "pipi ce chat" comparé à la dernière critique...
Suspens insoutenable.

Commentaires et javadoc. Il y a seulement un ou deux commentaires qui se battent en duel dans ton code. C'est pas dramatique puisque le programme n'est pas trop compliqué mais prend l'habitude dès maintenant de le faire.

Code :

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

1
2
3
4
5
6
7
//Commentaire sur une ligne
/* Commentaire
sur
plusieurs
ligne */
/** 2 étoiles, à mettre au début de chaque méthodes et de chaque classe
avec @param, @return, @pre & @ post si nécessaire */

Le mot de la fin : Désolé d'avoir la critique facile mais c'est des choses qu'on a pas arrêté de me rabâcher quand je codais.
Et il fallait que je me venge sur quelqu'un... tu étais la cible parfaite

**herve91** · 17/01/2010, 00h30

Bonsoir,
plusieurs remarques signalées en rouge.

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
public class Classe {
	private ArrayList<Eleve> tab = null;// inutile!

	public void addEleve(Eleve eleve) {
		...
	} // utiliser un Set et faire :
public void addEleve(Eleve eleve) {
   set.add(eleve);
}

	public void delEleve(Eleve eleve) {
		...
	}
} // utiliser un Set et faire :
public void delEleve(Eleve eleve) {
   set.remove(eleve);
}

	public void delEleve(int emplacement) {
		tab.remove(emplacement);
	}
 // inutile si un Set est utilisé

	// Trouver un eleve
	public int findEleve(Eleve eleve) {
		...
	}
} // utiliser un Set et faire :
public boolean findEleve(Eleve eleve) {
   return set.contains(eleve);
}
	public Eleve getEleve(int i) {
		return tab.get(i);
	}
 // inutile si un Set est utilisé

	public int getNbEleve() {
		...
	} // return set.size();


	public void sortList() {
		Collections.sort(tab);
	} // dans ce cas utiliser un SortedSet / TreeSet

}

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
public class Eleve implements Serializable, Comparable<Eleve>
 {
	...
	double moyenne          = -1; // pourquoi -1 et non 0 ?
// si on conserve le résultat du calcul de la moyenne il faut penser
// à "invalider" (ou recalculer) la variable moyenne dès qu'une note est
// ajoutée par addNote() 
	
	public Eleve(String nom, String prenom, double note[]
// préférer double ...notes
) {
		...
	}

	public Boolean compare(Eleve eleve) {} // inutile => définir la méthode equals(Object o)
		...
	}

	public int compareTo(Eleve eleve) {
		int comparaison = nomEleve1.compareTo(nomEleve2);
		return comparaison;

	}

	public List<Double> getNote() {
		return note;
	} // il est déconseillé voire dangereux de renvoyer une variable mutable
// utiliser par exemple : return Collections.unmodifiableList(note);


	public void setMoyenne(double moyenne) {
		this.moyenne = moyenne;
	} // quelle est la sémantique de cette méthode ?

	public String toString() {
		calculMoyenne();
		 // utiliser un StringBuilder()
String str = nom + " " + prenom;
	}
}

Code :

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

1
2
3
4
public class zEcole {
	public static void main(String[] args) {
	le main fait beaucoup trop de choses

**ChipsterJulien** · 17/01/2010, 20h36

Waaaaa, merci pour tous ces renseignements. Je vais me mettre dans les corrections / améliorations

Non, franchement si je poste, j'étais prêt à encaisser de me faire allumer

**_skip** · 18/01/2010, 08h43

Petit truc inacceptable :

Code :

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

1
2
3
4
5
6
7
8
9
10
11
12
13
14
public EcrireObjet(String nom) {
		setNomFichier(nom);
		try {
			ecriture = new ObjectOutputStream(new FileOutputStream(nom));
		}
		catch(FileNotFoundException e) {
			System.out.println("Fichier introuvable");
			System.exit(1);
		}
		catch(IOException e) {
			System.out.println("Erreur d'E/S");
			System.exit(1);
		}
	}

C'est une très mauvaise pratique de laisser une classe réagir à une exception de manière autonome, tout comme d'afficher dans la console.

Si tout à coup tu veux utiliser ce code dans une application swing ou une application web, ça va très mal se passer.
Laisse remonter l'exception et fais ton catch dans le code appelant, et là tu décides si tu réessaies, comment tu affiches l'erreur et autre suivant le contexte.

**polymorphisme** · 18/01/2010, 13h47

Bonjour,

tu n'en prends pas "plein la tête", tu apprends à coder correctement, ta démarche est très bien. Comme apprécier le bon vin ? Goutez en du mauvais ...

Lorsque tu nommer des objets, je te déconseille de mélanger le français et l'anglais car au bout d'un certains temps ou d'une certaine taille de ton projet, tu ne saura plus comment tu as fait. Dans la mesure du possible, il faut essayer de reste simple dès que l'on peut.

Une petite rectification que tu aura peut-être remarquer :

Code :

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

1
2
3
4
5
6
public String toString() {
		calculMoyenne();
		 // utiliser un StringBuilder()
String str = nom + " " + prenom;
return str;
	}

Il serais interessant que tu fasse passer ton code après les corrections que tu lui aura apporter.

**ChipsterJulien** · 21/01/2010, 13h59

Je regarde ça dès que j'ai un moment de libre et je poste le résultat

[amélioration] Critique de mon programme

avec Java

Vue hybride

Discussions similaires

Partager

Partager