Discussion :

[authchir]

Bonjour,
Je viens de terminer une exercice ayant pour but de mettre en place un système de cryptage/décryptage en utilisant le nombre de César.

Mon application fonctionne, mais je voudrais savoir si il y a des problèmes (bogues, problème de style, réinventer la roue, etc.) que je n'ai pas remarqués ou, plus simplement, si il y a des choses que j'aurais du faire carrément autrement. Le but est simplement de m'améliorer et de tenter d'avoir le code le plus robuste possible.

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
#include <iostream>
#include <algorithm>
#include <sstream>
#include <locale>
#include <array>
#include <string>
#include <cassert>
 
std::string encrypt(int shift, std::string word);
std::string decrypt(int shift, std::string word);
 
int main()
{
	int shift = 5;
	std::stringstream sentence("Hello World ABCDEFgHIJKLmNOPqRSTUVWXYZ");
	std::stringstream encrypted_sentence;
	std::stringstream decrypted_sentence;
 
 
	while(!sentence.eof())
	{
		std::string temp;
		sentence >> temp;
		encrypted_sentence << encrypt(shift, temp) << " ";
	}
 
	while(!encrypted_sentence.eof())
	{
		std::string temp;
		encrypted_sentence >> temp;
		decrypted_sentence << decrypt(shift, temp) << " ";
	}
 
	std::cout << sentence.str() << std::endl;
	std::cout << encrypted_sentence.str() << std::endl;
	std::cout << decrypted_sentence.str() << std::endl;
}
 
std::string encrypt(int shift, std::string word)
{
	static const std::array<char, 26> letters = {'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z'};
	if(shift != 0)
	{
		for(std::string::iterator it = word.begin(); it != word.end(); ++it)
		{
			bool was_lower = false;
			if(std::islower(*it))
			{
				was_lower = true;
				*it = std::toupper(*it);
			}
 
			std::array<char, 26>::const_iterator new_character = std::find(letters.begin(), letters.end(), *it);
 
			assert(new_character != letters.end());
 
			if(shift > 0)
			{
				for(int i = 0; i < shift; ++i)
				{
					++new_character;
 
					if(new_character == letters.end())
						new_character = letters.begin();
				}
			}
			else
			{
				for(int i = 0; i > shift; --i)
				{
					if(new_character == letters.begin())
						new_character = letters.end();
 
					--new_character;
				}
			}
 
			if(was_lower)
				*it = std::tolower(*new_character);
			else
				*it = *new_character;
		}
	}
 
	return word;
}
 
std::string decrypt(int shift, std::string word)
{
	return encrypt(-shift, word);
}

Pour ce qui est de l'algorithme que j'utilise, je parcours chaque lettre du mot à crypter et, pour chacune d'entre elles, je recherche la position correspondante dans l'alphabet et va chercher la lettre décalée selon la valeur de décalage reçue en paramètre. Une fois la nouvelle lettre trouvée, je remplace la lettre d'origine et passe au caractère suivant.

PS: Bien que ce ne soit pas vraiment important, puisque je tente de conserver un code multiplatforme, sachez que je compile avec GCC 4.5.2 sous GNU/Linux.

[pyros]

Salut,

C'est très bien de vouloir s'améliorer

J'ai quelques remarque sur la forme, pour le fond, voir si l'algo est bon, rapide, etc... je n'ai pas trop le temps de regarder.

Commentaires:
- Pour une fonction, toujours préciser à quoi servent les paramètres et ce qu'elle fait (même si là c'est évident, c'est toujours bon de prendre les bonnes habitudes dès le début . Tu peux utiliser une syntaxe à la doxygen par exemple:

Code :

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

1
2
3
4
5
6
7
8
 
/**
 * Encrypt la chaine
 * @param shift Decalage a appliquer
 * @param word Chaine a encrypter
 * @return chaine encrypte
 */
std::string encrypt(int shift, std::string word);

- Dans l'algo, préciser les points clef, ce que fait tel ou tel bloc de code, etc... sans paraphraser le code non plus (genre "je met l'alphabet dans la variable letters" ça ne sert pas à grand chose ^^)

Passer les param. par référence plutôt que par copie:

Code :

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

1
2
 
std::string encrypt(int shift, const std::string& word);

Ca évite de recopier la string lors du passage en paramètre. De même, tu peux passer la valeur de retour par référence au lieu de la retourner, cela évite aussi une copir lors du retour de la fonction:

