Free Automatons with State and Transitions : soumission d'un projet

**Kaamui** · 09/03/2012, 15h13

Bonjour,

Je viens ici pour vous présenter un code que j'ai fait, et qui va beaucoup me servir sous peu. C'est une classe d'automates générique. Les choix de conceptions sont surement à discuter (comme toujours), et je veux volontiers en discuter, mais il faudra des arguments forts pour que je remanie la partie conceptuelle (bon commencé comme ça, je vais avoir beaucoup de retour...). Entre parenthèses, j'ai pour projet ensuite de m'en servir pour proposer librement des automates de gestion multi-écrans, IA, météo, animations, etc dans les jeux vidéos 2D, d'où le nom.

Ce qui m'intéresse surtout, ce sont les remarques sur la propreté du code, sur les choses que j'aurais pas du faire (comme les _ en début de donnée membre (3DArchi me l'a rappelé déjà), ce que je peux encore faire pour améliorer les performences, pour améliorer la sécurité d'utilisation (je suppose que mon code n'est pas du tout exception-safe, mais je maitrise assez mal le sujet), etc...

je veux rendre ce projet irréprochable. Je compte sur vous pour me dire tout ce qui ne va pas

le tout est en pièce jointe, mais pour les moins courageux, je montre le main que j'ai joint pour tester la bête

Le main n'est là que pour illustrer

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
 
#include <iostream>
#include "Automaton.hpp"
#include <map>
 
using namespace std;
using namespace FAST;
 
typedef State<string> SInt;
typedef Transition<string> TInt;
 
 
int main()
{
 
    Automaton<string> Automate1("Automate 1");
 
    Automate1.NewState(new SInt("Etat Initial", initialState));
    Automate1.NewState(new SInt("Etat 2",       intermediaryState));
    Automate1.NewState(new SInt("Etat 3",       intermediaryState));
    Automate1.NewState(new SInt("Etat 4",       intermediaryState));
    Automate1.NewState(new SInt("Etat 5",       finalState));
 
    Automate1.NewTransitionForState("Etat Initial", new TInt("touche haut","transition vers l'etat numero 2"),   Automate1.GetStateByName("Etat 2"));
    Automate1.NewTransitionForState("Etat Initial", new TInt("touche bas","transition vers l'etat numero 3"),    Automate1.GetStateByName("Etat 3"));
    Automate1.NewTransitionForState("Etat 2",       new TInt("touche gauche","transition vers l'etat numero 4"), Automate1.GetStateByName("Etat 4"));
    Automate1.NewTransitionForState("Etat 3",       new TInt("touche droite","transition vers l'etat numero 5"), Automate1.GetStateByName("Etat 5"));
    Automate1.NewTransitionForState("Etat 4",       new TInt("touche gauche","transition vers l'etat numero 5"), Automate1.GetStateByName("Etat 5"));
    Automate1.NewTransitionForState("Etat 4",       new TInt("touche bas","transition vers l'etat numero 1"),    Automate1.GetStateByName("Etat Initial"));
 
    int compteur = 0;
    cout << "->";
    while(Automate1.GetCurrentState()->GetType() != finalState)
    {
        cout << Automate1.GetCurrentState()->GetName() <<"(";
        const std::string& currentStateName = std::string(Automate1.GetCurrentState()->GetName());
        if (currentStateName == "Etat Initial")
        {
            compteur++;
            if (compteur==10)
            {
                cout << Automate1.GetCurrentState()->GetTransitionByName("transition vers l'etat numero 3")->GetValue();
                Automate1.UpdateState(Automate1.GetCurrentState()->GetTransitionByName("transition vers l'etat numero 3"));
            }
            else
            {
                cout << Automate1.GetCurrentState()->GetTransitionByName("transition vers l'etat numero 2")->GetValue();
                Automate1.UpdateState(Automate1.GetCurrentState()->GetTransitionByName("transition vers l'etat numero 2"));
            }
        }
        else if (currentStateName == "Etat 2")
        {
            cout << Automate1.GetCurrentState()->GetTransitionByName("transition vers l'etat numero 4")->GetValue();
            Automate1.UpdateState(Automate1.GetCurrentState()->GetTransitionByName("transition vers l'etat numero 4"));
        }
        else if (currentStateName == "Etat 3")
        {
            cout << Automate1.GetCurrentState()->GetTransitionByName("transition vers l'etat numero 5")->GetValue();
            Automate1.UpdateState(Automate1.GetCurrentState()->GetTransitionByName("transition vers l'etat numero 5"));
        }
        else if (currentStateName == "Etat 4")
        {
            cout << Automate1.GetCurrentState()->GetTransitionByName("transition vers l'etat numero 1")->GetValue();
            Automate1.UpdateState(Automate1.GetCurrentState()->GetTransitionByName("transition vers l'etat numero 1"));
        }
        else
            cout << "Etat non reconnu" << endl;
 
            cout << ") ->";
    }
    cout << Automate1.GetCurrentState()->GetName() << endl;
 
    return 0;
}

Pour les perfs, je précise que je suis en mode debug, donc ça peut être beaucoup mieux en release je pense (mais je ne constate aucun changement personnellement, à part la taille de l'éxécutable) : le main fait passé un automate dans 30 états en 29ms (chez moi) et 47ms (au boulot)

je pose un bout de code plus concret pour ceux qui n'ont pas le temps :

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
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
 
#ifndef AUTOMATON_HPP
#define AUTOMATON_HPP
 
#include <vector>
#include "State.hpp"
 
 
namespace FAST
{
 
 
 
template<typename T>
class Automaton
{
    public :
        typedef std::vector<State<T>*> T_statesVector;
 
        Automaton(const std::string& name_, const T_statesVector& statesVector_ = std::vector<State<T>*>());
        ~Automaton();
 
        void NewState(State<T>* state_);
 
        const State<T>* GetCurrentState() const;
        State<T>* GetCurrentState();
        void UpdateState(Transition<T>* transition_);
 
        void NewTransitionForCurrentState(Transition<T>* transition_, State<T>* stateReached_);
        void NewTransitionForState(const std::string& name_, Transition<T>* transition_, State<T>* stateReached_);
 
        const State<T>* GetStateByName(const std::string& name_) const;
        State<T>* GetStateByName(const std::string& name_);
 
 
        const std::string& GetName() const;
        std::string& GetName();
        void SetName(const std::string& name_);
 
    private :
        T_statesVector _statesVector;
        std::string _name;
        State<T>* _currentState;
 
};
 
template<typename T>
Automaton<T>::Automaton(const std::string& name_, const T_statesVector& statesVector_) :
_statesVector(statesVector_),
_name(name_),
_currentState(NULL)
{
 
}
 
template<typename T>
Automaton<T>::~Automaton()
{
 
}
 
template<typename T>
void Automaton<T>::NewState(State<T>* state_)
{
    _statesVector.push_back(state_);
    if (_currentState == NULL)
        _currentState = state_;
}
 
template<typename T>
const State<T>* Automaton<T>::GetCurrentState() const
{
    return _currentState;
}
 
template<typename T>
State<T>* Automaton<T>::GetCurrentState()
{
    return _currentState;
}
 
template<typename T>
const State<T>* Automaton<T>::GetStateByName(const std::string& name_) const
{
    typename T_statesVector::iterator it;
    for(it=_statesVector.begin(); it=! _statesVector.end(); ++it)
    {
        if ((*it)->GetName() == name_)
            return it;
    }
    return NULL;
}
 
template<typename T>
State<T>* Automaton<T>::GetStateByName(const std::string& name_)
{
    typename T_statesVector::iterator it;
    for(it=_statesVector.begin(); it!= _statesVector.end(); ++it)
    {
        if ((*it)->GetName() == name_)
            return *it;
    }
    return NULL;
}
 
template<typename T>
void Automaton<T>::UpdateState(Transition<T>* transition_)
{
    if (_currentState->GetTransitionByName(transition_->GetName()) != NULL)
        _currentState = _currentState->GetStateReachedByTransition(transition_->GetName());
}
 
template<typename T>
void Automaton<T>::NewTransitionForCurrentState(Transition<T>* transition_, State<T>* stateReached_)
{
    _currentState.NewTransition(transition_, stateReached_);
}
 
template<typename T>
void Automaton<T>::NewTransitionForState(const std::string& name_, Transition<T>* transition_, State<T>* stateReached_)
{
    typename T_statesVector::iterator it;
    for(it=_statesVector.begin(); it!= _statesVector.end(); ++it)
    {
        if ((*it)->GetName() == name_)
            (*it)->NewTransition(transition_, stateReached_);
    }
}
 
template<typename T>
const std::string& Automaton<T>::GetName() const
{
    return _name;
}
 
template<typename T>
std::string& Automaton<T>::GetName()
{
    return _name;
}
 
template<typename T>
void Automaton<T>::SetName(const std::string& name_)
{
    _name = name_;
}
 
 
 
}

**dragonjoker59** · 09/03/2012, 15h23

Première remarque :
const std::string* currentStateName = new std::string(Automate1.GetCurrentState()->GetName());Déjà, dans ton code il n'y a aucun intérêt de déclarer un pointeur pour ça, de plus tu ne fais jamais de delete => Memory Leaks.
Fais donc comme ceci :
const std::string & currentStateName = Automate1.GetCurrentState()->GetName();Ensuite, pourquoi n'utilises-tu pas un enum plutôt que des chaînes de caractères pour les états de ton automate ? Au niveau des comparaisons tu gagnerais un temps non négligeable.

**Kaamui** · 09/03/2012, 15h50

Envoyé par dragonjoker59

Première remarque :
const std::string* currentStateName = new std::string(Automate1.GetCurrentState()->GetName());

en fait le type de retour de GetName() c'est soit const std::string* soit std::string* et comme je savais que je ne voulais pas le modifier ensuite, j'appelle la méthode const. Le pointeur, c'est juste pour collé au type de retour, j'avais pas envie que ce soit plus moche.

Envoyé par dragonjoker59

de plus tu ne fais jamais de delete => Memory Leaks.

Ah oui, merci. J'ai oublié de faire les Destructeurs ça me fait penser

Envoyé par dragonjoker59

Ensuite, pourquoi n'utilises-tu pas un enum plutôt que des chaînes de caractères pour les états de ton automate ? Au niveau des comparaisons tu gagnerais un temps non négligeable.

pour les états ? sois plus précis... c'est une base générique, le main n'est qu'un test, bien loin de ce à quoi cela va servir le tout.

Si tu parles des types d'état (initial, intermédiaire, final), j'utilise un enum. Sinon mes états sont des string juste pour tester, mais j'avais fait un test avec des int avant (c'est pour cela que dans le main il y a TInt et SInt), et j'ai même un exemple avec une classe Toto.

Edit : ah ok ce sont les touches haut, droite, etc qui t'ont perturbé. C'est juste pour illustrer.

**dragonjoker59** · 09/03/2012, 16h41

Ce ne sont pas les touches haut bas gauche et droite qui m'ont perturbé, mais bien les chaînes de caractères "Etat Initial", "Etat 2", "Etat 3", "Etat 4" et "Etat 5"
Et je pense que retourner un pointeur sur une chaîne n'est pas nécessaire ni spécialement une bonne idée (ce n'est que mon avis, j'essaie d'éviter les pointeurs quand je peux). Je retournerais plutôt une référence constante (ou pas, d'ailleurs) : const std::string & dans mon accesseur

En tout cas si tu passes par un pointeur, ne fais pas de new à cet endroit là, car ça nuit aux performances de ton appli.

**Kaamui** · 09/03/2012, 16h52

Envoyé par dragonjoker59

Ce ne sont pas les touches haut bas gauche et droite qui m'ont perturbé, mais bien les chaînes de caractères "Etat Initial", "Etat 2", "Etat 3", "Etat 4" et "Etat 5"
Et je pense que retourner un pointeur sur une chaîne n'est pas nécessaire ni spécialement une bonne idée (ce n'est que mon avis, j'essaie d'éviter les pointeurs quand je peux). Je retournerais plutôt une référence constante (ou pas, d'ailleurs) : const std::string & dans mon accesseur

En tout cas si tu passes par un pointeur, ne fais pas de new à cet endroit là, car ça nuit aux performances de ton appli.

Ok, c'est noté, pour les performances. Je fais immédiatement cette correction.

Maintenant, est-ce que je pourrais avoir tes réactions (et celle des autres) sur le code qui m'intéresse vraiment ? Dois-je gérer des exceptions? (si oui aurais-tu des conseils?), est-ce que d'autres choses ne vont pas ? etc...

**Ekleog** · 09/03/2012, 20h01

Si le but est d'écrire quelque chose de neuf, je ne dis rien.

Mais, si le but est d'avoir quelque chose d'efficace, à utiliser dans un projet plus grand, je ne peux que conseiller Boost.MetaStateMachine.

Free Automatons with State and Transitions : soumission d'un projet

C++

Vue hybride

Discussions similaires

Partager

Partager