Il y a peu, je suggérai sur une liste de diffusion concernant le développement d'un programme de revoir la politique des commentaires insérés dans le code. En effet, lors de la lecture des sources, une chose m'avait frappé (et gêné dans mon parcours) : il y avait des commentaires pour tout. En fait, dans les fichiers d'en-tête (il s'agit de C++) il y avait probablement plus de lignes de commentaires que de code.

Ce travers, puisque vous aurez compris que, pour moi, c'est une mauvaise habitude, est assez courante dans les sources de programmes. Même si j'ai l'impression que c'est en diminution par rapport à il y a quelques années ¹.

Lire du code source, lorsque l'on programme, et encore plus lorsque l'on aborde une section nouvelle pour nous, est l'activité principale. Les éditeurs font d'ailleurs tout pour apporter au programmeur l'information dont il a besoin au moment où il tape pour lui éviter des aller retours entre les fichiers.

Cette lecture doit donc être facilitée, afin que la compréhension du lecteur soit rapide et qu'il n'aille dans les détails que s'il en a besoin.

De là, les avis peuvent diverger. Certains diront que, puisqu'il faut donner de l'information au lecteur, il faut commenter le code de manière extensive. D'autres (et j'en fait partie), diront qu'il faut le commenter au minimum.

Je vais prendre un exemple, tiré de l'état actuel du code de Gnoll. La largeur de présentation de ce blog n'était pas assez longue, je vous conseille de copier/coller le contenu dans votre éditeur préféré. De plus, vous pourrez ainsi aller et venir entre l'éditeur et le texte d'explication qui suit le deuxième code source.

/***************************************************************************
 *   Copyright (C) 2006 by Puzzle Team                                     *
 *                                                                         *
 *   This program is free software; you can redistribute it and/or modify  *
 *   it under the terms of the GNU General Public License as published by  *
 *   the Free Software Foundation; either version 2 of the License, or     *
 *   (at your option) any later version.                                   *
 *                                                                         *
 *   This program is distributed in the hope that it will be useful,       *
 *   but WITHOUT ANY WARRANTY; without even the implied warranty of        *
 *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the         *
 *   GNU General Public License for more details.                          *
 *                                                                         *
 *   You should have received a copy of the GNU General Public License     *
 *   along with this program; if not, write to the                         *
 *   Free Software Foundation, Inc.,                                       *
 *   59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.             *
 ***************************************************************************/


/*----------------------------cmessagemanager------------------------------*\
|   This is a message manager                                               |
|                                                                           |
|   Changelog :                                                             |
|               05/15/2006 - Paf - Initial release                          |
|               04/10/2006 - Gabriel - Add namespace Gnoll and Core         |
|                                                                           |
\*-------------------------------------------------------------------------*/


#include <string>

#include "cmessagetype.h"
#include "cmessagelistener.h"

#ifndef __CMESSAGEMANAGER_H__
#define __CMESSAGEMANAGER_H__

using namespace std;

namespace Gnoll
{
    namespace Core
    {
        /**
         *  A message manager.
         *  This is a base implementation for all of the messages manager
         */
        class CMessageManager
        {

            public:

                /**
                 * This is a constructor
                 */
                CMessageManager() {}

                /**
                 * This is a destructor
                 */
                virtual ~CMessageManager() {}


                /**
                 * This adds a new listener
                 * @param handler The listener which handle messages
                 * @param messagetype The listener will care about every message of this message type
                 * @return True if ok, false otherwise
                 */
                virtual bool addListener ( shared_ptr<CMessageListener> handler, CMessageType messagetype ) = 0;

                /**
                 * This deletes a new listener
                 * @param handler The listener which handle messages
                 * @param messagetype The listener which have cared about every message of this message type
                 * @return True if ok, false otherwise
                 */
                virtual bool delListener ( shared_ptr<CMessageListener> handler, CMessageType messagetype ) = 0;

                /**
                 * This process a given message as parameter
                 * @param message The message to be processed
                 * @return True if ok, false otherwise
                 */
                virtual bool trigger ( shared_ptr<CMessage> message ) = 0;

                /**
                 * This enqueue a message
                 * @param message The message to be processed
                 * @return True if ok, false otherwise
                 */
                virtual bool queueMessage ( shared_ptr<CMessage> message ) = 0;

                /**
                 * This abort the first message or every message of type messagetype
                 * @param messagetype The message's or messages' type to be aborted
                 * @param alloftype Are every messages of type messagetype aborted ? (default : False)<br> True -> Yes <br> False -> No (only the first one is aborted)
                 * @return True if ok, false otherwise
                 */
                virtual bool abortMessage ( CMessageType messagetype, bool alloftype = false ) = 0;

                /**
                 * This validate a message type
                 * @param messagetype The message type to be validated
                 * @return True if ok, false otherwise
                 */
                virtual bool validateType( CMessageType messagetype ) = 0;

