Discussion :

[Portus]

Bonjour,

Je viens de terminer un projet, il fonctionne, mais je sais qu'il y a probablement beaucoup d'améliorations possibles au niveau de la conception du code.

Le programme sert à calculer le temps nécessaire pour arriver sur un village ennemi dans un jeu. Il est possible d'ajouter des mondes et d'ajouter des villages. Il est également possible d'exporter cela pour copier le BBCode sur le forum du jeu.

Le code est à cette adresse : http://www.fatal-destiny.com/tribaltime.zip

Auriez-vous quelques recommandations/conseils à me donner ?

Merci d'avance !

[3DArchi]

Salut,

J'ai survolé et je crois qu'il y a d'abord la conception à remanier.

ConfigMonde et Village peuvent se réduire à des structures. Leur constructeur par défaut ne sert à rien.

TribalTime a trop de responsabilité => à refactoriser pour bien séparer les différentes responsabilités. J'ai l'impression qu'elle regroupe à la fois les responsabilités d'affichage, de contrôle et de modèle (cf Modèle/Vue/Contrôleur par exemple)

Pourquoi utiliser des float ?

Tu utilises à plusieurs reprises des int pour gérer du 1/0 => bool.

J'aime pas (et ce n'est pas MISRA par exemple) les définitions multiples sur la même ligne :

Code :

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

1
2
        float vitMonde = ui->vitesseMondeAjout->text().toFloat(),
            vitUnite = ui->vitesseUniteAjout->text().toFloat();

=>

Code :

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

1
2
        float vitMonde = ui->vitesseMondeAjout->text().toFloat();
            float vitUnite = ui->vitesseUniteAjout->text().toFloat();

Il y a des constantes 'magiques' : exportation->setGeometry(20, 20, 360, 160); par exemple. Pourquoi 20,20,360,160 ? Ca représente quoi ? Il y a-t-il un rapport entre elles ?

Je n'aime pas les tests à rallonge. Exemples :

Code :

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

1
2
3
if(ui->xVillageCible->text().toInt() > 0 && ui->yVillageCible->text().toInt() > 0 && ui->xVillagePersonnel->text().toInt() > 0 && ui->yVillagePersonnel->text().toInt() > 0)
if(numMonde > 0 && vitMonde > 0 && vitUnite > 0)
if(nomVillage.length() > 0 && xVillage > 0 && xVillage <= 1000 && yVillage > 0 && yVillage <= 1000 && (groupeVillage == 0 || groupeVillage == 1))

On ne sait pas dire quel est l'objectif du test.


Je me méfie des constructions : while(std::getline(listeMondes, ligneFichier) && stop == 0). En général la seconde condition de sorties est soit parce que le parcours ne devait pas être complet soit une gestion cachée d'erreur. Voir break (ou return si la responsabilité a été isolée dans une fonction dédiée) dans le premier cas, et ne pas avoir peur des exceptions.

Ou quelque chose m'échappe, ou :

Code :

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

1
2
3
4
5
6
7
8
 
                for(my_tok::const_iterator i = tok.begin(); i != tok.end(); ++i)
                {
                    if(a == 0)
                        element = QString::fromStdString(*i).toInt();
 
                    a++;
                }

doit pouvoir se réduire plus clairement à

Code :

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

1
2
3
4
5
6
 
                my_tok::const_iterator i = tok.begin()
                if(i != tok.end())
                {
                        element = QString::fromStdString(*i).toInt();
                }

Je pense qu'en construisant un itérateur sur la lecture des enregistrements du fichier permettrait d'écrire une partie des fonctions à l'aide des algos de la STL de façon plus expressives (tout les while(std::getline(listeVillages, ligneFichier) && stop == 0))

Tu dois pouvoir utiliser des tuples pour éviter les boucles avec la variable a sur les chaînes découpées en token.

Code :

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

QDateTime heureArrivee = heureDepart.addSecs(-(ceil(60*(tempsUnites[ui->selectionUniteLente->currentIndex()]/(this->vitesseMonde*this->vitesseUnite))*nbCases)+delais));

... Une expression comme ça, je ne sais pas dire si la formule est valide, si les variables utilisées dans la formule sont les bonnes, si les types sont bons.

ui devrait être géré dans un unique_ptr.

unites et tempsUnites sont des données statiques, connues à la compilation et reconstruites à chaque fois. C'est dommage.

Tu n'as pas besoin de this-> pour accéder aux variables membres.

Voilà pour une première volée de remarques. Je pense qu'il y en a encore d'autres.

Sinon, les noms semblent bien appropriés. Ca facilite grandement la lecture et ça aide à comprendre l'objectif de la variable/fonction/classe/etc..

[Portus]

Merci beaucoup pour tes explications !

J'ai quelques questions cependant :

- Pourquoi mettre des double à la place des float ? Un float ne prend-t-il pas moins de mémoire qu'un double ? Je sais qu'ils sont moins précis, mais je ne sais pas à quel point ?

-

Code :

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

1
2
std::vector<int> tempsUnites; //temps par unité en secondes
    tempsUnites.push_back(35); //noble

