Einfache C++ - Funktion - Ist dieser Code "gut"?

Question

Der folgende Code wurde von einem für meine Gruppe arbeitenden Berater erstellt. Ich bin kein C++ - Entwickler (arbeitete in vielen Sprachen), hätte aber gerne eine unabhängige Meinung zum folgenden Code. Dies ist in Visual Studio C++ 6.0. Ich habe eine Bauchreaktion (offensichtlich keine gute), aber ich würde gerne ein paar "Bauchreaktionen" von erfahrenen C++ - Entwicklern (oder gar nicht so ausgereiften) bekommen. Danke im Voraus!

// Example call 
strColHeader = insert_escape(strColHeader, ',', '\\'); //Get rid of the commas and make it an escape character 

... schnipp ...

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

CString eine Methode Replace() hat ... (das war meine erste Reaktion)

Ich habe eine Menge von schlechten Code gesehen und viel schlimmer als das. Allerdings keine eingebaute Funktionalität, wenn es keinen ersichtlichen Grund gibt, nicht zu sein, ist ... schlecht.

Answer 2

hmm. Ich denke,

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

würde genauso gut tun.

Ich mag den Code nicht, ich neige dazu, aus der While-Schleife zu brechen, anstatt eine unnötige Bool 'Continue' Flag zu haben. Das geht doppelt, wenn er while (occurenceInd != 0) als seine Schleifensteuerungsvariable anstelle des Booleschen hätte verwenden können.

Die Erhöhung des Zählers beruht auch auf "+2", was nicht sofort verständlich erscheint (nicht auf einen Blick), und zuletzt (und am wichtigsten) scheint er keine Kommentare zu machen.

Answer 3

Der geschriebene Code wird tatsächlich funktionieren, da er die neue Zeichenfolge zurückgibt. Es erzwingt nur den zusätzlichen Schritt, die alte Zeichenfolge = auf die zurückgegebene Zeichenfolge zu setzen.

Wie andere gesagt haben, ist die eingebaute Funktionalität von Strings effizienter und weniger fehleranfällig als das Rad neu zu erfinden.

Answer 4

Sieht gut aus, wenn ein bisschen, ich weiß nicht, hacken. Besser, die Bibliothek zu benutzen, aber ich würde diese Routine nicht umschreiben.

Answer 5

Dies ist in Visual Studio C++ 6.0.

Gut Reaktion: blech. Ernst! Der C++ - Compiler, der mit VC++ 6 ausgeliefert wird, ist bekanntlich fehlerhaft und läuft im Allgemeinen sehr schlecht und ist 10 Jahre alt.

@Downvoters: Betrachten Sie es! Ich meine das ernst. VC6 ist nur vergleichsweise unproduktiv und sollte nicht mehr verwendet werden! Vor allem seit Microsoft die Unterstützung für die Software eingestellt hat. Es gibt Fälle, in denen dies nicht vermieden werden kann, aber sie sind selten. In den meisten Fällen spart ein Upgrade der Codebasis Geld. VC++ 6 erlaubt es einfach nicht, das Potenzial von C++ zu nutzen, das ein objektiv minderwertiges Werkzeug darstellt.

Answer 6

Sieht so aus, als hätten die Leute einige der Funktionsaspekte dieses Codes für Sie in Angriff genommen, aber ich würde vorschlagen, sich von Variablennamen zu entfernen, wie Sie sie hier verwendet haben.

Mit Ausnahme von UI-Steuerelementen ist es in der Regel verpönt, ungarische Notationen zu verwenden. Dies ist wichtiger mit Zahlen ...zum Beispiel:

Ich erkläre:

float fMyNumber = 0,00;

Und dann benutze ich das in meiner gesamten Anwendung. Aber später ändere ich das zu einem Doppel, weil mir klar wird, dass ich mehr Präzision brauche. Jetzt habe ich:

doppelt fMyNumber = 0.00;

Es ist wahr, dass die meisten guten Refactoring-Tools dies für Sie beheben können, aber es ist wahrscheinlich am besten, diese Präfixe nicht anzuhängen. Sie sind in einigen Sprachen häufiger anzutreffen als in anderen, aber aus einer allgemeinen Stilperspektive sollten Sie versuchen, sie zu vermeiden. Wenn Sie nicht mit dem Editor arbeiten, haben Sie wahrscheinlich etwas mit Intellisense zu tun, so dass Sie den Variablennamen nicht unbedingt sehen müssen, um herauszufinden, um welchen Typ es sich handelt.

Answer 7