                /**
                 * This process the message queue
                 */
                virtual void process( ) = 0;


        };
    };
};

#endif // __CMESSAGEMANAGER_H__

Puis voilà une autre version. Je ne me suis occupé que des commentaires. Il y aurait d'autres choses à faire, mais je laisse les autres sujets de côté.

//  Copyright (C) 2006 by Puzzle Team
//  Check LICENSE.TXT for the complete license terms.

#ifndef __CMESSAGEMANAGER_H__
#define __CMESSAGEMANAGER_H__

#include <string>

#include "cmessagetype.h"
#include "cmessagelistener.h"

namespace Gnoll
{
    namespace Core
    {
        /** Interface for message managers
        *
        * A message manager is in charge distributing messages to corresponding listeners.
        */
        class CMessageManager
        {
            public:
                CMessageManager();
                virtual ~CMessageManager();

                virtual bool addListener(std::shared_ptr<CMessageListener> handler, CMessageType messagetype) = 0;
                virtual bool delListener(std::shared_ptr<CMessageListener> handler, CMessageType messagetype) = 0;

                virtual bool triggerMessage(std::shared_ptr<CMessage> message) = 0;
                virtual bool queueMessage(std::shared_ptr<CMessage> message ) = 0;
                virtual bool abortFirstMessage(CMessageType messagetype) = 0;
                virtual bool abortAllMessages(CMessageType messagetype) = 0;
                virtual bool isMessageTypeValid(CMessageType messagetype) = 0;

                virtual void processQueue() = 0;
        };
    };
};

#endif // __CMESSAGEMANAGER_H__

Puis comparons.

Tout d'abord, et même si cela dépend des écrans, la version originale tiens sur 3 pages de mon éditeur. La version remaniée tient sur une page. 120 lignes contre 41. Je peux englober toute la déclaration de la classe d'un coup d'œil sur la version remaniée, je dois scroller pour la version original.

Mais cela n'aurait aucun sens si j'avais par la même occasion détruit de l'information utile au lecteur.

Comparons donc de haut en bas.

• l'entête de copyright. Cette section est longue. Malheureusement, elle peut être dictée par le côté légal. Ma version fait deux lignes, l'une pour rappeler le copyright, l'autre pour donner un pointeur vers des renseignements complets. Est-ce suffisant ? Je ne sais pas. Je l'espère. D'un point de vue lecture du code, aucune information n'est détruite. D'un point de vue juridique... à voir. Ce n'est pas mon sujet.
• le récapitulatif des changements. Je l'ai supprimé. Toutes les informations sur les changements sont dans le dépôt de Gnoll, qui utilise Git. De plus, ces changements ne sont pas forcément d'actualité et, s'ils étaient mis systématiquement à jour, prendraient de plus en plus de place. On peut toujours se faire une règle de repli automatique pour cette section comme pour la précédente avec des éditeurs comme Vim. D'un point de vue lecture du code, ça n'a aucun intérêt.
• les #includes ont été déplacé. Cela n'a rien à voir avec mon sujet, mais ils m'embêtaient bien tels qu'ils étaient, en dehors des gardes d'inclusion.
• les commentaires de classe. Je les ai changé.
  • Le titre : initialement, le titre du commentaire est \"A message manager\". Étant donné que c'est le nom, au préfixe prêt de la classe, on peut s'en douter. Le commentaire n'apporte rien.
  • La phrase suivante, de description, indique « This is a base implementation for all of the messages manager ». C'est un bel exemple du danger que peuvent représenter des commentaires. Cette phrase est fausse ! Cette classe n'est ps une implémentation de base pour tous les gestionnaires de messages. Peut-être que ça l'était, avant qu'elle ne devienne une interface pure, peut-être que c'est un mauvais copier/coller. Mais c'est faux. Et lors de ma première lecture, je l'ai crue, ce qui m'a induit en erreur. En fait, l'implémentation de base se nomme CGenericMessagerManager et se trouve dans le même répertoire. On remarque d'ailleurs que son nom indique bien ce qu'il est. J'ai pu trouver cette classe par un simple parcours des fichiers présents dans le répertoire, sans recours à aucun commentaire.
  • Mon commentaire indique donc que la classe est une interface pour les gestionnaires de messages. La description indique rapidement quel est le contrat de l'interface. Si cette classe ne devait pas être vue par un client, j'enlèverais tout simplement le commentaire. On vient de voir qu'un commentaire était facilement obsolète.
