Discussion :

[tickyletroll]

Dans le logiciel que je maintiens, je me retrouve avec ce morceau code (répété ... trop de fois):

Code :

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

1
2
3
4
5
6
7
8
9
 
if ( args["action"]=="play" && args["filename"]!="") {
   string pathMusic;
   if (args["filename"][0]!='/')
       pathMusic = scriptMgr->get_script_path() + args["filename"];
   else
       pathMusic = args["filename"];
       media->audioMusicLoad(pathMusic);
}

En gros, si args["filename"] != 0 on informe le logiciel qu'on a un nom de fichier. Si ce nom de fichier n'est pas un nom de fichier exprimé de manière absolue, alors on modifie le nom du fichier en rajoutant un chemin indiqué par scriptMgr->get_script_path()

Le code fonctionne pour ce qu'on lui demande mais il n'y a aucun contrôle de l’existence du fichier par exemple !

Je souhaite alors modifier cela et faire disparaître cette horreur.

J'ai crée une classe qui s'occupe de faire mes tests plus proprement.

Code :

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

1
2
3
4
5
6
7
8
9
10
11
class FilePath
{
public:
	FilePath(std::string fileName);
	std::string getExtention();
	std::string getPath();
	std::string toString();
	bool exist();
private:
	...
};

exist() est une fonction qui vérifie vraiment l'existence du fichier sur le disque dur et que son accès est possible.
Enfin le constructeur FilePath fait son travail de vérification des noms du fichier

Mon morceau de code devient alors

Code :

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

1
2
3
4
5
6
7
8
 
if ( args["action"]=="play" && args["filename"]!="") {
   FilePath myFile = FilePath(args["filename"]);
            if (myFile.exist())
                media->audioMusicLoad(myFile.toString());
            else
                debug_message = _("command 'audio': filename not found");
}

Je l'ai mis en place sur un cas et cela fonctionne bien mieux.
Mais je trouve mon code final peu élégant. Aussi je viens vers vous pour me donner des pistes afin de m'améliorer !

Cordialement

[ternel]

Il y a moyen de faire un poil mieux avec les opérateurs de conversion.

Code :

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

1
2
3
4
5
6
if ( args["action"]=="play" && args["filename"]!="") {
    if (FilePath myFile = FilePath(args["filename"]))
        media->audioMusicLoad(myFile);
    else
        debug_message = _("command 'audio': filename not found");
}

Pour faire cela, il suffit d'ajouter deux opérateurs de conversions.

Code :

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

1
2
3
4
5
6
7
8
9
10
11
12
13
14
class FilePath
{
public:
	FilePath(std::string fileName);
	std::string getExtention();
	std::string getPath();
	std::string toString();
	bool exist();
 
	explicit operator bool() const { return exist(); }
	operator std::string() const { return toString(); }
private:
	...
};

Cela dit, le second n'est pas requis si tu crées une surcharge de audioMusicLoad qui prend un FilePath en argument.

Pour faire plus esthétique, il faudrait avoir plus de code, pour revoir éventuellement l'architecture technique.

Tu pourrais envisager une map de foncteurs telle que std::map< std::string, std::function<void, decltype(args)>.
Voire une classe de base

Code :

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

1
2
3
4
5
6
7
8
9
10
11
12
13
14
template <typename Args_type, typename Result = void>
class Action {
public:
    using arguments_type = Args_type;
    using result_type = Result;
 
    Result handle(Args_type const& args) {
        if (!verify(args)) throw action_error( _("command 'audio': filename not found") );
        do_handle(args);
    }
private:
    virtual bool verify(Args_type const& args) const { return true; }
    virtual Result do_handle(Args_type const& args) = 0;
}

[tickyletroll]

Merci !!!

Je pensais pas que l'on pouvait réduire à ce point !

En tout cas, cela représente au final exactement ce que je cherchais !

Merci pour l'idée de la map de foncteurs, je vais regarder cela .

[ternel]

J'ai oublié d'expliquer le pourquoi

On peut déclarer une variable dans le test d'un if, pour peu que son type soit convertible (explicitement) en booléen.
Cette variable est définie pour l'intégralité du test (la condition, le code du "then" et celui du else).

La conversion vers string semble ici être une bonne idée, pour la rendre transparente, mais il faut éviter de le faire.

Attention, la conversion vers bool devrait toujours être explicit, ce qui n'était pas possible avant C++11, et est la cause du "safe bool idiom".
Quand ce n'est pas le cas, le type peut être converti indirectement en nombre.

[tickyletroll]

Je digresse mais ma curiosité bat son plein avec ton indication:

Voici la portion complète de la fonction qui me pose problème

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
 
