Revue de code :)

Version imprimable

Revue de code :)

Bonjour,

Petite revue de code, autrement dit critiques constructives demandées pour le code ci-dessous :

L'idée étant simplement de pouvoir lire le contenu d'un fichier texte (encodé en UTF-8) et d'afficher le contenu sur la sortie console avec le numéro de chaque ligne.

Version alpha
Initialement, j'avais envisagé qqch comme :
Code:

1 2 3 4 5 6 7 8 FileReader fr = new FileReader(FICHIER_NOM); BufferedReader br = new BufferedReader(fr); LineNumberReader lnr = new LineNumberReader(br); String ligne = null; while( null != ( ligne = lnr.readLine() ) ) { System.out.println( lnr.getLineNumber() + " : " + ligne ); }
Cependant, le constructeur de FileReader utilise toujours l'encodage par défaut du système, qui peut être différent d'un OS à l'autre? De fait, il est préférable de préciser l'encodage, ce qui peut se faire via InputStreamReader. Confirmez-vous ?

Version beta
Un petit programme complet, qui tourne, suivi de quelques questions :
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 package testjava; import java.io.*; import java.nio.charset.*; public class Program { private static String FICHIER_NOM; private static Charset FICHIER_ENCODAGE; static { FICHIER_NOM = "LISEZMOI.TXT"; FICHIER_ENCODAGE = StandardCharsets.UTF_8; } private static void lireFichier() throws FileNotFoundException, IOException { FileInputStream fis = null; InputStreamReader isr = null; BufferedReader br = null; LineNumberReader lnr = null; String ligne = null; try { fis = new FileInputStream(FICHIER_NOM); isr = new InputStreamReader(fis, FICHIER_ENCODAGE); br = new BufferedReader(isr); lnr = new LineNumberReader(br); while( null != ( ligne = lnr.readLine() ) ) { System.out.println( lnr.getLineNumber() + " : " + ligne ); } } finally { if( lnr != null ) lnr.close(); // Est-ce utile ? else if( br != null ) br.close(); else if( isr != null ) isr.close(); else if( fis != null ) fis.close(); } } public static void main(String[] args) { try { lireFichier(); } catch( Exception ex ) { System.out.println( "Une erreur est survenue : " + ex.toString() ); } } }
Ouverture et lecture du fichier sont réalisées dans un try{...}finally{...}. Trois questions à ce sujet :

La méthode close() de LineNumberReader est héritée de BufferedReader... de fait, est-il possible (souhaitable?) de n'appeler close() que sur br ? Autrement dit de remplacer "if( lnr != null ) lnr.close(); else if( br != null ) br.close(); else if..." par "if( br != null ) br.close(); else if..." et de ne pas se soucier de réaliser un close() sur lnr ?
Est-il préférable de catcher les erreurs ? Là, je m'en sors facilement via un simple "throws FileNotFoundException, IOException" dans la déclaration de ma fonction lireFichier() : est-ce une bonne pratique ?
Si vous catchez, catchez-vous en imbriquant les try/catch les uns dans les autres ? Catchez-vous également les erreurs possibles sur les closes() dans le catch "final" ?

Version finale
Toutes ces questions sur try/catch m'ont fait penser à l'utilisation de "try-with-resources", et à coder finalement mon petit programme comme suit :
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 package testjava; import java.io.*; import java.nio.charset.*; public class Program { private static String FICHIER_NOM; private static Charset FICHIER_ENCODAGE; static { FICHIER_NOM = "LISEZMOI.TXT"; FICHIER_ENCODAGE = StandardCharsets.UTF_8; } private static void lireFichier() throws FileNotFoundException, IOException { String ligne = null; try ( FileInputStream fis = new FileInputStream(FICHIER_NOM); InputStreamReader isr = new InputStreamReader(fis, FICHIER_ENCODAGE); BufferedReader br = new BufferedReader(isr); LineNumberReader lnr = new LineNumberReader(br); ) { while( null != ( ligne = lnr.readLine() ) ) { System.out.println( lnr.getLineNumber() + " : " + ligne ); } } } public static void main(String[] args) { try { lireFichier(); } catch( Exception ex ) { System.out.println( "Une erreur est survenue : " + ex.toString() ); } } }
Je ne vois plus rien à améliorer, mais peut-être pensez-vous autrement, auquel cas je suis intéressé par vos remarques !

A vos claviers ;)

18/09/2014, 11h19
thelvin
Citation:

Envoyé par Isidore.76

Cependant, le constructeur de FileReader utilise toujours l'encodage par défaut du système, qui peut être différent d'un OS à l'autre? De fait, il est préférable de préciser l'encodage, ce qui peut se faire via InputStreamReader. Confirmez-vous ?

Oui. Ce n'est pas pour rien qu'avec Java 1.7, quand ils ajouté la classe utilitaire Files et ses méthodes newBufferedReader() et readAllLines(), ils leur ont donné un paramètre obligatoire pour indiquer le charset.

Citation:

Envoyé par Isidore.76

La méthode close() de LineNumberReader est héritée de BufferedReader... de fait, est-il possible (souhaitable?) de n'appeler close() que sur br ? Autrement dit de remplacer "if( lnr != null ) lnr.close(); else if( br != null ) br.close(); else if..." par "if( br != null ) br.close(); else if..." et de ne pas se soucier de réaliser un close() sur lnr ?

Comme tu es en lecture, il n'est vraiment nécessaire de fermer que le FileInputStream : c'est lui qui a ouvert des ressources système, c'est-à-dire une ouverture de fichier. Le reste se contente de tirer les données de ce FileInputStream et de les traiter en mémoire. Leur méthode close() délègue simplement close() à l'objet sur lequel ils s'appuient, redescendant donc jusqu'au FileInputStream.
Ça veut dire qu'en théorie, tu aurais besoin d'appeler close() que sur un seul de ces objets, le plus haut, le plus bas, peu importe ça fera descendre la fermeture jusqu'à la vraie ressource à fermer. En pratique, toutefois, créer le InputStreamReader à partir du FileInputStream peut échouer. Et donc, à ce moment-là, l'InputStreamReader, le BufferedReader et le LineNumberReader n'existent pas et ne peuvent pas être fermés. Le FileInputStream, lui, existe bel et bien et doit être fermé. Il est donc indispensable de bien le fermer lui, directement, au cas où on ne puisse pas fermer les autres parce qu'on n'a pas pu les créer.

J'ai commencé par "comme tu es en lecture," car en écriture c'est différent. Ce que tu écris dans un Writer buffeurisé ne sera envoyé au système de fichier que quand le buffeur sera vidé, et cela n'arrivera comme il faut que si tu fermes le Writer qui s'en sert. Il peut aussi y avoir d'autres traitements à faire quand on finit d'écrire, genre si tu écris en UTF-7, ou si tu compresses, chiffres ou encode en base64 à la volée. En écriture donc, il est indispensable de fermer en premier le maillon qu'on a ouvert en dernier. Et il est indispensable de fermer aussi ceux qui ont servi à construire la chaîne, dans l'ordre inverse, pour bien libérer les ressources au cas où quelque chose échoue dans la fermeture de la chaîne.

Du coup comme comme dans un cas il faut absolument fermer tous les objets... Il vaut peut-être mieux faire pareil aussi dans tous les cas. Ça fait un peu cargo culte dans les cas où, en fait, ce n'était pas nécessaire. Mais ça évite de devoir se demander si on est sûr qu'on est bien dans un cas où on peut ci ou on peut ça. "C'est fermable alors on le ferme," ça évite des erreurs.

Cela dit, c'est là que Files.newBufferedReader(), Files.readAllLines() et Files.lines() de Java 1.7 et 1.8 prennent beaucoup de sens.
Files.newBufferedReader() et Files.lines() renvoient un seul objet à fermer pour libérer les ressources. Elles s'occupent elles-mêmes du reste de la chaîne.
Files.readAllLines() lit le fichier à notre place et nous renvoie le résultat, elle s'occupe elle-même d'ouvrir et fermer les ressources pour ce faire, on n'a pas à s'en occuper.
Rien de tout ça ne fournit de décompte de lignes comme LineNumberReader... Mais bon, un compteur de lignes c'est pas la mer à boire à faire, quand même. J'ai jamais vu personne se servir de LineNumberReader dans le monde réel.

Citation:

Envoyé par Isidore.76

Est-il préférable de catcher les erreurs ? Là, je m'en sors facilement via un simple "throws FileNotFoundException, IOException" dans la déclaration de ma fonction lireFichier() : est-ce une bonne pratique ?

La bonne pratique c'est de faire ce qui est dicté par le besoin.
La plupart du temps, s'il y a une erreur alors il y a une erreur : ça n'a pas marché et il faut remonter une Exception, quoi d'autre ?
Mais on peut imaginer dans certains cas que l'erreur soit parmi les cas prévus et qu'on veuille réagir autrement qu'en renvoyant une erreur, ce qui justifie un catch. Par exemple en essayant de lire autre chose à la place, bien qu'en principe ça devrait plutôt être l'appelant qui s'en occupe.

Citation:

Envoyé par Isidore.76

Si vous catchez, catchez-vous en imbriquant les try/catch les uns dans les autres ?

Même chose ça dépend du besoin, mais en principe je ne catcherais que l'englobant, dans la mesure où l'endroit précis où ça a échoué ne me concerne pas.
Sauf si, justement, l'endroit précis où ça a échoué me concerne.

Citation:

Envoyé par Isidore.76

Catchez-vous également les erreurs possibles sur les closes() dans le catch "final" ?

Idem.
Cela dit dans le cas où un close() en lecture échoue, on peut se demander ce qu'on peut y faire. On n'a pas perdu de travail puisque ce n'est pas de l'écriture, le problème reste que la ressource n'a pas été fermée, et la seule chose à faire contre ça est d'arrêter le programme puisque close() n'a pas pu le faire.

Citation:

Envoyé par Isidore.76

Je ne vois plus rien à améliorer, mais peut-être pensez-vous autrement, auquel cas je suis intéressé par vos remarques !

- On ne peut qu'approuver le choix de try-with-resources.

- Je suggère fortement Files.lines() de Java 1.8 au lieu d'ouvrir tous ces objets intermédiaires. Éventuellement Files.readAllLines() à la place, si lire le fichier intégralement en mémoire n'est pas un problème.

- Ce bloc static n'est pas très logique, il vaut mieux assigner directement les variables au moment de leur déclaration. D'ailleurs en principe on n'écrit en majuscule que les variables static final immutables, ce qui en fait des genres de "constantes" même si ce ne sont pas forcément des vraies constantes. Là elles sont immutables et tu n'as aucune raison de ne pas les rendre final, donc le mieux serait de le faire. Ça donne :
Code:

1 2 private static final String FICHIER_NOM = "LISEZMOI.TXT"; private static final Charset FICHIER_ENCODAGE = StandardCharsets.UTF_8;
C'est un peu plus clair quand on duplique pas les lignes, non ?

- Ce n'est pas toujours possible, mais il faut éviter d'initialiser des variables à null. D'ailleurs il vaut mieux les déclarer au plus proche de l'endroit où on s'en sert.
Un while( null != ( ligne = lnr.readLine() ) ) ça a tôt fait d'être remplacé par autre chose qui fait pas ce qu'il faut.

J'encouragerais plutôt un truc genre :

Code:

for(String ligne = reader.readLine(); ligne != null; ligne = reader.readLine())

Ou
Code:

1 2 3 4 5 String ligne = reader.readLine(); while(ligne != null) { // ... ligne = reader.readLine(); }
On se répète mais ça se lit plus directement.

Cela dit, avec Files.readAllLines(), on obtient :
Code:

1 2 3 4 5 int ligneNb = 1; for(String ligne : lignes) { // ... ligneNb++; }
au prix de lire le fichier entièrement en mémoire. À voir quand on sait que c'est adapté.

- le nom de classe Program est pas terrible. WriteLineNumbers, par exemple ?

Merci pour ta réponse, longue, argumentée, instructive, bref excellente !

Ci-dessous, donc, une version tenant compte de plusieurs de tes remarques:
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 package testjava; import java.io.IOException; import java.nio.file.*; import java.nio.charset.*; public class Program { private static final String FICHIER_NOM = "LISEZMOI.TXT"; private static final Charset FICHIER_ENCODAGE = StandardCharsets.UTF_8; private static void lireFichier() throws IOException { int numeroLigne = 0; for( String ligne : Files.readAllLines( Paths.get(FICHIER_NOM), FICHIER_ENCODAGE) ) { System.out.println( ++numeroLigne + " : " + ligne ); } } public static void main(String[] args) { try { lireFichier(); } catch( Exception ex ) { System.out.println( "Une erreur est survenue : " + ex.toString() ); } } }
Notes bien, il s'agissait d'un petit exercice, suite à la lecture de cet article: http://oliviercroisier.developpez.co...ue-simplement/

Article au sujet duquel, j'ai également une question, si tu veux bien y répondre? Voilà, l'auteur parle du Design Pattern "Decorateur" qui (je cite) "permet d'ajouter des comportements (méthodes) à un objet de base, par composition plutôt que par héritage, favorisant ainsi la cohésion et la réutilisabilité".

S'agissant de la réutilisabilité, je trouve que l'héritage, justement, c'est pas mal non plus ;) Et surtout, je ne vois pas où est la composition dans les exemples de l'article? En effet, java.io.LineNumberReader dérive de java.io.BufferedReader qui dérive de java.io.Reader : héritage, donc, mais s'agissant de la composition, je ne vois pas où, je passe à coté :koi: ? (Modification de mon post : je pose la question directement sur le fil de discussion de l'article, où elle a peut-être d'avantage sa place : http://www.developpez.net/forums/d14...ue-simplement/ )

Salut,

Quelques petites remarques :
LineNumberReader est un BufferedReader, donc il est inutile de cumuler les deux. Soit l'un soit l'autre.
Comme l'a dit thelvin autant utiliser Files.newBufferedReader() et une variable pour compter les lignes...

On peut se contenter de fermer le flux du FileInputStream, puisque les autres ne sont pas lié à une ressource.
Mais d'une manière générale je déconseillerais, car cela peut amener des erreurs dans certains cas. Pour éviter toute ambiguité il est préférable de fermer explicitement pour les ressources, même si c'est un peu redondant.
Et l'utilisation du try-with-ressource est parfaite pour cela !
Je n'aime pas trop la version beta avec le try/finally, pour deux raison :
1 - plusieurs close() dans le même finally peuvent poser problème si on a plusieurs ressources.
2 - cela oblige à déclarer les variables à null avant de les initialiser.
:arrow: Sous Java 6 et inférieur (sans try-with-ressource), je préfère largement utiliser un try/finally par ressource, juste après l'initialisation.

Perso j'opterais plutôt pour quelque chose comme cela :
Code:

1 2 3 4 5 6 7 8 9 10 11 12 private static void lireFichier() throws FileNotFoundException, IOException { LineNumberReader lnr = new LineNumberReader(new InputStreamReader( new FileInputStream(FICHIER_NOM), FICHIER_ENCODAGE)); try { String ligne; while (null != (ligne = lnr.readLine())) { System.out.println(lnr.getLineNumber() + " : " + ligne); } } finally { lnr.close(); } }
On peut se permettre d'emboiter les appels car les constructeurs ne génère pas d'I/O et donc d'erreurs.
Tout ce qu'on crains à ce niveau c'est des OutofMemory ou des NullPointer.

Toutefois dans le cas extrême ou on veut gérer ces erreurs à la création d'InputStreamReader et LineNumberReader on peut faire cela :
Code:

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 private static void lireFichier() throws FileNotFoundException, IOException { FileInputStream fis = new FileInputStream(FICHIER_NOM); try { LineNumberReader lnr = new LineNumberReader(new InputStreamReader( fis, FICHIER_ENCODAGE)); try { String ligne; while (null != (ligne = lnr.readLine())) { System.out.println(lnr.getLineNumber() + " : " + ligne); } } finally { lnr.close(); } } finally { fis.close(); } }
Bien sûr à partir de Java 7 on ne réfléchit pas et on opte pour le try-with-ressource.
a++