Bonjour , voila que j'ai lu les 12 premiers chapitres de c++ primer édition

j'ai décidé de faire un de leur exercices p484 qui consiste a créer un programme qui compte le nombre d’occurrence d'un mot dans un ficher et qui affiche les lignes ou il y a ce mot , j'ai décidé de faire cet exercice tout seul (même si dans le livre on nous présente une solution)

voici mon code , je voudrais savoir si j'ai pas fait (trop) d'erreurs , si il est "propre" , et éventuellement avoir quelques conseils

Je vous remercie d'avance

main.cpp :

#include <iostream>
#include <string>
#include <fstream>

#include "textfinder.h"
#include "results.h"

TextFinder init() {
    std::string file_tmp,word_tmp;
    std::cout << "please enter a file (.txt)" << std::endl;
    while (!(std::cin >> file_tmp)) ;
    std::cout << "please enter a word" << std::endl;
    while (!(std::cin >> word_tmp)) ;
    std::cout << std::endl << std::endl;
    return TextFinder(file_tmp,word_tmp);
}

int main() {
    std::string quit;
    while (quit != ":q") {
        TextFinder f = init();
        Results r = f.process();
        f.print(std::cout,r);
        std::cout << "press a key for continue / :q for quit";
        std::cin >> quit;
    }
    return 0;
}

textfinder.h :

#ifndef TEXTFINDER_H
#define TEXTFINDER_H

#include <vector>
#include <string>
#include <fstream>
#include <iostream>
#include <sstream>

#include "results.h"

struct Results;

class TextFinder
{
public:
    TextFinder(const std::string &is,const std::string &str = "");
    void reload();
    void setWord(const std::string &str);
    Results process();
    void print(std::ostream &os,const Results &res);
private:
    std::string word;
    std::string path;
    std::vector<std::string> lines;
    Results resultats;
};

#endif // TEXTFINDER_H

textfinder.cpp

#include "textfinder.h"

size_t occurence(const std::string &mot,std::string &line) {
    std::istringstream iss(line);
    std::string tmp;
    size_t nb = 0;
    while (iss >> tmp) {
        if (tmp == mot) { nb++; }
    }
    return nb;
}

TextFinder::TextFinder(const std::string &is,const std::string &str): path(is) {
    this->setWord(str);
    reload();
}

void TextFinder::reload() {
    std::string line;
    std::ifstream file(path);
    try {
        if (!file.is_open()) {
            throw std::runtime_error("can't open the file");
        }
    } catch (std::exception e) {
        std::cerr << e.what() << std::endl;
    }

    while(getline(file,line)) {
        lines.push_back(line);
    }
    file.close();
}

void TextFinder::setWord(const std::string &str) {
    this->word = str;
}

Results TextFinder::process() {
    std::string word = this->word;
    Results tmp;
    size_t occurences = 0;
    size_t pos {0};
    for (auto i = lines.begin();i != lines.end();i++) {
        std::string mot = *i;
        if (size_t o = occurence(word,mot)) {
            tmp.lines.insert(pos);
            occurences += o;
        }
        pos++;
    }
    tmp.times = occurences;
    tmp.word = this->word;
    return tmp;
}

void TextFinder::print(std::ostream &os,const Results &res) {
    os << "the word : " << res.word << " appear " << res.times << " times" << std::endl;
    for (auto i = res.lines.begin();i != res.lines.end();i++) {
        os << "(line :" << *i << ") -> " << this->lines.at(*i) << std::endl;
    }
}

results.h :

#ifndef RESULTS_H
#define RESULTS_H

#include <set>
#include <string>

struct Results {
    size_t times;
    std::string word;
    std::set<size_t> lines;
};

#endif // RESULTS_H

-
Edité par Anonyme 12 octobre 2018 à 22:11:19

C'est pas mal du tout dans l'ensemble. Je n'ai pas regardé la logique complète, juste survolé des constructs

a- Ces lignes sont particulièrement obfusquées ,  `while (!(std::cin >> file_tmp)) ;`tu veux faire quoi ? En plus, si cin est vraiment KO, ou fermé, tu ne pourras plus rien faire dessus -- surtout s'il est fermé. Cf. FAQ pour la validation des entrées.

b- Tu inclus beaucoup de choses qui ne sont pas nécessaires dans tes .h

