Función C++ simple: ¿este código es "bueno"?

Question

El siguiente código fue producido por un consultor que trabaja para mi grupo. No soy un desarrollador de C++ (aunque trabajé en muchos idiomas), pero me gustaría tener algunas opiniones independientes sobre el siguiente código. Esto es en Visual Studio C++ 6.0. Tengo una reacción visceral (no una buena, obviamente), pero me gustaría algunas "reacciones viscerales" de los desarrolladores de C++ experimentados (o incluso no tan desprovistos). ¡Gracias por adelantado!

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

... tijeretazo ...

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 tiene un método replace() ... (que fue mi primera reacción)

he visto un montón de mal código y mucho peor que esto Sin embargo, no usar funcionalidad incorporada cuando aparentemente no hay una buena razón para no hacerlo es ... pobre.

Answer 2

hmm. Creo que

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

lo harían igual de bien.

No me gusta el código, tiendo a romper el ciclo while en lugar de tener un indicador innecesario bool 'continuar'. Eso va doble cuando podría haber usado while (occurenceInd != 0) como su variable de control de bucle en lugar de booleano.

Incrementar el contador también depende de "+2" que no parece inmediatamente comprensible (no a primera vista), y por último (y lo más importante) parece que no hace comentarios.

Answer 3

El código escrito funcionará, ya que devuelve la nueva cadena. Solo está forzando el paso adicional de establecer la cadena anterior = a la cadena devuelta.

Como han dicho otros, la funcionalidad incorporada en cadenas es más eficiente y menos propensa a errores que la reinvención de la rueda.

Answer 4

Se ve bien, aunque sea un poco, no sé, piratear. Es mejor usar la biblioteca, pero no iría a reescribir esta rutina.

Answer 5

Esto es en Visual Studio C++ 6.0.

Reacción de tripas: blech. ¡Seriamente! Se sabe que el compilador de C++ incluido con VC++ 6 tiene errores y, en general, funciona muy mal y tiene 10 años.

@Downvoters: ¡considéralo! Quiero decir esto con toda seriedad. ¡VC6 es comparativamente improductivo y no debería usarse más! Especialmente desde que Microsoft suspendió su soporte para el software. Hay casos en que esto no se puede evitar, pero son raros. En la mayoría de los casos, una actualización de la base de código ahorra dinero. VC++ 6 simplemente no permite aprovechar el potencial de C++ que hace una herramienta objetivamente inferior.

Answer 6

Parece que las personas han abordado algunos de los aspectos de funcionalidad de este código para usted, pero le sugiero que se mantenga alejado de los nombres variables que ha empleado aquí.

Con la excepción de los controles de la interfaz de usuario, generalmente se desaprueba el uso de la notación húngara. Esto es más importante con los números ...por ejemplo:

Declaro:

flotador fMyNumber = 0,00;

Y luego lo uso en toda mi aplicación. Pero luego, más tarde, lo cambio a un doble porque me doy cuenta de que necesito más precisión. Ahora tengo:

double fMyNumber = 0.00;

Es cierto que la mayoría de las buenas herramientas de refactorización podrían solucionar esto, pero probablemente sea mejor no adjuntar esos prefijos. Son más comunes en algunos idiomas que en otros, pero desde una perspectiva de estilo general, debe tratar de evitarlos. A menos que esté usando el Bloc de notas, probablemente tenga algo parecido a Intellisense, por lo que realmente no necesita mirar el nombre de la variable para averiguar de qué tipo es.

Answer 7

Siempre hay una mejor implementación. Si está utilizando la función como un ejemplo de que el consultor no es muy bueno, también podría considerar que, si bien no conocían una función que ya existía, podrían tener experiencia y comprensión de la construcción del proyecto.

El desarrollo de software no se trata solo de la función perfecta sino también de cuán buena es la arquitectura de todo.

Answer 8

Si quieres una evaluación del nivel de habilidad C++ de este desarrollador, diría que esto demuestra el extremo inferior del intermedio.

El código hace el trabajo, y no contiene ningún "aullador" obvio, pero como otros han escrito, hay mejores formas de hacerlo.

Answer 9

Siempre me preocupa cuando veo un ciclo de hacer ... while; OMI, siempre son más difíciles de entender.

Answer 10

Hay un error fuera de serie allí en el medio: Mire lo que sucede si el primer carácter es una coma: ", abc, def, ghi": estoy asumiendo la salida deseada sería "\, abc \, \ def, ghi", pero en su lugar se obtiene la secuencia original de nuevo:

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

OccurrenceInd devuelve 0, ya que se encontró charFind en el primer carácter.

if(occurenceInd>0) 

0 no es mayor que 0, por lo tanto, tome la rama else y devuelva la cadena original. CString::Find devuelve -1 cuando no puede encontrar algo, así que al menos que la comparación debe ser:

if(occurrenceInd >= 0) 

La mejor manera sería utilizar la función Reemplazar, pero si quiere hacerlo a mano, una mejor aplicación, probablemente se vería algo como esto:

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

