Besoin de rendre mon code plus lisible pour les autres programmeurs de mon équipe

Je travaille un projet à delphi et je crée un installateur pour l'application, il y a trois parties principales.

1. Installation / désinstallation de PostgreSQL
2. myapplication (la configuration de myapplication est créée à l'aide de nsi) installation / désinstallation.
3. Création de tables dans Postgres via script (fichiers batch).

Tout se passe bien et sans problème, mais si quelque chose échoue, j'ai créé un LogToFileger qui enregistrera chaque étape du processus,
comme ceci

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

La fonction LogToFileToFile.LogToFile()Ceci écrit le contenu dans un fichier. Cela fonctionne bien, mais le problème est que cela a foiré le code car il est devenu difficile de lire le code car on ne voit que l' LogToFileToFile.LogToFile()appel de fonction partout dans le code

un exemple

 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;

comme vous pouvez le voir, il y a eu beaucoup d' LogToFileToFile.LogToFile()appels
avant

 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)

c'est le cas dans tout mon code maintenant.
c'est difficile à lire.

quelqu'un peut-il me suggérer une bonne façon de désencombrer les appels à LogToFile?

comme

1. Indenter l'appel «LogToFileToFile.LogToFile ()`
   comme ceci

      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. Unité distincte comme LogToFileger
   Cette unité aura tous les messages LogToFile dans un switch casecomme celui-ci

    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;
   

donc je peux simplement appeler LogToFilegingMyMessage (1) là où cela est nécessaire.

Quelqu'un peut-il me dire quelle est la meilleure et plus propre approche de LogToFileging de cette façon?

Lorsque vous avez ajouté la journalisation, vous avez introduit deux choses:

1. Le code est devenu plus volumineux parce que pour presque chaque action, vous avez ajouté une ligne qui enregistre cette action (ou son échec)
2. Les lignes de journal elles-mêmes semblent gonflées et nuisent à la lisibilité car elles occupent tellement de place.

Chacun de ces problèmes a sa propre solution, relativement simple:

1. Divisez le code en fonctions plus petites. Au lieu d'avoir une fonction géante qui contient toutes vos copies ainsi que des messages de journal pour les erreurs / succès, vous pouvez introduire une fonction "CopyFile", qui copierait exactement un fichier et enregistre son propre résultat. De cette façon, votre code principal serait simplement composé d'appels CopyFile et resterait facile à lire.

2. Vous pourriez rendre votre enregistreur plus intelligent. Au lieu de passer dans une chaîne géante qui contient beaucoup d'informations répétitives, vous pouvez passer des valeurs d'énumération qui rendraient les choses plus claires. Ou vous pouvez définir des fonctions Log () plus spécialisées, c'est-à-dire LogFileCopy, LogDbInsert ... Quoi que vous répétiez souvent, envisagez de le prendre en compte dans sa propre fonction.

Si vous suivez (1), vous pourriez avoir un code qui ressemble à ceci:

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

Ensuite, votre CopyFile () n'a besoin que de quelques lignes de code pour effectuer l'action et enregistrer son résultat, de sorte que tout votre code reste concis et facile à lire.

Je resterais loin de votre approche # 2 car vous détachez des informations qui devraient rester ensemble dans différents modules. Vous demandez simplement que votre code principal ne soit plus synchronisé avec vos instructions de journal. Mais en regardant LogMyMessage (5), vous ne le saurez jamais.

MISE À JOUR (réponse au commentaire): Je ne connais pas la langue exacte que vous utilisez, donc cette partie devra peut-être être adaptée un peu. Il semble que tous vos messages de journal identifient 3 choses: composante, action, résultat.

Je pense que c'est à peu près ce que MainMa a suggéré. Au lieu de passer une chaîne réelle, définissez des constantes (en C / C ++ / C #, elles feraient partie du type d'énumération énumération). Ainsi, par exemple pour les composants, vous pourriez avoir: DbInstall, AppFiles, Registry, Shortcuts ... Tout ce qui réduit le code le rendra plus facile à lire.

Il serait également utile que votre langue prenne en charge le passage des paramètres variables, vous ne savez pas si c'est possible. Par exemple, si l'action est "FileCopy", vous pouvez définir cette action pour avoir deux paramètres utilisateur supplémentaires: le nom de fichier et le répertoire de destination.

Ainsi, vos lignes de copie de fichiers ressembleraient à ceci:

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

* Remarque, il n'y a également aucune raison de copier / coller la ligne de journal deux fois si vous pouvez stocker le résultat de l'opération dans une variable locale distincte et simplement passer cette variable dans Log ().

