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

Si vous souhaitez une évaluation du niveau de compétence en C++ de ce développeur, je dirais que cela démontre le bas de l'intermédiaire.

Le code fait le travail et ne contient pas d'erreurs évidentes, mais comme d'autres l'ont écrit, il existe de meilleures façons de le faire.

Answer 2

Je ne fournirai pas de code alternatif, car cela ne ferait que s'ajouter au code déjà fourni.

Mais, j'ai l'impression que quelque chose ne va pas avec le code.

Pour le prouver, j'énumérerai quelques points dans la fonction originale qui montrent que son développeur n'était pas un développeur C++ expérimenté, des points que vous devriez enquêter si vous avez besoin d'une implémentation propre :

• copy : les paramètres sont passés en copie au lieu de const-référence. C'est un gros NON NON en C++ lorsqu'on considère les objets.
• bug : Je suppose qu'il y a une erreur dans la partie "if(occurenceInd>0)". En lisant la documentation de CString sur MSDN, la méthode CString::Find retourne -1, et non 0 lorsque la recherche a échoué. Ce code me dit que si une virgule était le premier caractère, elle ne serait pas échappée, ce qui n'est probablement pas le but de la fonction.
• variable inutile : "continueLoop" n'est pas nécessaire. Remplacer le code "continueLoop = false" par "continue" et la ligne "while(continueLoop)" par "while(true)" est suffisant. Notez que, en continuant ce raisonnement, le codeur peut changer l'interne de la fonction (remplacer un do..while par un simple while)
• changement du type de retour : Probablement chipotage, mais je proposerais une fonction alternative qui, au lieu de retourner la chaîne de résultat, l'accepterait en référence (une copie de moins au retour), la fonction originale étant en ligne et appelant l'alternative.
• ajout de const chaque fois que possible : encore une fois, en chipotant : les deux paramètres "char" devraient être const, ne serait-ce que pour éviter de les modifier par accident.
• possible réallocation multiple : la fonction repose sur des réallocation potentielles multiples des données de CString. La solution de Josh d'utiliser la réserve de std::string est une bonne solution.
• utilisation de l'API CString complète : Mais contrairement à Josh, parce que vous semblez utiliser CString, j'éviterais la std::string et utiliserais CString::GetBufferSetLength et CString::ReleaseBuffer, ce qui me permet d'avoir le même code, avec une allocation d'objet en moins.
• Méthode Insert mystérieuse ? : C'est moi, ou il n'y a pas de CString::Insert ??? (voir http://msdn.microsoft.com/en-us/library/aa252634(VS.60).aspx). En fait, j'ai même échoué à trouver CString dans le même MSDN pour Visual C++ 2008 et 2005... Cela pourrait être parce que je devrais vraiment aller dormir, mais quand même, je pense que cela vaut la peine d'être investigué.

Answer 3

Est-ce que ce consultant est payé au mot de code? Plusieurs personnes ont souligné que la classe CString fournit déjà cette fonctionnalité, donc même si vous n'êtes pas programmeur, vous savez:

• La fonction est inutile. Elle ajoute à la complexité, la taille, et possiblement le temps d'exécution du programme.

• La fonction CString fonctionne probablement et est probablement efficace; celle-ci peut l'être ou non.

• La fonction CString est documentée, et est donc supportable.

• Le consultant est soit peu familier avec la fonction standard CString, soit a pensé qu'il/elle pourrait faire mieux en écrivant une nouvelle.

• On pourrait conclure que le consultant est peu familier avec les autres fonctionnalités standard et les meilleures pratiques.

• Choisir d'écrire un nouveau code pour une fonction de base, sans considérer qu'une version standard pourrait exister, est une pratique déconseillée acceptée.

Et peut-être le plus gros drapeau rouge de tous: vos instincts vous ont poussé à obtenir des opinions de la communauté StackOverflow.

Faites confiance à vos instincts.

Answer 4

Le code écrit fonctionnera en effet, car il renvoie la nouvelle chaîne. Il force simplement l'étape supplémentaire de définir ensuite la vieille chaîne égale à la chaîne retournée.

Comme d'autres l'ont dit, la fonctionnalité intégrée sur les chaînes est plus efficace et moins sujettes aux erreurs que de réinventer la roue.

Answer 5

A l'air correct, un peu, je ne sais pas, bidouillé. Mieux vaut utiliser la bibliothèque, mais je ne partirais pas en réécrire cette routine.