Code :

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

1
2
 
void encrypt(int shift, const std::string& word, std::string& out);

Un peu délicat comme notion au début mais ça viendra.

Sinon les nom de variable sont bien choisi et le code n'est pas trop fouilli.

[oodini]

cryptage/décryptage

-> chiffrage/déchiffrage

Sinon, extrais de tes boucles tes créations de string (lignes 22 & 29).
La création d'un objet coûte cher.

[Joel F]

Passer les param. par référence plutôt que par copie:

Code :

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

1
2
 
std::string encrypt(int shift, const std::string& word);

Ca évite de recopier la string lors du passage en paramètre. De même, tu peux passer la valeur de retour par référence au lieu de la retourner, cela évite aussi une copir lors du retour de la fonction:

Code :

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

1
2
 
void encrypt(int shift, const std::string& word, std::string& out);

Non, via la copy-elision et les mecanismes d'optimisations des valeur de retour, renvoyer une instance est aussi voir plus rapide que de pourrir le prototype avec une reference.

http://cpp-next.com/archive/2009/08/...pass-by-value/

[pyros]

Cool, je connaissais pas cette optim

[authchir]

Hé bien d'abord, merci de vos commentaires.

Pour une fonction, toujours préciser à quoi servent les paramètres et ce qu'elle fait (même si là c'est évident, c'est toujours bon de prendre les bonnes habitudes dès le début . Tu peux utiliser une syntaxe à la doxygen...

Oui, je connais un peu Doxygen et c'est vrai que ça doit être très pratique pour travailler sur un gros programme ou une grosse bibliothèque. Jusqu'ici, je n'avais jamais prit l'habitude d'écrire systématiquement ce genre de commentaires...

Dans l'algo, préciser les points clef, ce que fait tel ou tel bloc de code, etc...

C'est une bonne idée. Pour l'instant, la fonction me parait évidente, mais je l'aurai probablement oublié dans un ou deux mois et cela pourrais éviter d'avoir à relire et recomprendre le code dans son entièreté. Je penserais à quelque chose comme ça:

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
std::string encrypt(int shift, std::string word)
{
	static const std::array<char, 26> letters = {'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z'};
 
	// No job to do if shift is zero
	if(shift != 0)
	{
		// Replace each character by it's corresponding encrypt character
		for(std::string::iterator it = word.begin(); it != word.end(); ++it)
		{
			// We must work on upper case letters
			bool was_lower = false;
			if(std::islower(*it))
			{
				was_lower = true;
				*it = std::toupper(*it);
			}
 
			std::array<char, 26>::const_iterator new_character = std::find(letters.begin(), letters.end(), *it);
 
			assert(new_character != letters.end());
 
			if(shift > 0)
			{
				// Increment until we find the encrypt character
				for(int i = 0; i < shift; ++i)
				{
					++new_character;
 
					if(new_character == letters.end())
						new_character = letters.begin();
				}
			}
			else
			{
				// Decrement until we find the encrypt character
				for(int i = 0; i > shift; --i)
				{
					if(new_character == letters.begin())
						new_character = letters.end();
 
					--new_character;
				}
			}
 
			// Replace the new character to the initial upper/lower case
			if(was_lower)
				*it = std::tolower(*new_character);
			else
				*it = *new_character;
		}
	}
 
	return word;
}

Je remarque que ce sont surtout les boucles qui sont pratique à commenter...

Sinon, extrais de tes boucles tes créations de string (lignes 22 & 29).
La création d'un objet coûte cher.

Mais si l'initialisation est en dehors de la boucle, la variable vivra alors jusqu'à la fin de la fonction. Nous aurons donc à subir le poid de celle-ci alors qu'elle ne sert plus à rien.

J'avoue cependant qu'un std::string ne doit pas peser très lourd. Dans ce genre de cas, est-il plus commun de choisir la vitesse d'exécution (en initialisant la variable à l'extérieur) ou bien l'utilisation de mémoire vive (en initialisant la variable à l'interrieur)?

Pour ce qui est du passage par référence ou par copie, j'ajouterais que je souhaite que la variable de l'utilisateur reste inchangée et que je dois modifier ma copie de la variable pour la chiffrer. C'est pour cette raison que j'avais choisie de la recevoir par copie. Par contre, je suis heureux d'apprendre que les compilateurs sont à même d'optimiser le retour par copie d'une variable reçue par copie.

