Necesito hacer que mi código sea más legible para los otros programadores de mi equipo

Estoy trabajando en un proyecto en Delphi y estoy creando un instalador para la aplicación, hay tres partes principales.

1. Instalación / desinstalación de PostgreSQL
2. instalación / desinstalación de myapplication (la configuración de myapplication se crea usando nsi).
3. Creación de tablas en Postgres a través de script (archivos por lotes).

Todo funciona bien y sin problemas, pero si algo falla, he creado un LogToFileger que registrará LogToFile en cada paso del proceso, de
esta manera

LogToFileToFile.LogToFile('[DatabaseInstallation]  :  [ACTION]:Postgres installation started');

La función LogToFileToFile.LogToFile()Esto escribirá el contenido en un archivo. Esto está funcionando bien, pero el problema es que ha estropeado el código ya que se ha vuelto difícil leer el código ya que uno solo puede ver la LogToFileToFile.LogToFile()llamada a la función en todas partes del código

un ejemplo

 if Not FileExists(SystemDrive+'\FileName.txt') then
 begin
    if CopyFile(PChar(FilePathBase+'FileName.txt'), PChar(SystemDrive+'\FileName.txt'), False) then
       LogToFileToFile.LogToFile('[DatabaseInstallation] :  copying FileName.txt to '+SystemDrive+'\ done')
       else
       LogToFileToFile.LogToFile('[DatabaseInstallation] :  copying FileName.txt to '+SystemDrive+'\ Failed');
 end;
 if Not FileExists(SystemDrive+'\SecondFileName.txt')      then
   begin
     if CopyFile(PChar(FilePathBase+'SecondFileName.txt'), PChar('c:\SecondFileName.txt'), False) then
       LogToFileToFile.LogToFile('[DatabaseInstallation] : copying SecondFileName.txt to '+SystemDrive+'\ done')
   else
       LogToFileToFile.LogToFile('[DatabaseInstallation] :  copying SecondFileName.txt to '+SystemDrive+'\ Failed');
 end;

como puedes ver hay muchas LogToFileToFile.LogToFile()llamadas,
antes de que fuera

 if Not FileExists(SystemDrive+'\FileName.txt') then
    CopyFile(PChar(FilePathBase+'FileName.txt'), PChar(SystemDrive+'\FileName.txt'), False) 
 if Not FileExists(SystemDrive+'\SecondFileName.txt')      then
   CopyFile(PChar(FilePathBase+'SecondFileName.txt'), PChar('c:\SecondFileName.txt'), False)

Este es el caso en todo mi código ahora.
Es difícil de leer.

¿Alguien puede sugerirme una buena manera de ordenar las llamadas a LogToFile?

me gusta

1. Sangría la llamada 'LogToFileToFile.LogToFile () `de
   esta manera

      if Not FileExists(SystemDrive+'\FileName.txt') then
        begin
            if CopyFile(PChar(FilePathBase+'FileName.txt'), PChar(SystemDrive+'\FileName.txt'), False) then
           {Far away--->>}                   LogToFileToFile.LogToFile(2,'[DatabaseInstallation] :  [ACTION]:copying FileName.txt to '+SystemDrive+'\ sucessful')
      else
           {Far away--->>}                   LogToFileToFile.LogToFile(2,'[DatabaseInstallation] :  [ACTION]:copying FileName.txt to '+SystemDrive+'\ Failed');
      end;
   