Mi reacción visceral es: WTF. Inicialmente por la forma en que se formatea el código (hay muchas cosas que no me gustan sobre el formato) y luego por el examen de lo que el código realmente está haciendo.

Hay un problema grave con la comprensión de este desarrollador de la copia de objetos en C++.El ejemplo es un WTF en sí mismo (si el desarrollador de la función realmente usó su/su propia función como esta):

// 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. pasar una copia de strColHeader como originalString (nótese que no hay &)
2. la función modificado esta copia (fino)
3. la función devuelve una copia de la copia, que a su vez reemplaza el original strColHeader. El compilador probablemente optimizará esto en una sola copia, pero aún así, pasar copias de objetos como esta no funciona para C++. Uno debe saber sobre referencias.

Un desarrollador más experimentado habría diseñado esta función como:

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

o:

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

(Y probablemente habría llamado los parámetros de forma un poco diferente)

Y como muchos han señalado, lo más sensato que el desarrollador podría haber hecho es verificar la documentación API para ver si CString ya tenía un método Replace ...

Answer 12

Ya se han mencionado los errores. Pero me parecen el tipo de problemas que cualquiera podría tener con el código acelerado que no se ha probado correctamente, especialmente si no están familiarizados con CString.

Me preocuparía más sobre las cosas estilísticas, ya que sugieren que alguien que no se siente cómodo con C++. El uso de bool continueLoop es simplemente malo C++. Representa un tercio del código de la función que podría eliminarse mediante el uso de una construcción simple ... break, haciendo que el código sea más fácil de seguir en el proceso.

Además, el nombre de la variable "originalString" es muy engañoso. Porque lo pasan por valor, no es la cadena original, ¡es una copia! Luego, modifican la cadena de todos modos, por lo que ya no es el mismo objeto o la misma cadena de texto que el original. Esta doble mentira sugiere patrones de pensamiento confusos.

Answer 13

No proporcionaré un código alternativo, ya que solo agregaría al código ya proporcionado.

Pero, mi intuición es que hay algo mal con el código.

Para demostrarlo, voy a enumerar algunos puntos de la función original que muestra su desarrollador no era un experimentado desarrollador de C++, señala que se debe investigar si necesita una aplicación limpia:

• copia: los parámetros se pasan como copia en lugar de const-reference. Este es un gran NO NO en C++ al considerar objetos.
• error Supongo que hay un error en la parte "if (occurenceInd> 0)". Al leer el documento de CString en MSDN, el método CString :: Find devuelve -1 y no 0 cuando falla la búsqueda.Este código me dice si una coma era el primer carácter, no se escaparía, que probablemente no es el objetivo de la función
• variable innecesaria: "continueLoop" no es necesario. Reemplazar el código "continueLoop = false" por "continuar" y la línea "while (continueLoop)" por "while (true)" es suficiente. Tenga en cuenta que, al continuar este razonamiento, habilite el codificador para cambiar las funciones internas (reemplazando un do ... mientras por un tiempo simple)
• cambiando el tipo de retorno: Probablemente escogiendo los detalles, pero ofrecería una función alternativa que, en lugar de devolver la cadena de resultados, se aceptará como referencia (una copia menos a la vuelta), la función original se inline y se invoca la alternativa.
• agregando const siempre que sea posible de nuevo, escogiendo en detalle: los dos parámetros "char" deben ser const, aunque solo sea para evitar modificarlos por accidente.
• posible reasignación múltiple la función se basa en posibles reasignaciones múltiples de los datos de CString. La solución de Josh de usar la reserva de std :: string es buena.
• usando completamente la API de CString: Pero a diferencia de Josh, porque parece que usa CString, evitaría std :: string y usaría CString :: GetBufferSetLength y CString :: ReleaseBuffer, lo que me permitiría tener el mismo código, con una asignación de objeto menos.
• Mysterious Insertar método? soy yo, o no hay CString :: Insert ??? (ver http://msdn.microsoft.com/en-us/library/aa252634(VS.60).aspx). De hecho, incluso no encontré CString en la misma MSDN para Visual C++ 2008 y 2005 ... Esto podría ser porque yo realmente ir a dormir, pero aún así, creo que esto vale la pena investigar

Answer 14

¿Este consultor está siendo pagado por la línea de código? Varias personas han señalado que la clase CString ya proporciona esta funcionalidad, por lo que incluso si usted no es un programador, ya sabes:

• La función es innecesaria. Se agrega complejidad, tamaño y posiblemente el tiempo de ejecución del programa.
• La función CString probablemente funciona y es probablemente eficiente; este puede o no ser
• La función CString está documentada y, por lo tanto, es compatible.
• El consultor no está familiarizado con la función estándar CString o pensó que podría hacerlo mejor escribiendo una nueva.
  • Se puede concluir que el consultor no está familiarizado con otras características estándar y mejores prácticas.
  • Elegir escribir código nuevo para una función básica, sin considerar que puede existir una versión estándar, es una mala práctica aceptada.

Y quizás el más grande, más rojo de la bandera de todos: sus instintos le empujaron a obtener opiniones de la comunidad StackOverflow.

Confíe en sus instintos.