int AppCommandInterface::command_audio()
{
	status = 1;
	if (args["action"] != "") {
		if (args["action"]=="drop") {
			media->audioMusicDrop();
 
		} else if (args["action"]=="sync") {
			media->audioMusicSync();
 
		} else if (args["action"]=="pause") {
			media->audioMusicPause();
 
		} else if ( args["action"]=="play" && args["filename"]!="") {
 
			string pathMusic;
			if (args["filename"][0]!='/')
				pathMusic = scriptMgr->get_script_path() + args["filename"];
			else
				pathMusic = args["filename"];
			media->audioMusicLoad(pathMusic);
 
			media->audioMusicPlay(args["loop"]=="on");
		} else if ( args["action"]=="load" && args["filename"]!="") {
			string pathMusic = scriptMgr->get_script_path() + args["filename"];
			media->audioMusicLoad(pathMusic);
		} else if (args["action"]=="play") {
			media->audioMusicPlay(false);
		} else if (args["action"]=="resume") {
			media->audioMusicResume();
		} else {
			debug_message = _("command 'audio': unknown action parameter");
			Log.write( debug_message,cLog::ERROR, cLog::SCRIPT );
			status = 0;
		}
		return execute_command_status();
	}
 
	if (args["volume"]!="") {
		if (args["volume"] == "increment") {
			media->audioVolumeIncrement();
		} else if (args["volume"] == "decrement") {
			media->audioVolumeDecrement();
		} else media->audioSetVolume(eval_int(args["volume"]));
		return execute_command_status();
	}
 
	debug_message= _("command 'audio' : unknown argument");
	Log.write( debug_message,cLog::ERROR, cLog::SCRIPT );
	status = 0;
	return execute_command_status();
}

args est un std::map< std::string, std::string >

est ce que je pourrais traiter les args[...] autrement qu'avec la méthode :

Code :

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

if (args["machin"] == "bidule")

ta piste sur la map de foncteur pourrait elle encore m'aider ?

[ternel]

En fait, tu peux difficilement faire beaucoup mieux: le ratio de "signal/bruit" de ton code est proche de une ligne utile par ligne.

Par contre, tu as un bug: si la commande est "play" mais filename est vide, la fonction retourne execute_command_status() alors qu'elle n'a rien fait.

[tickyletroll]

Je vais donc modifier ce code avec tes informations et le rendre plus clair.

Encore merci !

[Bousk]

Ne peux-tu pas utiliser des enum ou entier au lieu de string partout ?
Les string c'est de loin le pire à manipuler pour ce genre de trucs. Avec des valeurs entières (ou enum), tu aurais un élégant switch et gagnerais quelques perfs au passage.

[Bktero]

La conversion vers string semble ici être une bonne idée, pour la rendre transparente, mais il faut éviter de le faire.

Pourquoi ? Pourquoi avec string en particulier ?

[ternel]

Effectivement, une enum serait utile, mais ne change pas grand chose au problème.
La chaine vient d'une ligne de commande ou d'un script, et il faut la relier à l'enum.
Le plus rapide pour le faire sera alors une map, pour ne comparer qu'à au plus log(nombre_commandes), et non N pour une chaine de if / else if.

[dalfab]

Un détail dont il faut de méfier avec les maps, c'est que chaque accès provoque une recherche dans la table, et ici args["action"] est répété de nombreuses fois, on pourrait faire :

Code :

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

1
2
3
4
5
6
7
8
9
int AppCommandInterface::command_audio() {
	status = 1;
        auto&& argAction = args["action"];  // unique recherche effectuée
	if ( !argaction.empty() ) {
		if ( argAction == "drop" ) {
			media->audioMusicDrop();
		} else if ( argAction == "sync" ) {
			media->audioMusicSync();
                ...

[koala01]

Salut,

Pourquoi ? Pourquoi avec string en particulier ?

Pour une raison toute bête: la comparaison de deux chaines de caractères compte parmi les comparaisons qui prennent le plus de temps.

En effet, la comparaison de deux valeur numérique (comme des entiers) se fait en une seule instruction processeur, mais, pour comparer deux chaines de caractères, il faut comparer les valeurs numérique de ... chaque caractère d'une chaine avec le caractère se trouvant à la même position dans l'autre chaine.

Utiliser une std::map<std::string,std::string> va déjà poser des problèmes pour le classement, parce que l'opérateur < agit de la même manière que l'opérateur == (il n'y a que l'instruction processeur qui change), ce qui fait que tu va déjà perdre du temps lors de la recherche de la clé qui t'intéresse.

Mais si, en plus, tu t'amuse à comparer la valeur obtenue grâce à cette clé avec d'autres chaines de caractères, tu augmente encore le temps nécessaire ) l'obtention de la réponse, surtout si ... tu multiplies à loisir les valeurs potentielles pour la valeur.

En remplaçant avantageusement une std::map<std::string, std::string> par une std::map<EnumKey, EnumValue> (où EnumKey et EnumValue seraient des énumérations ou, au pire des constantes numériques), tu peux en arriver à "booster" les performances à un niveau que tu n'aurais jamais oser atteindre

[ternel]

Je suis parfaitement d'accord dans le cas général.
Mais dans le cas présent, chaque valeur chaine en entrée n'est utilisée qu'une seule fois (si la recherche est factorisée comme le suggère dalfab)

Il faut bien à un moment donné chercher la chaine parmi les chaines valides. Que ce soit pour sélectionner la bonne commande à exécuter ou pour trouver la bonne valeur d'enum qui permet de trouver la commande.

Par contre, on y gagne en découplage: on valide d'abord, puis on suit la logique demandée.