Need to make my code more readable to the other programers in my team

Question

I am working a project in delphi and I am creating a installer for the application, there are Three main parts.

1. PostgreSQL installation/uninstallation
2. myapplication ( setup of myapplication is created using nsi) installation/uninstallation.
3. Creating tables in Postgres through script(batch files).

Every thing runs fine and smoothly, but if something fails I have created a LogToFileger which will LogToFile every step of the process,
like this

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

The function LogToFileToFile.LogToFile() This will write the contents to a file. This is working nicely, but the problem is this has messed up the code as in it has become difficult to read the code as one ca only see the LogToFileToFile.LogToFile() function call everywhere in the code

an example

 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;

as you can see there are lots of LogToFileToFile.LogToFile() calls,
before it was

 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)

this is the case in my whole code now.
its difficult to read.

can any one suggest me a nice way to unclutter the calls to LogToFile?

like

1. Indenting the ' LogToFileToFile.LogToFile()` call
   like this

      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. Separate unit like LogToFileger
   This unit will have all the LogToFile messages in a switch case like this

    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;
   

so I can just call the LogToFilegingMyMessage(1) where ever required.

Can anyone tell me which is a better and cleaner approach to LogToFileging this way?

Answer 1

When you added logging, you introduced two things:

1. Code became bigger because for almost every action, you added a line that logs that action (or its failure)
2. Log lines themselves appear to be bloated and take away from readability because they take up so much room.

Each of these has problems has its own, relatively simple solution:

1. Break the code up into smaller functions. Instead of having one giant function that contains all your copies as well as log messages for error/success, you could introduce a function "CopyFile", that would copy exactly one file and logs it's own result. That way your main code would just consist of CopyFile calls and would remain easy to read.

2. You could make your logger smarter. Instead of passing in a giant string that has a lot of repetitive information, you could pass in enumerations values that would make things clearer. Or you could define more specialized Log() functions, i.e. LogFileCopy, LogDbInsert... Whatever you repeat a lot, consider factoring that out into its own function.

If you follow (1), you could have code that looks like this:

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

Then your CopyFile() just needs few lines of code to do the action and log its result, so all your code remains concise and easy to read.

I would stay away from your approach #2 as you are detaching information that should stay together into different modules. You are just asking for your main code to get out of sync with your log statements. But looking at LogMyMessage( 5 ), you'll never know that.

UPDATE (response to comment): I'm not familiar with exact language you are using, so this part might have to be adapted a bit. It seems all your log messages identify 3 things: component, action, outcome.

I think this is pretty much what MainMa suggested. Instead of passing actual string, define constants (in C/C++/C#, they would be part of enum enumeration type). So for example for components, you might have: DbInstall, AppFiles, Registry, Shortcuts... Anything that make the code smaller will make it easy to read.

It would also help if your language supported variable parameter passing, not sure if that's possible. So for example if action is "FileCopy", you could define that action to have two additional user parameters: file name and destination directory.

So your file copying lines would look something like this:

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

*note, there's also no reason to copy/paste the log line twice if you can store the result of the operation in a separate local variable and just pass that variable into Log().

You see the theme here, right? Less repetitive code -> more readable code.

Answer 2

Looks like you need to abstract out the concept of a "LoggableAction". I'm seeing a pattern in your example where all the calls return a bool to indicate success or failure and the only difference is the log message.

It's been years since I wrote delphi so this is pretty much c# inspired pseudo-code but I would have thought you want something like

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

Then your calling code becomes

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

I can't remember the Delphi syntax for function pointers but whatever the implementation details, some sort of abstraction around the log routine would seem to be what you're looking for.

Answer 3

One possible approach is to reduce the code by using constants.

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

would become:

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)

which has a better log code/other code ratio when counting the number of characters on the screen.

This is close to what you suggested in the point 2 of your question, except that I wouldn't go so far: Log(9257) is obviously shorter than Log(2, SqlInstal, Action, CopyMapSuccess, sOSdrive), but also quite difficult to read. What is 9257? Is it a success? An action? Is it related to SQL? If you work on this codebase for the past ten years, you'll learn those numbers by heart (if there is a logic, i.e. 9xxx are success codes, x2xx are related to SQL, etc.), but for a fresh developer who discovers the codebase, short codes will be a nightmare.

You can go further by mixing the two approaches: use a single constant. Personally, I wouldn't do that. Either your constants will grow in size:

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

or the constants will remain short, but not very explicit:

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?

This also has two drawbacks. You'll have to:

• Keep a separate list associating log info to their respective constants. With a single constant, it will grow fast.

• Find a way to enforce a single format in your team. For example, what if instead of T2SSQ, someone will decide to write ST2SQL?

Answer 4

Try extracting a series of little functions to handle all of the messy looking stuff. There is a lot of repeated code that could very easily be all done in a single place. For example:

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;

The trick is to look at any duplication in your code, and find ways to remove it. Use lots of whitespace, and use the begin/end to your advantage (more whitespace, and easy to find/fold code blocks). It really shouldn't be too difficult. These methods could be a part of your logger... it's up to you really. But that looks like a good place to start.

Answer 5

I would say that the idea behind option 2 is the best. However, I think the direction you took it makes things worse. The integer doesn't mean anything. When you are looking at the code, you will see something is being logged, but you don't know what.

Instead I would do something like this:

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

This retains the message structure but allows your code to be flexible. You can define constant strings as needed for the phases and only use those as the phase parameter. This allows you to be able to make change to the actual text in one place and effect everything. The other benefit to the helper function is that the important text is with the code (like it was a comment), but the text that is only important to the log file is abstracted away.

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

This isn't something that you mentioned in your question, but I noticed about your code. Your indentation is not consistent. The first time you use begin it is not indented, but the second time it is. You do a similar thing with else. I would say this is much more important than the log lines. When the indentation is not consistent, it makes it hard to scan the code and follow the flow. Lots of repetitive log lines are easy to filter out when scanning.

Answer 6

How about something along this line:

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

The NewEntry() method would build the line of text (including adding the [ & ] around the proper entries) and hold that in waiting until the success() or failure() methods are called, which append the line with 'success' or 'failure', and then output the line to the log. You could also make other methods, such as info() for when the log entry is for something other than success/failure.