[PadawanDuDelphi]

Salut,

is si l'initialisation est en dehors de la boucle, la variable vivra alors jusqu'à la fin de la fonction. Nous aurons donc à subir le poid de celle-ci alors qu'elle ne sert plus à rien.

J'avoue cependant qu'un std::string ne doit pas peser très lourd. Dans ce genre de cas, est-il plus commun de choisir la vitesse d'exécution (en initialisant la variable à l'extérieur) ou bien l'utilisation de mémoire vive (en initialisant la variable à l'interrieur)?

Ca dépend de ce que tu as besoin...Dans ton cas l'important c'est la vitesse d'execution, donc il vaut mieux créer tes objets à l'extérieur de tes boucles, à moins d'être vraiment très juste au niveau de la mémoire vive (enfin un string ça fait pas non plus 2Go).

Pour ce qui est du passage par référence ou par copie, j'ajouterais que je souhaite que la variable de l'utilisateur reste inchangée et que je dois modifier ma copie de la variable pour la chiffrer. C'est pour cette raison que j'avais choisie de la recevoir par copie. Par contre, je suis heureux d'apprendre que les compilateurs sont à même d'optimiser le retour par copie d'une variable reçue par copie.

Avec le mot clef const le compilateur s'assurera que ta fonction ne modifiera pas ta variable. En outre c'est plus explicite et plus clair ainsi (on sait que ce paramètre est constant).

[pyros]

Pour ma part, il est très important de mettre un petit commentaire sur les boucles en c++. Autant dans certains langage, elle sont évidentes (dans beaucoup de langage plus haut niveau (C#, python, ruby, etc...) tu peux écrire une boucle "for each letter in alphabet" par exemple), autant en c++ c'est beaucoup moins évident. Et encore moins avec des itérateur (même si les itérateur c'est bien !).
Donc un petit commentaire comme tu l'as fait est très appréciable.

Ensuite, j'ai tendance à traiter les cas particuliers des fonctions au début, pour laisser place au vrai algo. Donc j'écrirais ta fonction encrypt comme ceci:

Code :

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

1
2
3
4
5
6
7
8
9
10
 
std::string encrypt(int shift, std::string word)
{
	static const std::array<char, 26> letters = {'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z'};
 
	// No job to do if shift is zero
	if(shift == 0)
	    return;
 
...

Ca permet de ne pas se trainer l'indentation du if tout le long de la fonction. Mais certaines personne n'aime pas mettre des return au milieu du code (ça le rend en effet un peu moins clair). Donc c'est une question de gout.

Pour finir, déclarer le string dans ou hors de la boucle, c'est commencer à faire de la micro-optimisation (google est ton amis pour plus de précision). Et ça, c'est le mal !!!, surtout lorsqu'on débute. Déclare ta variable là ou tu pense qu'elle vas bien. Le compilo optimisera sûrement tout ça de lui même. Cette remarque s'applique aussi sur passer par référence ou par copie. Si tu ne sais pas quoi faire, fait ce qui te semble le plus clair et le plus propre, et fait confiance au compilo pour faire ce qu'il faut.

[oodini]

Mais si l'initialisation est en dehors de la boucle, la variable vivra alors jusqu'à la fin de la fonction. Nous aurons donc à subir le poid de celle-ci alors qu'elle ne sert plus à rien.

Si cela te pose vraiment un problème, tu peux ajouter un groupe d'accolade :

Code :

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

1
2
3
4
5
6
7
8
9
10
...
{
    std::string pouet;
 
    for (...)
    {
        ...
    }
}
...

J'avoue cependant qu'un std::string ne doit pas peser très lourd. Dans ce genre de cas, est-il plus commun de choisir la vitesse d'exécution (en initialisant la variable à l'extérieur) ou bien l'utilisation de mémoire vive (en initialisant la variable à l'intérieur) ?

Il est très rare que la mémoire vive soit à ce point prioritaire par rapport à la vitesse d'exécution (compte tenu de la place occupée par le string qui nous occupe ici).
Et souviens-toi : toujours éviter la construction inutile d'objet. Cela occasionne appel de fonction et création de membres. Multiplié par le nombre d’exécution de la boucle.

D'autant plus que dès que tu sors de la boucle, il faut tout refaire dans le sens inverse...

[authchir]

Hé bien merci pour vos réponses, je vais partir à la recherche d'autres exercices.