2. Unidad separada como LogToFileger
   Esta unidad tendrá todos los mensajes de LogToFile de switch caseesta manera

    Function LogToFilegingMyMessage(LogToFilegMessage : integer)
   
    begin
   case  LogToFilegMessage of
   
   1         :  LogToFileToFile.LogToFile(2,'[DatabaseInstallation] :  [ACTION]:copying FileName.txt to '+SystemDrive+'\ sucessful');
   2         :  LogToFileToFile.LogToFile(2,'[DatabaseInstallation] :  [ACTION]:copying FileName.txt to '+SystemDrive+'\ Failed');
      150        :  LogToFileToFile.LogToFile(2,'[somthing] :  [ACTION]: somthing important);
   
   end;
   

así que solo puedo llamar a LogToFilegingMyMessage (1) cuando sea necesario.

¿Alguien puede decirme cuál es un enfoque mejor y más limpio para LogToFileging de esta manera?

Cuando agregó el registro, introdujo dos cosas:

1. El código se hizo más grande porque para casi todas las acciones, agregó una línea que registra esa acción (o su falla)
2. Las líneas de registro parecen estar hinchadas y le quitan la legibilidad porque ocupan mucho espacio.

Cada uno de estos problemas tiene su propia solución relativamente simple:

1. Divide el código en funciones más pequeñas. En lugar de tener una función gigante que contenga todas sus copias, así como mensajes de registro para error / éxito, puede introducir una función "Copiar archivo", que copiaría exactamente un archivo y registra su propio resultado. De esa manera, su código principal consistiría en llamadas de CopyFile y sería fácil de leer.

2. Podría hacer que su registrador sea más inteligente. En lugar de pasar una cadena gigante que tiene mucha información repetitiva, puede pasar valores de enumeración que aclararían las cosas. O podría definir funciones Log () más especializadas, es decir, LogFileCopy, LogDbInsert ... Sea lo que sea que repita mucho, considere factorizarlo en su propia función.

Si sigue (1), podría tener un código similar al siguiente:

CopyFile( sOSDrive, 'Mapannotation.txt' )
CopyFile( sOSDrive, 'Mappoints.txt' )
CopyFile( sOSDrive, 'Mapsomethingelse.txt' )
. . . .

Luego, su CopyFile () solo necesita unas pocas líneas de código para realizar la acción y registrar su resultado, por lo que todo su código permanece conciso y fácil de leer.

Me mantendría alejado de su enfoque # 2, ya que está separando información que debería permanecer unida en diferentes módulos. Solo está solicitando que su código principal no esté sincronizado con sus declaraciones de registro. Pero mirando LogMyMessage (5), nunca lo sabrás.

ACTUALIZACIÓN (respuesta al comentario): no estoy familiarizado con el lenguaje exacto que está utilizando, por lo que es posible que esta parte deba adaptarse un poco. Parece que todos sus mensajes de registro identifican 3 cosas: componente, acción, resultado.

Creo que esto es más o menos lo que sugirió MainMa. En lugar de pasar una cadena real, defina constantes (en C / C ++ / C #, serían parte del tipo de enumeración de enumeración). Entonces, por ejemplo, para los componentes, es posible que tenga: DbInstall, AppFiles, Registry, Shortcuts ... Cualquier cosa que haga que el código sea más pequeño facilitará la lectura.

También ayudaría si su idioma admite el paso de parámetros variables, no estoy seguro de si eso es posible. Entonces, por ejemplo, si la acción es "FileCopy", podría definir esa acción para tener dos parámetros de usuario adicionales: nombre de archivo y directorio de destino.

Entonces sus líneas de copia de archivos se verían así:

Bool isSuccess = CopyFile(PChar(sTxtpath+'Mapannotation.txt'), PChar(sOSdrive+'\Mapannotation.txt'), False)
LogBook.Log( DbInstall, FileCopy, isSuccess, 'Mapannotation.txt', sOSDrive )

* nota, tampoco hay razón para copiar / pegar la línea de registro dos veces si puede almacenar el resultado de la operación en una variable local separada y simplemente pasar esa variable a Log ().

Ves el tema aquí, ¿verdad? Código menos repetitivo -> código más legible.

Parece que necesita resumir el concepto de "LoggableAction". Veo un patrón en su ejemplo donde todas las llamadas devuelven un bool para indicar éxito o fracaso y la única diferencia es el mensaje de registro.

Han pasado años desde que escribí Delphi, por lo que este es un pseudocódigo inspirado en C #, pero hubiera pensado que querías algo como

void LoggableAction(FunctionToCallPointer, string logMessage)
{
    if(!FunctionToCallPointer)
    {  
        Log(logMessage).
    }
}

Entonces su código de llamada se convierte

if Not FileExists(sOSdrive+'\Mapannotation.txt') then
    LoggableAction(CopyFile(PChar(sTxtpath+'Mapannotation.txt'), "Oops, it went wrong")

No recuerdo la sintaxis de Delphi para los punteros de función, pero sean cuales sean los detalles de la implementación, algún tipo de abstracción en torno a la rutina de registro parece ser lo que estás buscando.

Un enfoque posible es reducir el código mediante el uso de constantes.

if CopyFile(PChar(sTxtpath+'Mapannotation.txt'), PChar(sOSdrive+'\Mapannotation.txt'), False) then
   LogBook.Log(2,'[POSTGRESQL INSTALLATION] :  [ACTION]:copying Mapannotation.txt to '+sOSdrive+'\ sucessful')
   else
   LogBook.Log(2,'[POSTGRESQL INSTALLATION] :  [ACTION]:copying Mapannotation.txt to '+sOSdrive+'\ Failed');

se convertiría:

if CopyFile(PChar(sTxtpath+'Mapannotation.txt'), PChar(sOSdrive+'\Mapannotation.txt'), False) then
   Log(2, SqlInstal, Action, CopyMapSuccess, sOSdrive)
   else
   Log(2, SqlInstal, Action, CopyMapFailure, sOSdrive)

que tiene una mejor relación de código de registro / otro código al contar el número de caracteres en la pantalla.

Esto está cerca de lo que sugirió en el punto 2 de su pregunta, excepto que no iría tan lejos: Log(9257)obviamente es más corto que Log(2, SqlInstal, Action, CopyMapSuccess, sOSdrive), pero también bastante difícil de leer. ¿Qué es 9257? ¿Es un éxito? ¿Una acción? ¿Está relacionado con SQL? Si trabajó en esta base de código durante los últimos diez años, aprenderá esos números de memoria (si hay una lógica, es decir, 9xxx son códigos de éxito, x2xx están relacionados con SQL, etc.), pero para un desarrollador nuevo que descubre la base de código, los códigos cortos serán una pesadilla.

Puede ir más allá mezclando los dos enfoques: use una sola constante. Personalmente, no haría eso. O tus constantes crecerán en tamaño:

Log(Type2SuccessSqlInstallCopyMapSuccess, sOSdrive) // Can you read this? Really?

o las constantes permanecerán cortas, pero no muy explícitas:

Log(T2SSQ_CopyMapSuccess, sOSdrive) // What's T2? What's SSQ? Or is it S, followed by SQ?
// or
Log(CopyMapSuccess, sOSdrive) // Is it an action? Is it related to SQL?

Esto también tiene dos inconvenientes. Tendrás que:

• Mantenga una lista separada que asocie la información del registro a sus respectivas constantes. Con una sola constante, crecerá rápido.

• Encuentre una manera de imponer un formato único en su equipo. Por ejemplo, ¿qué pasa si en lugar de T2SSQalguien decidirá escribir ST2SQL?

Intente extraer una serie de pequeñas funciones para manejar todas las cosas de aspecto desordenado. Hay una gran cantidad de código repetido que podría hacerse fácilmente en un solo lugar. Por ejemplo:

procedure CopyIfFileDoesNotExist(filename: string);
var
   success: boolean;
begin
   if Not FileExists(sOSdrive+'\'+filename') then
   begin
      success := CopyFile(PChar(sTxtpath+filename), PChar(sOSdrive+filename), False);

      Log(filename, success);
   end;
end;

procedure Log(filename: string; isSuccess: boolean)
var
   state: string;
begin
   if isSuccess then
   begin
      state := 'success';
   end
   else
   begin
      state := 'failed';
   end;

   LogBook.Log(2,'[POSTGRESQL INSTALLATION] : [ACTION]:copying ' + filename + ' to '+sOSdrive+'\ ' + state);
end;

El truco es mirar cualquier duplicación en su código y encontrar formas de eliminarlo. Use muchos espacios en blanco y use el inicio / fin para su ventaja (más espacios en blanco y bloques de código fáciles de encontrar / plegar). Realmente no debería ser demasiado difícil. Estos métodos podrían ser parte de su registrador ... realmente depende de usted. Pero ese parece ser un buen lugar para comenzar.

Yo diría que la idea detrás de la opción 2 es la mejor. Sin embargo, creo que la dirección que tomaste empeora las cosas. El entero no significa nada. Cuando vea el código, verá que se está registrando algo, pero no sabe qué.

En cambio, haría algo como esto:

void logHelper(String phase, String message) {
   LogBook.Log(2, "[" + phase + "] :  [Action]: " + message);
}

Esto conserva la estructura del mensaje pero permite que su código sea flexible. Puede definir cadenas constantes según sea necesario para las fases y solo usarlas como parámetro de fase. Esto le permite poder realizar cambios en el texto real en un solo lugar y efectuar todo. El otro beneficio de la función auxiliar es que el texto importante está con el código (como si fuera un comentario), pero el texto que solo es importante para el archivo de registro se abstrae.

if (!FileExists(sOSdrive+'\Mapannotation.txt')) {
    if (CopyFile(PChar(sTxtpath+'Mapannotation.txt'), PChar(sOSdrive+'\Mapannotation.txt'), False)) {
       logHelper(POSTGRESQL, 'copying Mapannotation.txt to '+ sOSdrive +'\ sucessful')
    } else {
       logHelper(POSTGRESQL, 'copying Mapannotation.txt to '+ sOSdrive +'\ Failed');
    }
}

Esto no es algo que mencionaste en tu pregunta, pero me di cuenta de tu código. Tu sangría no es consistente. La primera vez que lo usa beginno está sangrado, pero la segunda sí. Haces algo similar con else. Yo diría que esto es mucho más importante que las líneas de registro. Cuando la sangría no es consistente, hace que sea difícil escanear el código y seguir el flujo. Muchas líneas de registro repetitivas son fáciles de filtrar al escanear.

¿Qué tal algo en esta línea:

LogBook.NewEntry( 2,'POSTGRESQL INSTALLATION', 'copying Mapannotation.txt to '+sOSdrive);

if CopyFile(PChar(sTxtpath+'Mapannotation.txt'), PChar(sOSdrive+'\Mapannotation.txt'), False) then
    LogBook.Success()
else
    LogBook.Failed();

El método NewEntry () construiría la línea de texto (incluida la adición de [&] alrededor de las entradas adecuadas) y lo mantendría en espera hasta que se invoquen los métodos success () o failure (), que añaden la línea con 'success' o 'fallo' y luego envía la línea al registro. También puede hacer otros métodos, como info () para cuando la entrada del registro es para algo que no sea éxito / fracaso.