• Les commentaires des constructeurs et destructeurs sont des cas d'écoles. En les enlevant, je n'enlève aucune information qui ne soit évidente, pourvu que l'on connaisse la syntaxe du langage. J'ai aussi enlevé les blocs vides. C'est une préférence personnelle, je n'aime pas voir de code dans un fichier d'en-tête. Mais je ne suis pas contre dans ce cas précis qui décrit une interface.
• addListener : le commentaire était une redondance du nom de la méthode. De plus, il est faut. Un commentaire plus juste pourrait être « This adds a new listener and returns true or return false if the listener was not added ». On se rend compte (mais on s'en rend compte en lisant le code du gestionnaire générique) que cette méthode en fait beaucoup plus que ce qu'elle indique, que ce soit par son nom même ou par son commentaire. On déborde là du sujet qui m'intéresse dans ce billet. Le fait qu'un listener écoute les messages qui lui sont adressés a déjà été dit dans ma version corrigée du commentaire de classe. Le fait de renvoyer vrai en cas de succès ne détail pas du tout ce qui pourrait être un échec. Le commentaire n'a pas de valeur, je l'enlève.
• delListener : même raisons, même effet. À noter qu'à cause d'un copier/coller malheureux, le commentaire là encore est faux : « This deletes a new listener ». A new listener ? Intéressant...
• trigger : cette méthode m'a posé problème. Trigger... mais trigger quoi ? Le commentaire ne m'aide pas plus car il annonce que cette méthode process un message. Trigger, process ? Lequel ? On apprend déjà que c'est un message qui est affecté, mais ceci, on peut s'en douter à la lecture de la signature de la méthode, qui prend un message en paramètre. J'ai du aller lire le code du gestionnaire générique pour comprendre ce que faisait la méthode. En fait, elle distribue au listeners un message immédiatement. Un bon commentaire aurait donc donné cette information. Mais trigger, après tout, contient cette notion d'instantanéité. Pourquoi pas. Mais pourquoi que l'on change un peu le nom de la méthode, le commentaire n'a plus d'intérêt. J'opte pour triggerMessage(), pour ne pas trop m'éloigner. Je ne suis pas entièrement satisfait du nom cependant. Un sendImmediateMessage() me semble encore mieux. Encore une fois, la méthode renvoie un booléen sous des conditions qui ne sont pas expliquées. Mais c'est un autre sujet.
• queueMessage : tout comme les autres, le commentaire est redondant avec le nom de la méthode. J'enlève.
• abortMessage : tout comme les autres... ou presque. Cette méthode donne une information sur le paramètre alloftype. Je n'aime pas ce fonctionnement dual en fonction d'un paramètre qui n'est pas très clair. ll y a un besoin de supprimer un seul message parfois et un besoin d'effacer tous les messages ? Je soupçonne que ces cas sont séparés et pas appelés par les mêmes clients. Je duplique la méthode en séparant les besoins, et j'élimine le besoin de commentaire.
• validateType : j'ai du aller voir le code pour celui-ci aussi. Le commentaire n'aide pas : il indique que le message va être validé. Si les autres méthodes ne renvoyaient pas toute une booléen commenté de la même façon, on aurait un indice sur le fait que nous avons affaire à une fonction de validation. Ici, on ne sait pas trop si c'est une fonction ou une commande qui validerait le type auprès du gestionnaire (pour lui donner, par exemple, la liste des types de message valides). Après lecture du gestionnaire générique, je comprends qu'il s'agit bien d'une fonction. Je change donc le nom en isMessageTypeValid(). Je suis un peu ennuyé par le classement public de cette méthode, mais comme ce n'est pas le sujet de ce billet, je ne suis pas allé fouiller le reste du code pour savoir si cette méthode avait du sens. Peut-être, peut-être pas. En tout cas, en changeant son nom et en supprimant les commentaires, je donne plus d'informations au lecteur qu'avec son ancien nom accompagné de ses commentaires.
• process : le nom de la méthode n'indique pas ce qu'elle fait. Le commentaire oui. Mais alors, pourquoi pas intégrer l'information dans le nom ? Je change en processQueue() et j'efface les commentaires.

Je n'ai, après toutes ses opérations, enlevé aucune information pour lecteur. Celui-ci peut maintenant embrasser d'un coup d'œil l'interface. Il devrait probablement aller voir le code du gestionnaire générique pour comprendre dans quels cas les cinq premières méthodes renvoient vrai ou faux, mais cette information n'était pas disponible avant et est plus un problème de design que d'écriture (bien que ces sujets soient très liés).

La règle qui consiste à dire que plus un code source est court, plus il est compréhensible et plus il est long, plus il est lourd à lire, à comprendre et sujets au bugs. Cette règle est assez bien perçue en ce qui concerne l'implémentation elle-même, au code qui va être compilé. Mais elle ne s'applique pas qu'à ce domaine, elle s'applique à la globalité du fichier. Et cela signifie qu'il ne faut pas être bavard dans les commentaires mais plutôt s'attacher à écrire du code assez lisible pour ne pas nécessité de commentaires.