c- Les fichiers sont implicitement fermés en C++

d- En général, on se contente de `if (fichier)` pour vérifier s'il a bien été ouvert après sa construction.

e- Dans `for (auto i = lines.begin();i != lines.end();i++)`, autant utiliser un for range loop: `for (auto && line : lines)`

-
Edité par lmghs 13 octobre 2018 à 10:09:22

Salut,

Trois petites choses :

Pourquoi une fonction membre reload() dans ta classe TextFinder, alors que tu ne l'utilise pas ?

La règle est en effet que, si tu n'as pas besoin de "quelque chose", il n'y a aucune raison de l'implémenter, parce que

1. Cela complexifie ton projet "inutilement"
2. C'est une source d'erreurs (à l'exécution) qui ne seront pas détectable immédiatement, vu que tu ne t'en sers pas
3. c'est risquer que quelqu'un "d'un peu distrait" fournisse la même chose sous un autre nom (pourquoi pas "rescan"?) quand il en aura besoin

En l'occurrence, le (2) s'avère en deux points, qui sont les deux autres remarques que je voulais faire

La deuxième remarque est que tu rajoutes dans reload, les chaines line que tu trouves dans ton tableau lines.  Mais, si l'utilisateur a déjà utilisé reload ou process, lines contient déjà "quelque chose", qui correspond d'ailleurs potentiellement à ... ce que tu vas y ajouter durant l'exécution de reload.

Selon l'ordre dans lequel l'utilisateur de ta classe fera appel aux différentes fonctions, tu le placeras donc dans une situation dans laquelle ... il pourra l'utiliser de manière incorrecte (d'autant plus que "reload" signifie recharger, et implique donc qu'il s'agit de charger quelque chose qui ... l'a déjà été une première fois :P).

Or, le conseil de scott meyers est sans doute l'un des plus important à respecter :

Make interfaces easy to use correctly and hard to use incorrectly.

(Rendez vos interfaces facile à utiliser correctement et difficiles à utiliser de manière incorrecte)

Il y a là une erreur de logique que tu devrais corriger

Plus embêtant encore (ce sera ma troisième remarques) : Tu décides -- de manière très judicieuse -- de lancer une exception si tu n'arrives pas à ouvrir le fichier indiqué.

C'est une excellente chose, car, si l'application ne peut pas ouvrir le fichier en question, elle va se trouver face à l'impossibilité de fournir un résultat cohérent à l'utilisateur de l'application.

A ceci près que tu lances cette exception dans un bloc try ... catch qui la récupérera immédiatement, et qui ne la relance pas.

C'est déjà discutable en soi, car l'idéal est qu'une exception à laquelle aucune solution n'a pu être trouvée devrait ... provoquer l'abandon du programme.

Or, on ne peut pas dire que tu as trouvé une solution dans le bloc catch, vu que, tout ce que tu fais, c'est afficher le problème qui est survenu et qu'apporter une solution à un problème, cela implique de faire qu'il ne se reproduira plus.

Mais, surtout, si le bloc catch ne renvoie pas l'exception, il va considérer que le problème est résolu, et permettre l'exécution de "tout ce qui suit", exactement "comme s'il n'y avait pas de problème".

Pas de bol, ce qui suit le bloc catch, c'est la boucle

while(getline(file, line)){
    // ...
}

autrement dit : "tant que tu peux extraire une ligne du flux file"  Sauf que, si l'exception a été lancée (et soit disant "traitée" par le bloc catch), tu va avoir un problème, car ... file est dans un état invalide.

Une chance d'ailleurs, car la première exécution de la boucle correspondra à while(false) !  Mais, après, tu as

file.close();

Et là, il faudra quand même m'expliquer comment tu espère pouvoir fermer un fichier qui ... n'a jamais pu être ouvert

Merci pour vos réponses , j'ai tout corrigé :

pour la méthode reload , je l'ai changé de nom elle s’appelle maintenant loadFile , je pense que le nom est explicite x) , l'idée c'est de charger le ficher et de lettre toutes les lignes dans un vector , pour après pourvoir utiliser process() , (ça permet de changer de fichier sans devoir instancier une 2eme fois la classe)

la fonction reload comme ca je suis d'accord elle sert a rien j'ai d'ailleurs oublié de l'enlever

Je voulais séparer la lecture du fichier et les résultats dans 2 méthodes différentes .

J'ai ajouté un "line.clear()" dans cette même méthode , car comme tu me l'a fais remarquer ""lines" contient déjà quelque chose" , si on utilisait 2 fois la classe il y aurai des résultats en double merci

Quant aux exceptions je sais pas super bien m'en servir , je l'ai donc remplacé par une condition , si elle n'est pas respecté il y a un message d'erreur et la fonction s’arrête (le .close() n'est donc pas exécuté)

Voici mes modifications :

loadFile:

void TextFinder::loadFile(const std::string &path_tmp) {
    lines.clear();
    std::string line;
    path = path_tmp;
    std::ifstream file(path_tmp);
    if (!file.is_open()) {
        std::cerr << "can't open the file" << std::endl;
        return;
    }
    while(getline(file,line)) {
        lines.push_back(line);
    }
    file.close();
}

Pour finir j'ai viré les while dans la fonction init() (ils ne servent a rien) , je voulais juste que l'utilisateur tape le nom de son ficher et son mot a rechercher

init()

std::string file_tmp,word_tmp;
std::cout << "please enter a file (.txt)" << std::endl;
std::cin >> file_tmp;
std::cout << "please enter a word" << std::endl;
std::cin >> word_tmp;
std::cout << std::endl;
return TextFinder(file_tmp,word_tmp);

J'ai également corrigé tout le reste (un header en trop qui sert a rien , un for range loop plus qu'avec itérateurs etc...)

merci a vous , des autres soucis ?

-
Edité par Anonyme 13 octobre 2018 à 14:40:28

Hafnium a écrit:

Quant aux exceptions je sais pas super bien m'en servir , je l'ai donc remplacé par une condition , si elle n'est pas respecté il y a un message d'erreur et la fonction s’arrête (le .close() n'est donc pas exécuté)

C'est une idée, mais cela ne résoud qu'une partie du problème, de la manière dont tu présente les choses...

Car toi et moi avons beau être de "petits privilégiés" (comparé à quelqu'un qui n'aurait que ton fichier d'en-tête) et savoir que la fonction loadFile peut s'arrêter "sans avoir rien fait" (si le fichier ne peut pas être ouvert ca parrait normal ), cela ne nous permet pas de réagir en conséquence au niveau de la fonction appelante.

Cela nous met -- finalement -- dans la même situation que le "pauvre malheureux" qui n'aurait que le fichier d'en-tête pour se faire une idée du fonctionnement de ta classe:

En effet, si tu décides de fournir ta classe dans une bibliothèque, celle-ci sera compilée, et tous les utilisateurs de ta classe n'auront que ... le fichier d'en-tête pour les aider à l'utiliser correctement.

Tout ce qu'il sauront de ta classe, c'est qu'il existe une fonction loadFile qui prend le chemin vers un fichier en paramètre et qui ne renvoie rien.  Ils n'ont donc aucune raison d'imaginer que l'exécution puisse se poursuivre si cette fonction venait à "rencontrer un problème".

Si bien que, pour eux, un code proche de

int main(){
    // OUPPSS... c'est "fichier.txt", sans "s"!!!!
    std::string filename{"/home/<user_name>/fichiers.txt"};
    TextFinder f(filename, "truc");
    // WTF??? pourquoi devoir fournir deux fois le nom du 
    // fichier
    f.loadFile(filename);
    f.print(std::cout); // ah, ben oui, j'ai chargé le fichier,
                        // j'ai rien récupéré, donc, je
                        // part du principe que f a "tout 
                        // le nécessaire" 
    return 0;
}

fournira une valeur différente de 0, vu qu'ils savent pertinemment que "truc" apparaît au moins dix fois dans le fichier.

Et ils auront beau voir l'affichage de l'erreur (du moins, si cerr n'est pas redirigé vers un fichier de log ), ils ne sauront pas comment résoudre le problème.

Car la cause réelle du problème qui les différentes fonctions de la classe TextFinder ne peuvent pas ouvrir le fichier "/home/<user_name>/fichiers.txt", parce qu'il n'existe pas : il existe uniquement un fichier "/home/<user_name>/fichier.txt" (remarque l'absence du s avant l'extension!!!) ne trouve pas son origine au niveau du code des différentes fonctions de ta classe, mais ... au niveau de la fonction appelante (la fonction main, en l'occurrence)!

Or, c'est ce qui se passe dans la majorité des cas où une fonction n'arrive pas à accéder à une ressource "externe à l'application": les informations fournies par la fonction appelantes (ou par une des fonctions qui a fait appel à la fonction appelante) sont incorrectes.

Mais, du coup, la véritable solution du problème, celle qui pourra corriger le problème et s'assurer qu'il ne se reproduira plus, ne peut -- éventuellement -- se trouver qu'au niveau ... de la fonction qui a fourni les informations incorrectes.

C'est là tout l'attrait des exceptions : on met fin au "cours normal des choses" (au flot d'exécution "normal"), et on remonte les différentes fonctions qui ont mené à celle qui "pose problème" dans l'espoir de trouver une fonction capable de résoudre le problème.

Et, s'il n'y a pas de solution prévue au problème, hé ben, la meilleure des choses à faire est encore ... de laisser "planter" l'application, car nous ne pourrons pas la remettre dans un "état cohérent".

Si bien qu'il est particulièrement rare que le bloc try... catch se trouve dans la fonction qui lancera l'exception.  Et ce, pour une raison toute simple : ce n'est pas à ce niveau là que nous pourrons trouver la solution.

L'idée de lancer une exception si le fichier ne peut pas être ouvert est donc excellente!  C'est l'idée de la récupérer tout de suite (alors que l'on n'a aucun moyen de la corriger) qui pose problème

Nous pourrions même aller plus loin, en créant notre propre exception, sous une forme proche de

#include <stdexcept>
#include <string>
class FileNotFound : public std::runtime_error{
	static const char * combine(std::string const & filename){
		static std::string str{"Unable to open "};
		str.append(filename);
		return str.data();
	}
public:
	explicit FileNotFound(std::string const & filename):
		std::runtime_error(combine(filename)){
			
	}
};

et que nous pourrions utiliser, dans loadFile, sous une forme proche de

void TextFinder::loadFile(const std::string &path_tmp) {
    std::ifstream ifs(path_tmp);
    if(! ifs)
        throw FileNotFound(path_tmp);
    /* si on arrive ici, c'est que "tout va bien" */
    /* ... */
}

et nous pourrions donc prévoir de gérer cette exception bien spécifique dans la fonction main sous une forme proche de

int main(){
    bool again{true};
    while(again){
        try{
            auto filename = askFilename();
            auto toFind = askMotif();
            TextFinder f;
            f.loadFile(filename, toFind); // lance l'exception
                                          // si fichier pas ouvert
            /* A partir d'ici, on peut garantir que loadFile
             * a fonctionné "correctement"
             */
            // ...
        } catch(File notFound & e){
            printError(e.what());
            again = askNewTry();
            if(! again){ // si l'utilisateur décide  de ne pas 
                         // recommencer, on ne peut pas apporter
                         // de solution au problème.
                         // on n'a donc pas d'autre choix que de...
                         // relancer l'exception 
                throw e;
            }
        }
    }
    return 0;
}

Bien sur, std::string askFilename(), std::string askMotif(), void printError(const char *)  et bool askNewTry() sont autant de fonctions que tu devras créer par toi-même, et il faudra adapter loadFile pour qu'elle accepte en deuxième paramètre le "motif" qui doit être recherché.

Mais tu pourra constater que je me suis donné "toutes les chances possibles" d'arriver à résoudre le problème et que, si cela ne suffit pas, hé bien, je n'ai pas d'autre choix que de laisser advenir ce qui doit advenir du fait de l'exception

Si tu es dans une approche service, un reload ne sert quasiment à rien, je plante ma classe pour une seule utilisation, parce que ça va probablement me coûter plus cher de recycler celle que j'ai déjà que d'en faire une neuve. Conclusion, je bouzille celle que j'ai, et j'en fais une neuve C'est plus simple, je n'ai pas à gérer les effets de bord à la con  qui pourraient être induits par le changement de configuration.

-
Edité par int21h 13 octobre 2018 à 21:27:52

je vous remercie beaucoup pour ces explications très détaillées (j'avoue que ca m'a pris un peu de temps a comprendre ^^)

je modifirai éventuellement mon programme quand j'aurai un peu de temps libre

-
Edité par Anonyme 14 octobre 2018 à 20:41:01