Vous voyez le thème ici, non? Code moins répétitif -> code plus lisible.

Il semble que vous ayez besoin d'abstraire le concept d'une "action logique". Je vois un modèle dans votre exemple où tous les appels renvoient un booléen pour indiquer le succès ou l'échec et la seule différence est le message de journal.

Cela fait des années que j'ai écrit delphi donc c'est à peu près un pseudo-code inspiré de c # mais j'aurais pensé que vous vouliez quelque chose comme

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

Ensuite, votre code d'appel devient

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

Je ne me souviens pas de la syntaxe Delphi pour les pointeurs de fonction, mais quels que soient les détails de l'implémentation, une sorte d'abstraction autour de la routine de journal semble être ce que vous recherchez.

Une approche possible consiste à réduire le code en utilisant des 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');

deviendrait:

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)

qui a un meilleur rapport code de connexion / autre code lors du comptage du nombre de caractères à l'écran.

Ceci est proche de ce que vous avez suggéré dans le point 2 de votre question, sauf que je n'irais pas aussi loin: Log(9257)est évidemment plus court que Log(2, SqlInstal, Action, CopyMapSuccess, sOSdrive), mais aussi assez difficile à lire. Qu'est-ce que 9257? Est-ce un succès? Une action? Est-ce lié à SQL? Si vous travaillez sur cette base de code au cours des dix dernières années, vous apprendrez ces chiffres par cœur (s'il y a une logique, c'est-à-dire que 9xxx sont des codes de réussite, x2xx sont liés à SQL, etc.), mais pour un nouveau développeur qui découvre la base de code, les codes courts seront un cauchemar.

Vous pouvez aller plus loin en mélangeant les deux approches: utilisez une seule constante. Personnellement, je ne ferais pas ça. Soit vos constantes vont grossir:

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

ou les constantes resteront courtes, mais peu explicites:

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?

Cela présente également deux inconvénients. Tu devras:

• Conservez une liste distincte associant les informations de journal à leurs constantes respectives. Avec une seule constante, il se développera rapidement.

• Trouvez un moyen d'appliquer un format unique dans votre équipe. Par exemple, si au lieu de T2SSQ, quelqu'un décide d'écrire ST2SQL?

Essayez d'extraire une série de petites fonctions pour gérer toutes les choses en désordre. Il y a beaucoup de code répété qui pourrait très facilement être fait en un seul endroit. Par exemple:

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;

L'astuce consiste à examiner toute duplication dans votre code et à trouver des moyens de la supprimer. Utilisez beaucoup d'espace, et utilisez le début / fin à votre avantage (plus d'espace, et des blocs de code faciles à trouver / plier). Cela ne devrait vraiment pas être trop difficile. Ces méthodes pourraient faire partie de votre enregistreur ... cela dépend vraiment de vous. Mais cela semble être un bon point de départ.

Je dirais que l'idée derrière l'option 2 est la meilleure. Cependant, je pense que la direction que vous avez prise aggrave les choses. L'entier ne veut rien dire. Lorsque vous regardez le code, vous verrez que quelque chose est enregistré, mais vous ne savez pas quoi.

Au lieu de cela, je ferais quelque chose comme ceci:

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

Cela conserve la structure du message mais permet à votre code d'être flexible. Vous pouvez définir des chaînes constantes selon les besoins des phases et les utiliser uniquement comme paramètre de phase. Cela vous permet de modifier le texte réel en un seul endroit et de tout effectuer. L'autre avantage de la fonction d'assistance est que le texte important est avec le code (comme s'il s'agissait d'un commentaire), mais le texte qui n'est important que pour le fichier journal est résumé.

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

Ce n'est pas quelque chose que vous avez mentionné dans votre question, mais j'ai remarqué votre code. Votre indentation n'est pas cohérente. La première fois que vous l'utilisez, begince n'est pas en retrait, mais la deuxième fois. Vous faites une chose similaire avec else. Je dirais que c'est beaucoup plus important que les lignes de journal. Lorsque l'indentation n'est pas cohérente, il est difficile de scanner le code et de suivre le flux. De nombreuses lignes de journal répétitives sont faciles à filtrer lors de la numérisation.

Que diriez-vous quelque chose le long de cette ligne:

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

La méthode NewEntry () construirait la ligne de texte (y compris en ajoutant le [&] autour des entrées appropriées) et maintiendrait cela en attendant que les méthodes success () ou failure () soient appelées, qui ajoutent la ligne avec 'success' ou «échec», puis affichez la ligne dans le journal. Vous pouvez également créer d'autres méthodes, telles que info () lorsque l'entrée du journal concerne autre chose que succès / échec.