[F] [mOved] Rendre lisible un code tout en gérant les erreurs - Page 1

Ce sujet est issu d'une discussion ayant dérivé sur deux thèmes distincts. Pour vous rendre sur le sujet d'origine, suivez ce lien.

Pour ceux qui ont l'habitude de ballader un .clang-format dans leurs projets, voici un site interactif avec exemples systématique, pour paramétrer vos formattages de sources aux petits oignons : http://clangformat.com/

---

Voici un bout de mon code du moment : // Create the button texture (transparent) and set it as render target m_Texture = SDL_CreateTexture(Game::get()->renderer(), px, SDL_TEXTUREACCESS_TARGET, width, height); if (m_Texture == nullptr) { throw Exception(ERROR_SDL_STR); } if (SDL_SetTextureBlendMode(m_Texture, SDL_BLENDMODE_BLEND) != 0) { cleanup(); throw Exception(ERROR_SDL_STR); } if (SDL_SetRenderDrawColor(Game::get()->renderer(), 0, 0, 0, 0) != 0) { cleanup(); throw Exception(ERROR_SDL_STR); } if (SDL_RenderClear(Game::get()->renderer()) != 0) { cleanup(); throw Exception(ERROR_SDL_STR); } if (SDL_SetRenderTarget(Game::get()->renderer(), m_Texture) != 0) { cleanup(); throw Exception(ERROR_SDL_STR); }Comme vous le voyez, on a 5 lignes de code utiles pour une vingtaine de lignes de vérification. Au début, ça va, puis ça facilite bien le débogage. Mais à un moment ça devient pénible à lire (sans parler du fait que ce n'est pas beau ).
Comment s'y prendre pour alléger les choses ?
- une macro, mais parfois le code de nettoyage avant de lancer l'exception varie, donc ça risque de faire un autre bordel, mais un bordel quand même
- comme sur tous les exemples SDL que je vois sur internet : ne rien vérifier. C'est pas forcément casse-gueule en pratique, mais c'est pas optimal question débogage par exemple (surtout que mon exception fait afficher le fichier, la fonction, la ilgne incriminée et l'erreur SDL, ça aide).
- ... ???

Merci pour vos idées.

1) rajouter ce qu'il faut dans cleanup() pour qu'elle puisse être appelée à tout moment (en gros, faire que ça n'essaie pas de nettoyer ce qui n'a pas encore été initialisé) -> tu élimines le cas particulier de ton premier if()

