Discussion :

[Invité]

Bonjour,

J'ai une question.
J'ai fait un classe, toute simple, qui n'est là que pour me simplifier un peu la tâche (c'est le but de la prog vous allez me dire). Voici le code :

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
class DrawableList : public sf::Drawable, public std::vector<sf::Drawable*>
{
    public:
        DrawableList() : Drawable(), vector<sf::Drawable>(), deleteAll(false)
        {}
 
        DrawableList(delAll) : Drawable(), vector<sf::Drawable>(), deleteAll(delAll)
        {}
 
        ~DrawableList()
        {
        if(deleteAll)
            for(std::size_t i(0) ; i < size() ; i++)
                delete (*this)[i];
        }
 
        void setDeleteAll(bool delAll)
        {
            deleteAll = delAll;
        }
 
        bool getDeleteAll()
        {
            return deleteAll;
        }
 
    private:
        void draw(sf::RenderTarget& target, sf::RenderStates states) const;
        // See 'sf::Drawable' in the SFML documentation.
 
        bool deleteAll;
};

La question est : vu que l'attribut 'deleteAll' est tout simple, qu'il n'y a aucune vérification ou opération quand on le modifie que de lui donner la valeur en argument, et aucune restriction pour obtenir sa valeur, est-ce grave si je le rend public ? Histoire de simplifier un peu la chose.
Parce que pour le coup cela n'aurait aucune différence entre faire monInstance.setDeleteAll(true) et monInstance.deleteAll = true. Si ? Je sais que, principe d'encapsulation, tout ça. Mais est-ce grave si je fais cette exception ?
Je suis en fait partagé entre mon côté pratique qui me dit "vas-y, rend-le public, ça ne change rien à part simplifier les choses" et mon côté "bon petit soldat" qui me rappelle que non, les attributs d'une classe doivent toujours être privés.

Merci d'avance pour vos avis.

[Bousk]

Salut,

non, les attributs d'une classe doivent toujours être privés.

Ha bon ? Depuis quand ?

Y'aurait bien plus à redire du reste de la classe que de mettre ou non cette variable public.

[Invité]

Depuis quand ? Moi j'ai appris comme ça.

Mais, heu..le reste de ma classe ? Quel est (ou quels sont ?) le problème ?

Si c'est à propos des constructeur, dans la vraie implémentation, il y en a un seul avec l'argument 'delAll' qui a une valeur par défaut à false (plutôt que d'avoir deux constructeurs). Là j'en ai mis deux pour que le code puisse fonctionner alors que toute la définition de la classe est à un même endroit.
Mais j'imagine qu'il y a plus grvae (je vois difficilement quoi

EDIT :
AH oui, j'ai oublié de mettre la définition de draw().

Code :

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

1
2
3
4
5
6
void draw(sf::RenderTarget& target, sf::RenderStates states) const
{
    for(std::size_t i(0) ; i < size() ; i++)
        if((*this)[i])  // If the pointer at index 'i' is not null...
            target.draw( *((*this)[i], states) );
}

[bacelar]

Vous avez de bons reflexes de développeur.
La règle de ne pas mettre en public les attributs vient plus de la culture JAVA que C++.
Mais il faut avoir un certain recule sur toutes les règles.

Il faut comprendre que tout ce qui est public fait partie de l'interphase de service de votre objet.
Il est donc très difficile de revenir après dessus car cela un impact sur le code client de vos objets.

Il est donc primordial que les détails d'implémentation de votre classe soit invisible du code client.

Ce que vous présentez à l'extérieur de votre classe doit être très murement évalué.

Une classe comme Point qui ne stocke que les coordonnées d'un point peut facilement exposer directement ses membres car cela constitue une interphase raisonnable.

Un truc qui défrise avec votre classe, c'est qu'elle hérite déjà publiquement de "std::vector<sf:rawable*>", il est donc bizarre d'avoir besoin de ce genre de détail qui ne correspond à rien du point de vue de l'utilisateur et il peut faire sans ce détail.

[Invité]

Un truc qui défrise avec votre classe, c'est qu'elle hérite déjà publiquement de "std::vector<sf::Drawable*>", il est donc bizarre d'avoir besoin de ce genre de détail qui ne correspond à rien du point de vue de l'utilisateur et il peut faire sans ce détail.

Comment ça ? Mettons que l'on crée des sf::Drawable exprès pour mettre leurs adresses dans une DrawableList, et que par la suite leurs adresses ne sont QUE dans cette DrawableList. Il faut bien que celle-ci puisse libérer la mémoire si on ne veut âs de fuite ? De même, si au contraire on partage (pour diverses raisons...) un même Drawable dans plusieurs DrawableList (d'où le fait que l'on stocke des pointeurs et non les Drawable eux-mêmes), il ne faut au contraire pas libérer la mémoire en détruisant un des DrawableList, ou le programme crashera quand un autre essaiera d'utiliser le Drawable supprimé.
Exemple de situation répondant à ce cas :
Je crée un programme éducatif. Celui-ci est composé d'énigmes qui s'enchaînent les unes après les autres. Pour chaque énigme, j'utilise un DrawableList afin de simplifier les choses (c'est le but). Mettons que dans chaque énigme il y ait un bouton "Retour au menu principal". Inutile de créer le même 15 fois, oui ? Donc chaque DrawableList aura un pointeur vers un seul et même bouton qui ainsi, sera toujours au même endroit, fera la même chose etc. Pareil avec un éventuel bouton "Valider".

Après je ne sais pas si c'est ce que vous vouliez dire.

[bacelar]

C'est beaucoup trop casse-gueule votre machin.
Faites simple, parce que là, on n'est pas foutu de savoir qui est responsable ou pas de la libération mémoire.
Un petit conseil, débarrassez-vous très rapidement de ces pointeurs nus pour des pointeurs intelligents.
Vous verrez que vous vous prenez la tête pour pas grand chose.

[Invité]

Pas bête ! Je vais utiliser des std::shared_ptr. Ils sont parfaits pour ce que je veux faire, non ?

[koala01]

Salut,

A vrai dire, je ne suis pas loin du tout de penser à un énorme problème de conception si tu décide de faire hériter ta list de sf::Drawable et de std::vector<Drawable*>.

Qu'à la limite, elle hérite effectivement de manière publique de sf::Drawable pourrait éventuellement se conprendre, bien que j'aurais personnellement tendance à considérer que je veux avoir une liste (tout à fait normale) d'éléments dérivés de sf::Drawable qui expose (je parle de la liste ici ) un composant permettant de l'afficher dans la SFML. (*)

Mais lorsque je vois que tu fais hériter ta classe de std::vector<Drawable*>, là, je commence vraiment à sentir l'oignon En effet, aucune fonction membre de la classe std::vector n'est virtuelle et surtout pas son destructeur.

Car, pour qu'une classe puisse servir de classe de base dans une hiérarchie de classe, il que le destructeur soit public et virtuel, ou qu'il soit protégé et non virtuel. Et, pour ton malheur, le destructeur de std::vector n'est ni protégé ni virtuel

En outre, je serais personnellement très surpris que tu aies vraiment besoin de l'ensemble de l'interface de std::vector, les fonctions les plus fréquemment utilisées étant généralement

• les deux versions de begin et end
• les deux versions de l'opérateur []
• push_back /emplace_back
• size
• clear
• reserve
• resize
• erase
• allez, j'en oublie peut être l'une ou l'autre

Enfin, à titre personnel, ce que je ferais bien volontiers, ce serait de virer l'héritage public d'avec std::vector pour le remplacer par un membre "tout bête" du type adéquat et de créer des fonctions membres (publiques) qui ne feraient qu'appeler celles de std::vector dont tu as besoin... Cela prendrait sans doute la forme de

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
 
class DrawableList : public sf::Drawable{
    public:
        using const_iterator = typename std::vector<std::unique_ptr<sf::Drawable>>::const_iterator;
        using iterator = typename std::vector<std::unique_ptr<sf::Drawable>>::iterator;
        sf::Drawable const & operator [] (size_t index)const {
            assert(index < items_.size());
            return *(items_[index].get());
        }
        sf::Drawable & operator[](size_t index) {
            assert(index < items_.size());
            return *(items_[index].get());
        }
        size_t size() const{
            return items_.size();
        }
        void push_back(sf::Drawable * toadd){
            items_.emplace_back(std::unique_ptr<sf::Drawable>(toadd);
        }
        void erase(size_t index){
            assert(index < items_.size());
            items_.erase(items_.begin()+index);
       }
       void erase(const_iterator torem){
           items_.erase(torem);
      }
      void clear(){
          items_.clear();
      }
      void resize(size_t newSize){
          itesms_.resize(newSize);
      }
      void reserve(size_t toReserve){
          items_.reserve(toReserve);
      }
    private:
        std::vector<std::unique_ptr<sf::Drawable>> items_;
};

(*) Il ne faut pas oublier que l'héritage public est la relation la plus forte qui puisse exister entre deux classes, vu qu'elle implique qu'une instance de la classe dérivée peut être transmis en paramètre à n'importe quelle fonction s'attendant à recevoir une instance du type de la classe de base (c'est ce qui s'appelle la subtituabilité ).

A ce titre, c'est aussi la relation que nous ne devrions envisager que si l'on a réellement de bonne raisons de l'utiliser et que son utilisation ne contrevient pas au principe de substitution de Liskov (Liskov Subtitution Principle ou LSP, en anglais )

Or, de toute évidence, la liste est destinée à contenir des éléments traçables et à être (de toute évidence) utilisée comme membre d'une autre classe que tu auras pris soin de faire hériter de sf::Drawable. Tu pourrais donc parfaitement envisager de tout simplement fournir un composant (une fonction toute bête, par exemple) qui s'assurera que tous les éléments contenus dans ta liste seront affichés, sans pour autant mettre toute la mécanique nécessaire à l'héritage public en place

En effet, je suis intimement persuadé que, d'une manière ou d'une autre, ta classe DrawableList sera utilisée sous une forme proche de

Code :

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

1
2
3
4
5
6
7
class MyDrawableItem : public sf::Drawable{
    public:
       void draw(/* paramètres éventuels*/) /* const */override{ // ou n'importe quel autre nom issu de sf::Drawable
           myList.draw(/* paramètres éventuels*/);
    private:
        DrawableList myList;
};

A ce stade, le fait que la foncton membre de DrawableList porte le même nom que la fonction issue de sf::Drawable est totalement anecdotique!!! tu aurais très bien pu choisir de la nommer drawAllItems ou afficheMoiToutCela, cela n'aurait pas eu la moindre influence Tout ce qu'il faut, c'est que cette fonction appelle la fonction draw (ou n'importe quel autre nom issu de sf::drawable sur chaque élément contenu dans la liste

Cela aurait très bien pu prendre la forme de

Code :

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

1
2
3
4
5
6
void DrawableList::afficheMoiToutCela(/* paramètres éventuels*/) const{
    for(auto const & it : items_){
        if(it.get()!=nullptr)
            it.get()->draw(/* paramètres éventuels*/);
    }
}

[Invité]

Alors l'intérêt de la faire hériter de sf::Drawable était de pouvoir faire

Code :

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

1
2
3
4
5
6
sf::RenderWindow window(/* arguments nécessaires à la création de la fenêtre */);
glan::DrawableList list;
/* remplissage de la liste */
window.clear();
window.draw(list);
window.display();

Vu que les utilisateur de la SFML appellent la fonction draw() de la fenêtre, puisque celle de sf::Drawable et ses dérivées est privée.
Juste histoire de se dire qu'ils peuvent juste faire comme d'habitude, sans avoir à penser que "ah non, c'est vrai, pour DrawableList je ne peux pas appeller RenderTarget::draw()".

Donc je garde l'héritage de sf::Drawable, qui, en bonne classe abstraite, est ed toutes façons faite pour être héritée.

Cependant j'hésite à retirer l'héritage de std::vector car, même si effectivement son destructeur n'est pas virtuel, la classe n'est pas non plus destinée être utilisée via un pointeur de vector...vu qu'on est supposé pouvoir appeler la fonction draw dessus !
Et puis bon, si c'est pour faire ça :

Code :

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

1
2
3
4
5
6
7
8
9
10
11
size_t DrawableList::size()
{
    return vect.size();
}
 
void DrawableList::clear()
{
    vect.clear();
}
 
// etc

Bon...autant laisser le vector membre public ! Ce qui nous ramène au titre du sujet...

[Flob90]

Si vraiment tu veux réimporter des morceaux de std::vector dans ta classe personnel, tu peux faire un héritage privé et utiliser des directive using :

Code :

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

1
2
3
4
5
 
struct A : private std::vector<int>
{
  using std::vector<int>::push_back;
};

C'est sémantiquement équivalent au dernier code que tu montres (donnée membre et fonctions qui appellent directement celles de std::vector).

[Médinoc]

Tu le dis toi-même:

la classe n'est pas non plus destinée être utilisée via un pointeur de vector...

Tu n'as donc aucune raison d'hériter publiquement de vector. Mais comme le dit Flob90, l'héritage privé est envisageable.

[Invité]

Va pour l'héritage privé de std::vector. Si c'est la seule chose acceptable qui puisse me convenir...