Fonction C++ simple - Ce code est-il "bon" ?

Question

Le code suivant a été produit par un consultant travaillant pour mon groupe. Je ne suis pas un développeur C++ (j'ai travaillé dans de nombreux langages cependant) mais j'aimerais avoir quelques opinions indépendantes sur le code suivant. Cela se trouve dans Visual Studio C++ 6.0. J'ai une réaction instinctive (pas une bonne, évidemment), mais j'aimerais avoir des "réactions instinctives" de développeurs C++ expérimentés (ou même pas si expérimentés que ça). Merci d'avance!

// Appel d'exemple
strColHeader = insert_escape(strColHeader, ',', '\\'); // Se débarrasser des virgules et en faire un caractère d'échappement

...snip...

CString insert_escape (CString originalString, char charFind, char charInsert) {
    bool continueLoop = true;   
    int currentInd = 0;

    do {
        int occurenceInd = originalString.Find(charFind, currentInd);

        if(occurenceInd>0) {
            originalString.Insert(occurenceInd, charInsert);
            currentInd = occurenceInd + 2; 
        }
        else {
            continueLoop = false;   
        }
    } while(continueLoop);
    return(originalString);
}

Answer 1

Hmm. Je pense

CString strColHeader;
strColHeader.Replace(",", "\\,") 

ferait tout aussi bien.

Je n'aime pas le code, j'ai tendance à sortir de la boucle while au lieu d'avoir un drapeau booléen 'continuer' inutile. Cela va encore plus loin lorsqu'il aurait pu utiliser while (occurenceInd != 0) comme variable de contrôle de boucle au lieu du booléen.

L'incrémentation du compteur repose également sur "+2" ce qui ne semble pas immédiatement compréhensible (pas au premier coup d'œil), et enfin (et surtout) il ne semble pas faire de commentaires.

Answer 2

Il y a un bug d'un décalage assis là en plein milieu : Regardez ce qui se passe si le premier caractère est une virgule : ",abc,def,ghi" : Je suppose que la sortie souhaitée serait "\,abc\,def\,ghi", mais à la place vous obtenez la chaîne originale :

int occurenceInd = originalString.Find(charFind, currentInd);

OccurrenceInd retourne 0, puisqu'il a trouvé charFind au premier caractère.

if(occurenceInd>0) 

0 n'est pas supérieur à 0, donc prenez la branche else et renvoyez la chaîne originale. CString::Find retourne -1 quand il ne peut pas trouver quelque chose, donc au moins cette comparaison devrait être :

if(occurrenceInd >= 0)

La meilleure façon serait d'utiliser la fonction Replace, mais si vous voulez le faire à la main, une meilleure implémentation ressemblerait probablement à ceci :

CString insert_escape ( const CString &originalString, char charFind, char charInsert ) {
    std::string escaped;
    // Réservez suffisamment d'espace pour chaque caractère à échapper
    escaped.reserve(originalString.GetLength() * 2); 
    for (int iOriginal = 0; iOriginal < originalString.GetLength(); ++iOriginal) {
        if (originalString[iOriginal] == charFind)
            escaped += charInsert;
        escaped += originalString[iOriginal];
    }
    return CString(escaped.c_str());
}

Answer 3

Les erreurs ont déjà été mentionnées. Mais elles me semblent être le genre de problèmes que tout le monde pourrait avoir avec du code rapidement écrit qui n'a pas été correctement testé, particulièrement s'ils ne sont pas familiers avec CString.

Je m'inquiéterais davantage des choses stylistiques, car elles suggèrent quelqu'un qui n'est pas à l'aise avec le C++. L'utilisation du booléen continueLoop est tout simplement du mauvais C++. Il représente un tiers du code de la fonction qui pourrait être éliminé en utilisant une simple structure if...break, rendant le code plus facile à suivre dans le processus.

De plus, le nom de variable "originalString" est très trompeur. Parce qu'ils le passent par valeur, ce n'est pas la chaîne originale, c'est une copie ! Ensuite, ils modifient la chaîne de toute façon, donc ce n'est plus le même objet ou la même chaîne de texte que l'original. Ce double mensonge suggère des schémas de pensée confus.

Answer 4

CString a une méthode Replace()... (c'était ma première réaction)

J'ai vu beaucoup de mauvais code, et bien pire que celui-ci. Cependant, ne pas utiliser les fonctionnalités intégrées quand il n'y a pas de bonne raison apparente de ne pas le faire est... pauvre.

Answer 5

Ma réaction instinctive est : WTF. Initialement par la façon dont le code est formaté (il y a beaucoup de choses que je n'aime pas concernant le formatage) et ensuite en examinant ce que le code fait réellement.

Il y a un grave problème avec la compréhension de la copie d'objets en C++ par ce développeur. L'exemple est un WTF en lui-même (si le développeur de la fonction a vraiment utilisé sa propre fonction de cette manière) :

// Appel d'exemple
strColHeader = insert_escape(strColHeader, ',', '\\'); // Se débarrasser des virgules et en faire un caractère d'échappement

CString insert_escape ( CString originalString, char charFind, char charInsert )

1. Passer une copie de strColHeader en tant que originalString (remarquez qu'il n'y a pas de &)
2. La fonction modifie cette copie (c'est bien)
3. La fonction renvoie une copie de la copie, qui à son tour remplace l'original strColHeader. Le compilateur optimisera probablement cela en une seule copie mais quand même, passer des copies d'objets de cette manière ne fonctionne pas pour C++. Il faut savoir utiliser des références.

Un développeur plus expérimenté aurait conçu cette fonction comme suit :

void insert_escape(CString &originalString, char charFind, char charInsert)

ou :

CString insert_escape(const CString &originalString, char charFind, char charInsert)

(Et probablement, aurait nommé les paramètres un peu différemment)

Et comme beaucoup l'ont souligné, la chose sensée que le développeur aurait pu faire était de vérifier la documentation de l'API pour voir si CString avait déjà une méthode Replace...