2) Splitter en deux fonctions : bool fonction1(void) { // Create the button texture (transparent) and set it as render target m_Texture = SDL_CreateTexture(Game::get()->renderer(), px, SDL_TEXTUREACCESS_TARGET, width, height); if (m_Texture == nullptr) return false; if (SDL_SetTextureBlendMode(m_Texture, SDL_BLENDMODE_BLEND) != 0) return false; if (SDL_SetRenderDrawColor(Game::get()->renderer(), 0, 0, 0, 0) != 0) return false; if (SDL_RenderClear(Game::get()->renderer()) != 0) return false; if (SDL_SetRenderTarget(Game::get()->renderer(), m_Texture) != 0) return false; return true; }void function2(void) { if (!fonction1) { cleanup(); throw Exception(ERROR_SDL_STR); } }Le principe est du C pur (et n'est pas Ximoon-compliant ), tu dois pouvoir faire ça plus élégamment en C++ avec des exceptions, mais comme c'est pas mon domaine je laisse quelqu'un d'autre proposer une meilleure solution

[edit] cross, mais comme on a des propositions différentes je laisse

Comme ton code de nettoyage est toujours le même, tu devrais pouvoir le factoriser assez facilement. Par contre c'est une solution qui ne s'applique pas si chaque étape ajoute une nouvelle action de nettoyage en cas d'échec. Bon, je me permets de virer les accolades pour les corps d'instruction d'une seule ligne parce que j'aime pas // Create the button texture (transparent) and set it as render target m_Texture = SDL_CreateTexture(Game::get()->renderer(), px, SDL_TEXTUREACCESS_TARGET, width, height); if (m_Texture == nullptr) throw Exception(ERROR_SDL_STR); if (SDL_SetTextureBlendMode(m_Texture, SDL_BLENDMODE_BLEND) != 0 || SDL_SetRenderDrawColor(Game::get()->renderer(), 0, 0, 0, 0) != 0 || SDL_RenderClear(Game::get()->renderer()) != 0 || SDL_SetRenderTarget(Game::get()->renderer(), m_Texture) != 0) { cleanup(); throw Exception(ERROR_SDL_STR); }

Ou aussi quelque chose du genre de :

void myassert(int i)
: throws Exception
{
if ( i != 0 ) {
        cleanup();
        throw Exception(ERROR_SDL_STR);
}
}


myassert(SDL_SetTextureBlendMode(m_Texture, SDL_BLENDMODE_BLEND)) ;
myassert(SDL_SetRenderDrawColor(Game::get()->renderer(), 0, 0, 0, 0)) ;
myassert(SDL_RenderClear(Game::get()->renderer())) ;
myassert(SDL_SetRenderTarget(Game::get()->renderer(), m_Texture)) ;

(Tu peux d'ailleurs utiliser un mécanisme d'assertion déjà défini, potentiellement en debug uniquement (définition via des macros)

Ou sinon tu peux aussi patcher le préprocesseur pour qu'il ajoute les vérifications après chaque appel SDL (ou faire un peu de programmation orientée aspect — POA, qui est une belle machine. Mais j'ai pas encore énormément d'expérience dans le domaine.)

./1329: Et tu ne crées pas un nouveau topic pour ? C'est quoi ce hors sujet.

Oui, tu aurais au moins pu digresser

C'est plus une réflexion qu'un grand thread sur de l'algorithmique bien pointue, s'pour ça

En tout cas, merci à tous pour vos réponses.

./1330: C'est moche plusieurs return dans une seul fonction bool fonction1(void) { bool ret = false; // Create the button texture (transparent) and set it as render target m_Texture = SDL_CreateTexture(Game::get()->renderer(), px, SDL_TEXTUREACCESS_TARGET, width, height); if (m_Texture == nullptr) goto exit; if (SDL_SetTextureBlendMode(m_Texture, SDL_BLENDMODE_BLEND) != 0) goto exit; if (SDL_SetRenderDrawColor(Game::get()->renderer(), 0, 0, 0, 0) != 0) goto exit; if (SDL_RenderClear(Game::get()->renderer()) != 0) goto exit; if (SDL_SetRenderTarget(Game::get()->renderer(), m_Texture) != 0) goto exit; ret = true; exit: return ret; }
Voila c'est mieux

Sinon il y a la technique Zephique, mais surement pas la Pensquarique

Folco > Il n'existe pas un wrapper C++ ? Ce serait ça le plus propre

()

Il y a bien ça, mais c'est pas officiel et pas complet : https://github.com/libSDL2pp/libSDL2pp
Par contre à surveiller, ça a l'air actif.

Pas mal Godzil, ça évite de passer par une macro et ça transforme tout en one-liner. La méthode de Zeph est bien pour ça aussi, mais en cas d'appel à une fonction "bloquante" en cas d'échec, ça demande d'écrire plusieurs conditions.

La nuit porte conseil, merci beaucoup pour tout. Et j'espère retrouver l'idée que j'ai eue et qui m'a échappé, c'est dommage ^^

(\o/)
La méthode Godzilique, c'est la méthode VB (on error goto lbl)

Perso, je trouve que le grand dogme du "return unique" n'apporte rien, particulièrement si on fait des goto pour y arriver. Au final ça revient exactement à la même chose.

Oui mais généralement on fait un traitement après le label, ça permet de le factoriser.

Uther (./1340) :
Perso, je trouve que le grand dogme du "return unique" n'apporte rien, particulièrement si on fait des goto pour y arriver. Au final ça revient exactement à la même chose.

Ça peut aider quand il y a de l'analyse automatique de code, me semble-t-il

Folco (./1329) :
Voici un bout de mon code du moment : // Create the button texture (transparent) and set it as render target m_Texture = SDL_CreateTexture(Game::get()->renderer(), px, SDL_TEXTUREACCESS_TARGET, width, height); if (m_Texture == nullptr) { throw Exception(ERROR_SDL_STR); } if (SDL_SetTextureBlendMode(m_Texture, SDL_BLENDMODE_BLEND) != 0) { cleanup(); throw Exception(ERROR_SDL_STR); } if (SDL_SetRenderDrawColor(Game::get()->renderer(), 0, 0, 0, 0) != 0) { cleanup(); throw Exception(ERROR_SDL_STR); } if (SDL_RenderClear(Game::get()->renderer()) != 0) { cleanup(); throw Exception(ERROR_SDL_STR); } if (SDL_SetRenderTarget(Game::get()->renderer(), m_Texture) != 0) { cleanup(); throw Exception(ERROR_SDL_STR); }Comme vous le voyez, on a 5 lignes de code utiles pour une vingtaine de lignes de vérification. Au début, ça va, puis ça facilite bien le débogage. Mais à un moment ça devient pénible à lire (sans parler du fait que ce n'est pas beau ).Comment s'y prendre pour alléger les choses ?

Personnellement, je mettrais: // Create the button texture (transparent) and set it as render target m_Texture = SDL_CreateTexture(Game::get()->renderer(), px, SDL_TEXTUREACCESS_TARGET, width, height); if (m_Texture == nullptr) throw Exception(ERROR_SDL_STR); if (SDL_SetTextureBlendMode(m_Texture, SDL_BLENDMODE_BLEND) != 0) { error: cleanup(); throw Exception(ERROR_SDL_STR); } if (SDL_SetRenderDrawColor(Game::get()->renderer(), 0, 0, 0, 0) != 0) goto error; if (SDL_RenderClear(Game::get()->renderer()) != 0) goto error; if (SDL_SetRenderTarget(Game::get()->renderer(), m_Texture) != 0) goto error;voire: // Create the button texture (transparent) and set it as render target m_Texture = SDL_CreateTexture(Game::get()->renderer(), px, SDL_TEXTUREACCESS_TARGET, width, height); if (m_Texture == nullptr) { throw_exception: throw Exception(ERROR_SDL_STR); } if (SDL_SetTextureBlendMode(m_Texture, SDL_BLENDMODE_BLEND) != 0) { error: cleanup(); goto throw_exception; } if (SDL_SetRenderDrawColor(Game::get()->renderer(), 0, 0, 0, 0) != 0) goto error; if (SDL_RenderClear(Game::get()->renderer()) != 0) goto error; if (SDL_SetRenderTarget(Game::get()->renderer(), m_Texture) != 0) goto error;

Zerosquare (./1330) :
Le principe est du C pur (et n'est pas Ximoon-compliant ), tu dois pouvoir faire ça plus élégamment en C++ avec des exceptions, mais comme c'est pas mon domaine je laisse quelqu'un d'autre proposer une meilleure solution

Bah, c'est un cas classique pour un try … finally. Le mot-clé finally n'existe pas en C++, mais peut-être facilement émulé par une macro. (Seul inconvénient: il faut utiliser des parenthèses en guise d'accolades, parce que la macro doit dupliquer le code à l'intérieur de l'accolade.) Donc une solution plus C++-compliant (mais pas encore tout à fait, je vais y venir par la suite) est:#define FINALLY(cleanups) catch(...) {cleanups throw;} {cleanups} // Create the button texture (transparent) and set it as render target m_Texture = SDL_CreateTexture(Game::get()->renderer(), px, SDL_TEXTUREACCESS_TARGET, width, height); if (m_Texture == nullptr) throw Exception(ERROR_SDL_STR); try { if (SDL_SetTextureBlendMode(m_Texture, SDL_BLENDMODE_BLEND) != 0) throw Exception(ERROR_SDL_STR); if (SDL_SetRenderDrawColor(Game::get()->renderer(), 0, 0, 0, 0) != 0) throw Exception(ERROR_SDL_STR); if (SDL_RenderClear(Game::get()->renderer()) != 0) throw Exception(ERROR_SDL_STR); if (SDL_SetRenderTarget(Game::get()->renderer(), m_Texture) != 0) throw Exception(ERROR_SDL_STR); } FINALLY( cleanup(); )
Maintenant, pourquoi n'est-ce toujours pas tout à fait C++-compliant? Et pourquoi n'y a-t'il pas de mot-clé finally en C++? Parce que la solution C++ est la technique RAII. (En gros, tu construis un objet local dont le destructeur, qui sera automatiquement appelé à la sortie du scope, sera chargé d'appeler cleanup().)

Godzil (./1336) :
./1330: C'est moche plusieurs return dans une seul fonction bool fonction1(void) { bool ret = false; // Create the button texture (transparent) and set it as render target m_Texture = SDL_CreateTexture(Game::get()->renderer(), px, SDL_TEXTUREACCESS_TARGET, width, height); if (m_Texture == nullptr) goto exit; if (SDL_SetTextureBlendMode(m_Texture, SDL_BLENDMODE_BLEND) != 0) goto exit; if (SDL_SetRenderDrawColor(Game::get()->renderer(), 0, 0, 0, 0) != 0) goto exit; if (SDL_RenderClear(Game::get()->renderer()) != 0) goto exit; if (SDL_SetRenderTarget(Game::get()->renderer(), m_Texture) != 0) goto exit; ret = true; exit: return ret; }
Voila c'est mieux

Le goto exit;, c'est utile si on a vraiment du code commun à factoriser (cf. mon post précédent), mais si c'est pour sauter à un return, ça ne sert strictement à rien.

Merci beaucoup Kevin, ça a l'air d'être bien le RAII, même si ça multiplie les classes (ce que j'aime pas trop).
Par contre, si ça a l'air utile pour les objets dont la durée de vie est locale à une fonction, ça n'a pas l'air si approprié à mon cas : de multiples appels de fonction dans un constructeur. Ceci dit, je retiens cette technique d'encapsulation des ressources.

Et ça me donne une idée, je peux tout simplement wrapper les appels SDL dans des fonctions inlinées. Rien de compliqué, juste une vérification de la valeur de retour dans le wrapper et zou.
Ca ne vous parait pas choquant ?

Merci à tous en tout cas, j'ai l'impression d'être nu gros débutant qui découvre qu'il y a autre chose que la programmation naïve

(Tu aurais carrément du digresser. On devrait toujours digresser !)

(En fait, oui. Mais je pensais que la réponse serait un one-liner et que ça serait réglé. Alors pourquoi j'ai posté là ? Parce qu'historiquement, je postais mes petites remarques sur la prog dans yArotweet, jusqu'à ce qu'on me fasse remarquer que certains non-codeurs ne s'y retrouvaient pas, et qu'il valait mieux que j'utilise ce topic un peu fourre-tout. Voili voulou )

---

Donc voilà ce que je suis en train de faire : un wrapper pour les vérifications de retour de fonction, et j'utilise la méthode RAII pour la gestion des objets dont la création peut être annulée. Merci à tous.

Pour ma part, je n'utiliserais ni les exceptions, ni encore pire, les structures de contrôle sans accolades.
Les exceptions ne sont pas gratuites en termes de taille de code (sur une base de code significative basée sur Qt, j'avais trouvé une différence proche de 20% ), et leur traitement des exceptions peut nécessiter des allocations mémoire et aggraver les OOM.
Les structures de contrôle sans accolades sont connues pour avoir causé, par exemple, un bug de sécurité grave dans le kernel Linux il y a des années. A titre professionnel, j'avais trouvé un test faux de la forme
[code]if (condition)
statement;
return value;

statements[/code]
qui masquait un vrai bug dans le code testé.


De nos jours, j'utiliserais probablement quelque chose comme
[code]
bool success = false; // ou plutôt une enum, éventuellement typée en C++11 et dialectes ultérieurs, contenant des codes de retour
do {
m_Texture = SDL_CreateTexture(Game::get()->renderer(), px, SDL_TEXTUREACCESS_TARGET, width, height);
// Séparation de toutes les conditions du if, pour permettre un éventuel ajout individuel de message d'erreur ou traitement particulier, un jour.
if (nullptr == m_Texture) {
break;
}

// Ici, par habitude, j'omettrais probablement "0 !=", mais c'est mal)
if (0 != SDL_SetTextureBlendMode(m_Texture, SDL_BLENDMODE_BLEND)) {
break;
}

if (0 != SDL_SetRenderDrawColor(Game::get()->renderer(), 0, 0, 0, 0)) {
break;
}

if (0 != SDL_RenderClear(Game::get()->renderer())) {
break;
}

if (0 != SDL_SetRenderTarget(Game::get()->renderer(), m_Texture)) {
break;
}
success = true;
} while(0);

if (<success_value> != success) { // si success est un booléen, version abrégée
cleanup(); // cleanup doit être capable de gérer le fait que m_Texture == nullptr.
}

// Plus loin, une forme de "return success;", pour signaler l'erreur à l'appelant - c'est à l'appelant de traiter ou propager l'erreur.
[/code]

Et ce serait intéressant de voir ce que pense Ximoon de nos morceaux de code

Folco (./1345) :
Merci beaucoup Kevin, ça a l'air d'être bien le RAII, même si ça multiplie les classes (ce que j'aime pas trop).Par contre, si ça a l'air utile pour les objets dont la durée de vie est locale à une fonction, ça n'a pas l'air si approprié à mon cas : de multiples appels de fonction dans un constructeur. Ceci dit, je retiens cette technique d'encapsulation des ressources.

Tu peux utiliser un nettoyeur générique:template <class T> Cleanup { private: T *m_p; void (T::*m_pCleanup)(); public: Cleanup (T *p, void (T::*pCleanup)()) : m_p(p), m_pCleanup(pCleanup) {} ~Cleanup() {m_p->m_pCleanup();} } // Create the button texture (transparent) and set it as render target m_Texture = SDL_CreateTexture(Game::get()->renderer(), px, SDL_TEXTUREACCESS_TARGET, width, height); if (m_Texture == nullptr) throw Exception(ERROR_SDL_STR); Cleanup<MyClass> c(this, &MyClass::cleanup); if (SDL_SetTextureBlendMode(m_Texture, SDL_BLENDMODE_BLEND) != 0) throw Exception(ERROR_SDL_STR); if (SDL_SetRenderDrawColor(Game::get()->renderer(), 0, 0, 0, 0) != 0) throw Exception(ERROR_SDL_STR); if (SDL_RenderClear(Game::get()->renderer()) != 0) throw Exception(ERROR_SDL_STR); if (SDL_SetRenderTarget(Game::get()->renderer(), m_Texture) != 0) throw Exception(ERROR_SDL_STR);
Ce n'est pas forcément plus lisible (ni plus efficace au niveau du code produit) que ma macro FINALLY, mais c'est plus conforme à l'esprit du C++.

C'est une manière de faire, mais ça garde le problème qui se pose et que je suis en train d'éviter : 30 lignes de code et un ajout de bloc while artificiel pour 5 lignes utiles.
Quant succès signalés systématiquement par valeur de retour, c'est handicapant parce que ça ne permet pas d'utiliser cette valeur, et ça ne simplifie pas le code.
L'avantage d'une exception qui traverse tout le programme, c'est que ça n'a pas le poids de ce genre de technique. Par contre, ça demande que le programme soit capable de se nettoyer correctement, d'où que vienne l'exception, mais ce n'est pas très difficile à penser. Et mon programme est simple, je n'ouvre ni connexions réseau ni bdd, le code n'est pas critique et l'OS est là pour ramasser les miettes.

Je suppose que tu réponds à Lionel là? As-tu vu ma solution RAII?

Ah oui, je répondais à Lionel, je n'ai pas vu le cross. J'ai vu ta méthode RAII avec son template, j'ai pensé à quelque chose comme ça en effet, mais je suis en train d'écrire mon wrapper SDL pour les fonctions qui m'intéressent. Je ferai probablement quelqeue chose dans ce goû-là après.

D'ailleurs, une alternative au template avec le pointeur-sur-méthode serait un nettoyeur qui prend un lambda C++11.

Lionel Debroux (./21) :
Les structures de contrôle sans accolades sont connues pour avoir causé, par exemple, un bug de sécurité grave dans le kernel Linux il y a des années.

inux utilise la forme if (truc) machin;Parce que le kernel Lour être peu lisible. En écrivant if (truc) machin; qui est connue p tu n'as pas ce problème.

Tout dépends de la taille de "truc"

Oui, mais il y a aussi une règle sur la longueur des lignes. Et si ça dépasse une ligne, autant mettre les accolades

Bon, ben je suis en train de modifier mon source comme ça :
- un header de fonctions inline pour wrapper les fonctions SDL (mais inline marche-t-il toujours ? à vérifier !) : http://www.mirari.fr/7r3v
- des petites classes pour contenir temporairement des objets ou listes d'objets créés dans les constructeurs (ici, la classe TextureContainer) : http://www.mirari.fr/MgvL

Le containeur initialise son contenu à nullptr (merci C++11), et le libère automatiquement en cas de pépin. Mais si tout a bien marché, il le donne a l'objet réellement intéressé et l'oublie, pour ne pas le libérer par erreur.
Quand j'aurai fait plusieurs conteneurs, je verrai éventuellement comment templater ça, pour le moment je manque de recul.

Et au final, le code du ./2 devient ça : http://www.mirari.fr/liia

On note le gros progrès effectué dans la direction voulue, taille du code et lisibilité, merci encore à tous !