Devi rendere il mio codice più leggibile agli altri programmatori della mia squadra

Quando hai aggiunto la registrazione, hai introdotto due elementi:

Il codice
1. è diventato più grande perché per quasi tutte le azioni è stata aggiunta una riga che registra quell'azione (o il suo errore)
2. Le linee di log stesse sembrano gonfiate e tolgono la leggibilità perché occupano molto spazio.

Ognuno di questi ha problemi ha una sua soluzione relativamente semplice:

1. Rompere il codice in funzioni più piccole. Invece di avere una funzione gigante che contiene tutte le tue copie e anche i messaggi di log per errori / successi, potresti introdurre una funzione "CopyFile", che copierebbe esattamente un file e registra il proprio risultato. In questo modo il tuo codice principale consisterebbe semplicemente in chiamate CopyFile e resterebbe di facile lettura.

2. Potresti rendere più intelligente il tuo logger. Invece di passare una stringa gigante che ha molte informazioni ripetitive, potresti passare valori di enumerazione che renderebbero le cose più chiare. Oppure puoi definire funzioni Log () più specializzate, ad esempio LogFileCopy, LogDbInsert ... Qualsiasi cosa tu ripeta, considera la possibilità di includerla nella sua funzione.

Se segui (1), potresti avere un codice simile al seguente:

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

Quindi il tuo CopyFile () ha solo bisogno di poche righe di codice per eseguire l'azione e registrare il risultato, quindi tutto il tuo codice rimane conciso e facile da leggere.

Starei lontano dal tuo approccio n. 2 mentre stai staccando le informazioni che dovrebbero rimanere insieme in moduli diversi. Stai solo chiedendo che il tuo codice principale non si sincronizzi con le tue dichiarazioni di registro. Ma guardando LogMyMessage (5), non lo saprai mai.

UPDATE (risposta al commento): Non ho familiarità con il linguaggio esatto che stai usando, quindi questa parte potrebbe dover essere adattata un po '. Sembra che tutti i tuoi messaggi di log identificano 3 elementi: componente, azione, risultato.

Penso che questo sia più o meno quello che MainMa suggeriva. Invece di passare la stringa effettiva, definire le costanti (in C / C ++ / C #, sarebbero parte del tipo di enumerazione enum). Quindi, ad esempio per i componenti, potresti avere: DbInstall, AppFile, Registro di sistema, scorciatoie ... Tutto ciò che rende il codice più piccolo lo renderà più facile da leggere.

Aiuterebbe anche se la tua lingua supporta il passaggio di parametri variabili, non è sicuro se ciò è possibile. Ad esempio, se l'azione è "FileCopy", è possibile definire quell'azione per avere due ulteriori parametri utente: nome file e directory di destinazione.

Quindi le tue linee di copiatura dei file sarebbero simili a questa:

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

* nota, non vi è alcun motivo per copiare / incollare la riga di registro due volte se è possibile memorizzare il risultato dell'operazione in una variabile locale separata e semplicemente passare tale variabile in Log ().

Vedi il tema qui, giusto? Codice meno ripetitivo - > codice più leggibile.

Sembra che sia necessario astrarre il concetto di "LoggableAction". Vedo un modello nell'esempio in cui tutte le chiamate restituiscono un valore bool per indicare il successo o l'errore e l'unica differenza è il messaggio di registro.

Sono passati anni da quando ho scritto delphi, quindi questo è praticamente uno pseudo-codice ispirato al c # ma avrei pensato che volessi qualcosa di simile

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

Quindi il tuo codice chiamante diventa

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

Non riesco a ricordare la sintassi di Delphi per i puntatori di funzione, ma qualunque sia il dettaglio dell'implementazione, una sorta di astrazione attorno alla routine di log sembra essere quello che stai cercando.

Un possibile approccio è ridurre il codice usando le costanti.

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');

diventerebbe:

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)

che ha un migliore codice di log / altro codice nel conteggio del numero di caratteri sullo schermo.

Questo è vicino a ciò che hai suggerito nel punto 2 della tua domanda, tranne che non andrei così lontano: Log(9257) è ovviamente più corto di Log(2, SqlInstal, Action, CopyMapSuccess, sOSdrive) , ma anche abbastanza difficile da leggere. Che cos'è 9257? È un successo? Un'azione? È collegato a SQL? Se lavori su questo codebase negli ultimi dieci anni, imparerai quei numeri a memoria (se c'è una logica, cioè 9xxx sono i codici di successo, x2xx sono legati a SQL, ecc.), Ma per un nuovo sviluppatore che scopre il codebase, i codici brevi saranno un incubo.

Puoi andare oltre mescolando i due approcci: usa una singola costante. Personalmente, non lo farei. O le tue costanti cresceranno di dimensioni:

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

o le costanti rimarranno brevi, ma non molto esplicite:

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?

Questo ha anche due inconvenienti. Dovrai:

• Mantieni una lista separata che associa le informazioni di registro alle rispettive costanti. Con una singola costante, crescerà rapidamente.

• Trova un modo per applicare un singolo formato al tuo team. Ad esempio, e se invece di T2SSQ , qualcuno deciderà di scrivere ST2SQL ?

Prova ad estrarre una serie di piccole funzioni per gestire tutte le cose dall'aspetto disordinato. C'è un sacco di codice ripetuto che potrebbe facilmente essere fatto in un unico posto. Ad esempio:

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;

Il trucco consiste nell'osservare qualsiasi duplicazione nel codice e trovare i modi per rimuoverlo. Usa un sacco di spazi bianchi e usa l'inizio / la fine a tuo vantaggio (più spazi bianchi e blocchi di codice facili da trovare / piegare). In realtà non dovrebbe essere troppo difficile. Questi metodi potrebbero far parte del tuo logger ... dipende da te davvero. Ma sembra un buon punto di partenza.

Direi che l'idea alla base dell'opzione 2 è la migliore. Tuttavia, penso che la direzione che hai preso peggiori le cose. Il numero intero non significa nulla. Quando guardi il codice, vedrai che qualcosa è stato registrato, ma non sai cosa.

Invece farei qualcosa del genere:

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

Ciò mantiene la struttura del messaggio ma consente al tuo codice di essere flessibile. È possibile definire stringhe costanti come necessario per le fasi e utilizzarle solo come parametro di fase. Questo ti permette di essere in grado di modificare il testo attuale in un unico punto ed effettuare tutto. L'altro vantaggio della funzione di aiuto è che il testo importante è con il codice (come se fosse un commento), ma il testo che è importante solo per il file di registro è astratto.

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');
    }
}

Questo non è qualcosa che hai menzionato nella tua domanda, ma ho notato il tuo codice. La tua indentazione non è coerente. La prima volta che usi begin non è indentato, ma è la seconda volta. Fai una cosa simile con else . Direi che è molto più importante delle linee di registro. Quando il rientro non è coerente, rende difficile la scansione del codice e segue il flusso. Un sacco di righe di registro ripetitive sono facili da filtrare durante la scansione.

Che ne dici di qualcosa lungo questa linea:

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();

Il metodo NewEntry () costruisce la linea di testo (incluso l'aggiunta di [&] attorno alle voci corrette) e lo mantiene in attesa finché non vengono chiamati i metodi success () o failure (), che aggiungono la riga a "success" o "failure", quindi inviare la riga al log. Puoi anche fare altri metodi, come info () per quando la voce di registro è per qualcosa di diverso da successo / fallimento.