Es gibt immer eine bessere Implementierung. Wenn Sie die Funktion als Beispiel dafür verwenden, dass der Berater nicht sehr gut ist, möchten Sie vielleicht auch berücksichtigen, dass sie, obwohl sie eine bereits vorhandene Funktion nicht kannten, Erfahrung und Verständnis für die Projektkonstruktion haben könnten.

Bei der Softwareentwicklung geht es nicht nur um die perfekte Funktion, sondern auch darum, wie gut die Architektur des Ganzen ist.

Answer 8

Wenn Sie eine Bewertung des C++ - Fähigkeitslevels dieses Entwicklers wünschen, würde ich sagen, dass dies das untere Ende der Zwischenstufe zeigt.

Der Code erledigt die Arbeit und enthält keine offensichtlichen "Heuler", aber wie andere geschrieben haben, gibt es bessere Möglichkeiten, dies zu tun.

Answer 9

Ich mache mir immer Sorgen, wenn ich eine Do .. while-Schleife sehe; IMO, sie sind immer schwerer zu verstehen.

Answer 10

In der Mitte ist ein Fehler, der nebenbei sitzt: Schau dir an, was passiert, wenn das erste Zeichen ein Komma ist: ", abc, def, ghi": Ich nehme die gewünschte Ausgabe an wäre "\ abc \, \ def, ghi", sondern erhalten Sie die ursprüngliche Zeichenfolge zurück:

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

OccurrenceInd 0 zurück, da es charFind beim ersten Zeichen gefunden.

0 ist nicht größer als 0, also nehmen Sie den Else-Zweig und geben Sie die ursprüngliche Zeichenfolge zurück. CString::Find -1 zurück, wenn es nicht etwas finden kann, so zumindest, dass der Vergleich sein sollte:

if(occurrenceInd >= 0) 

Der beste Weg wäre, um die Funktion Ersetzen zu verwenden, aber wenn Sie wollen, dass es von Hand machen, etwa so aussehen wahrscheinlich eine bessere Implementierung würde:

CString insert_escape (const CString &originalString, char charFind, char charInsert) { 
    std::string escaped; 
    // Reserve enough space for each character to be escaped 
    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 11

Mein Bauch Reaktion ist: WTF. Zunächst einmal, wie der Code formatiert ist (es gibt viele Dinge, die ich an der Formatierung nicht mag) und dann durch die Untersuchung, was der Code tatsächlich macht.

Es gibt ein schwerwiegendes Problem mit dem Verständnis dieses Entwicklers von Objektkopieren in C++.Das Beispiel ist ein WTF in sich selbst (wenn der Entwickler der Funktion wirklich seine/ihre eigene Funktion wie folgt verwendet):

// Example call 
strColHeader = insert_escape(strColHeader, ',', '\\'); //Get rid of the commas and make it an escape character 

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

1. Pass eine Kopie von strColHeader als originalString (Hinweis, dass es keine &)
2. die Funktion dieser Kopie (fein)
3. die Funktion gibt einen Kopie der Kopie verändert, was wiederum ersetzt die ursprüngliche strColHeader. Der Compiler wird dies wahrscheinlich auf eine einzige Kopie optimieren, aber die Weitergabe von Objektkopien wie dieser funktioniert nicht für C++. Man sollte über Referenzen wissen.

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

   oder:

Ein erfahrener Entwickler würde diese Funktion als entworfen haben

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

(und wahrscheinlich auch die Parameter etwas anders genannt hätte)

Und Viele haben darauf hingewiesen, dass das vernünftigste, was der Entwickler hätte tun können, wäre, die API-Dokumentation zu überprüfen, um zu sehen, ob CString hatte bereits eine Replace Methode ...

Answer 12

Die Fehler wurden bereits erwähnt. Aber sie erscheinen mir als die Art von Problemen, die jemand mit schnell ausgeschalteten Codes haben könnte, die nicht richtig getestet wurden, besonders wenn sie nicht mit CString vertraut sind.

Ich würde mir mehr Sorgen um die stilistischen Dinge machen, da sie jemanden vorschlagen, der nicht mit C++ vertraut ist. Die Verwendung des bool continueLoop ist einfach nur schlechtes C++. Sie stellt ein Drittel des Funktionscodes dar, der durch ein einfaches if ... break-Konstrukt eliminiert werden könnte, wodurch der Code leichter nachvollziehbar wird.

Auch der Variablenname "originalString" ist sehr irreführend. Weil sie es nach Wert übergeben, ist es nicht die ursprüngliche Zeichenfolge, es ist eine Kopie davon! Dann ändern sie die Zeichenfolge auf jeden Fall, so dass es nicht mehr dasselbe Objekt oder dieselbe Textzeichenfolge wie das Original ist. Diese doppelte Lüge deutet auf verwirrte Denkmuster hin.

Answer 13

Ich werde keinen alternativen Code zur Verfügung stellen, da dieser nur dem bereits bereitgestellten Code hinzugefügt würde.

Aber mein Bauchgefühl ist, dass etwas mit dem Code nicht stimmt.

es zu beweisen, werde ich einige Punkte in der ursprünglichen Funktion aufzuzählen, die seine Entwickler zeigt war kein erfahrener C++ Entwickler, zeigt, dass Sie, wenn Sie eine saubere Implementierung benötigen untersuchen sollte:

• Kopie: Parameter werden als Kopie anstelle von const-reference übergeben. Dies ist ein großes NEIN in C++, wenn Objekte betrachtet werden.
• Fehler Ich denke, es gibt einen Fehler im "if (occurenceInd> 0)" Teil. Beim Lesen des CString-Dokuments in MSDN gibt die CString :: Find-Methode -1 zurück, und nicht 0, wenn die Suche fehlgeschlagen ist.Dieser Code sagt mir, wenn ein Komma das erste Zeichen war, würde es nicht maskiert werden, was wahrscheinlich nicht der Punkt der Funktion ist
• nicht benötigte Variable: "continueLoop" wird nicht benötigt. Ersetze den Code "continueLoop = false" durch "continue" und die Zeile "while (continueLoop)" durch "while (true)" ist ausreichend. Beachten Sie, dass das Fortsetzen dieser Argumentation es dem Coder ermöglicht, die Funktionen intern zu ändern (Ersetzen einer Funktion ... während einer einfachen Weile)
• wechselnder Rückgabetyp: Wahrscheinlich Kommissionierung bei Details, aber ich würde eine alternative Funktion anbieten, die Anstatt die Ergebniszeichenfolge zurückzugeben, würde akzeptieren, dass es eine Referenz ist (eine Kopie weniger bei Rückgabe), wobei die ursprüngliche Funktion inline ist und die Alternative aufruft.
• hinzufügen const wann immer möglich wieder, im Detail auswählen: die beiden "char" -Parameter sollten const sein, wenn auch nur um zu vermeiden, sie durch Zufall zu ändern.
• mögliche mehrfache Neuzuweisung Die Funktion stützt sich auf mögliche mehrfache Neuzuweisungen der CString-Daten. Joshs Lösung, die Reserve von std :: string zu verwenden, ist eine gute Lösung.
• Verwendung vollständig CString API: Aber im Gegensatz zu Josh, weil Sie CString zu verwenden scheinen, würde ich vermeiden, die std :: string und CString :: GetBufferSetLength und CString :: ReleaseBuffer verwenden, die mir den gleichen Code haben, mit einer weniger Objektzuordnung.
• Mysteriöse Insert-Methode? ich bin es, oder es gibt keinen CString :: Insert ??? (siehe http://msdn.microsoft.com/en-us/library/aa252634(VS.60).aspx). In der Tat, ich nicht sogar CString in der gleichen MSDN für Visual C++ 2008 und 2005 zu finden ... Dies könnte, weil ich soll wirklich schlafen gehen, aber immer noch, ich denke, das wert ist, zu untersuchen

Answer 14

Wird dieser Berater nach der Codezeile bezahlt? Mehrere Leute haben darauf hingewiesen, dass die CString Klasse bereits diese Funktionalität bereitstellt, so dass selbst wenn Sie kein Programmierer sind, dann wissen Sie:

• Die Funktion nicht erforderlich ist. Dies erhöht die Komplexität, Größe und möglicherweise die Ausführungszeit des Programms. Die CString Funktion funktioniert wahrscheinlich und ist wahrscheinlich effizient; dieser kann oder darf nicht sein. Die CString-Funktion ist dokumentiert und daher unterstützbar
• Der Berater ist entweder mit der Standardfunktion CString nicht vertraut oder dachte, er/sie könnte es besser machen, indem er eine neue schreibt.
  • Daraus könnte man schließen, dass der Berater mit anderen Standardfunktionen und Best Practices nicht vertraut ist.
  • Die Entscheidung, neuen Code für ein grundlegendes Feature zu schreiben, ohne zu berücksichtigen, dass eine Standardversion existiert, ist eine akzeptierte schlechte Praxis.

Und vielleicht die größte, rötesten Flagge von allen: Ihre Instinkte Sie stupste Meinungen aus der Community Stackoverflow zu erhalten.

Vertrauen Sie Ihren Instinkten.