Il faudrait que je déclare le vecteur dans tribalTime.h, mais est-ce que je peux le remplir également dans ce fichier ? Au passage, n'est-il pas possible de remplir mon vecteur en une fois au lieu d'utiliser à chaque fois push_back() ?

- Pour ce qui est de l'unique_ptr je ne comprends pas trop. Si j'ai bien compris, c'est un peu comme auto_ptr ou shared_ptr, et le but serait de libérer correctement la mémoire. Je ne sais pas si c'est correct ?

-

Code :

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

1
2
3
4
5
my_tok::const_iterator i = tok.begin();
  if(i != tok.end())
  {
      element = QString::fromStdString(*i).toInt();
  }

Dans la mesure où je suis certain d'avoir toujours au moins une valeur pour i, la condition est-elle utile ?

- Pour ce qui est des tuples, je suppose que tu parles de ceux de Boost.

- Pour ce qui est du MVC, je devrais, par exemple, regrouper les fonctions comme "ajouterMonde, etc." dans une classe modèle, "exporter, aPropos, etc." dans une classe vue, et pour le contrôleur je ne vois pas de trop.

Encore merci !

[Niark13]

Merci beaucoup pour tes explications !

J'ai quelques questions cependant :

- Pourquoi mettre des double à la place des float ? Un float ne prend-t-il pas moins de mémoire qu'un double ? Je sais qu'ils sont moins précis, mais je ne sais pas à quel point ?

Double est le type par défaut pour les calculs flottants. Float ne devrait être utilisé que pour des cas très particuliers (comme le stockage de nombreuses données).

-

Code :

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

1
2
std::vector<int> tempsUnites; //temps par unité en secondes
    tempsUnites.push_back(35); //noble

Il faudrait que je déclare le vecteur dans tribalTime.h, mais est-ce que je peux le remplir également dans ce fichier ? Au passage, n'est-il pas possible de remplir mon vecteur en une fois au lieu d'utiliser à chaque fois push_back() ?

Tu ne devrais pas mettre de code dans les headers, seulement des déclarations de types et de fonctions, autrement, tu va exécuter le code à chaque fois que ton header sera inclu par un autre fichier, ce qui est beurk.

- Pour ce qui est de l'unique_ptr je ne comprends pas trop. Si j'ai bien compris, c'est un peu comme auto_ptr ou shared_ptr, et le but serait de libérer correctement la mémoire. Je ne sais pas si c'est correct ?

unique_ptr est une nouveauté de C++11. C'est en gros un boot::scoped_ptr qui peut être déplacé (il possède un constructeur de copie et un opérateur d'affectation qui prennent en paramètre une rvalue reference). Il est à préférer à shared_ptr quand une ressource a un seul propriétaire.
<mode jedi>Du dois oublier auto_ptr, il n'a jamais existé. D'ailleurs, ce ne sont pas ces droides que tu recherches *mouvement de la main*.</mode jedi>

- Pour ce qui est des tuples, je suppose que tu parles de ceux de Boost.

Ou de ceux de c++11...

[Portus]

Ok, merci !

Tu ne devrais pas mettre de code dans les headers, seulement des déclarations de types et de fonctions, autrement, tu va exécuter le code à chaque fois que ton header sera inclu par un autre fichier, ce qui est beurk.

Donc, je déclare mon vecteur dans le .h et je fais les push_back() dans le constructeur par exemple ?

Pour ce qui est des tuple, et tout ce qui touche à c++11, il faut que je change la version de gcc avec laquelle QtCreator compile apparemment :s

[Niark13]

Pour QtCreator et C++11, tu peux rajouter la ligne

Code :

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

QMAKE_CXXFLAGS += -std=c++0x

à ton fichier pro pour passer en mode C++11 (testé avec GCC 4.6 et QtCreator 2.4.1).

Pour ton vector, il faut faire des push_back dans le constructeur ou utiliser une liste d'initialisation si tu es en C++11.

[Portus]

C'est ce que je cherchais, merci !

Oui, je viens de voir pour les listes d'initialisation, vraiment pratique.

[Portus]

Etant donné que j'utilise directement les données (dans le code suivant, de la fonction getListeMondes()), j'ai choisi une autre solution que les tuples (voir ci-dessous).

J'espère qu'elle est aussi valable.

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
 
for(my_tok::const_iterator i = tok.begin(); i != tok.end(); ++i)
{
    if(a == 0)
    {
        QString element;
        element = QString::fromStdString(*i);
        ui->listeMondes->addItem(element);
    }
    if(a == 1)
    {
        double element;
        element = QString::fromStdString(*i).toDouble();
        vitessesMonde.push_back(element);
    }
    if(a == 2)
    {
        double element;
        element = QString::fromStdString(*i).toDouble();
        vitessesUnite.push_back(element);
    }
    a++;
}

Code que j'ai remplacé par :

Code :

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

1
2
3
4
my_tok::const_iterator i = tok.begin();
ui->listeMondes->addItem(QString::fromStdString(*(i++)));
vitessesMonde.push_back(QString::fromStdString(*(i++)).toDouble());
vitessesUnite.push_back(QString::fromStdString(*(i++)).toDouble());

Merci !