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
    if CopyFile(PChar(FilePathBase+'FileName.txt'), PChar(SystemDrive+'\FileName.txt'), False) then
       LogToFileToFile.LogToFile('[DatabaseInstallation] :  copying FileName.txt to '+SystemDrive+'\ done')
       LogToFileToFile.LogToFile('[DatabaseInstallation] :  copying FileName.txt to '+SystemDrive+'\ Failed');
 if Not FileExists(SystemDrive+'\SecondFileName.txt')      then
     if CopyFile(PChar(FilePathBase+'SecondFileName.txt'), PChar('c:\SecondFileName.txt'), False) then
       LogToFileToFile.LogToFile('[DatabaseInstallation] : copying SecondFileName.txt to '+SystemDrive+'\ done')
       LogToFileToFile.LogToFile('[DatabaseInstallation] :  copying SecondFileName.txt to '+SystemDrive+'\ Failed');

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?


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

       if Not FileExists(SystemDrive+'\FileName.txt') then
             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')
            {Far away--->>}                   LogToFileToFile.LogToFile(2,'[DatabaseInstallation] :  [ACTION]:copying FileName.txt to '+SystemDrive+'\ Failed');
  2. Separate unit like LogToFileger
    This unit will have all the LogToFile messages in a switch case like this

     Function LogToFilegingMyMessage(LogToFilegMessage : integer)
    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);

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?

  • 5
    To answer your topic: Have you tried asking your team if they understand it or if it all makes sense? If yes, then it should be "enough" readable.
    – Spoike
    Commented Feb 6, 2012 at 12:52
  • @Spoike:i did ask, its little difficult to read, as everywhere the logBook.log() is encountered. Commented Feb 6, 2012 at 13:04
  • "there are two main parts" numbered 1 to 3. I think I see why you have a question on readability. You might want to find someone who can "edit" for consistency.
    – S.Lott
    Commented Feb 6, 2012 at 14:02
  • @S.Lott i edited the 'two' to 'three' ..sory for the mistake Commented Feb 7, 2012 at 4:37
  • You might want to also try codereview.stackexchange.com Commented Feb 7, 2012 at 4:46

6 Answers 6


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.

  • +1, can you tell me more about you could pass in enumerations values this? Commented Feb 6, 2012 at 13:35
  • @PresleyDias: updated post
    – DXM
    Commented Feb 6, 2012 at 13:46
  • ok got it, yes less repetitive->more readable code Commented Feb 6, 2012 at 13:51
  • 2
    +1 "Break the code up into smaller functions." You can't stress that enough. It just makes so many problems just disappear. Commented Feb 6, 2012 at 15:36

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)

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.

  • I would probably go this way myself, but without knowing more about how the OP's code is structured, it's difficult to tell if this would be better than simply defining a couple of extra methods to call, without adding the potential confusion of method pointers (depending on how much the OP knows about such things.
    – S.Robins
    Commented Feb 6, 2012 at 13:26
  • +1, LoggableAction() this is nice, i can directly write the returned value instead of checking and the writing. Commented Feb 6, 2012 at 13:28
  • i wish +100, great answer ,but i can accept only one answer:(..i will try this suggestion in my next application, thanks for the idea Commented Feb 8, 2012 at 4:11

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

  • +1, for the clean log call, but can you explain me more it did not understand Log(2, SqlInstal, Action, CopyMapFailure, sOSdrive) , you mean to say SqlInstal will be my defined variable like SqlInstal:=[POSTGRESQL INSTALLATION] ? Commented Feb 6, 2012 at 13:31
  • @PresleyDias: SqlInstal can be anything, for example a value 3. Then, in Log(), this value will be effectively translated into [POSTGRESQL INSTALLATION] before being concatenated with other parts of log message. Commented Feb 6, 2012 at 13:41
  • single format in your team is a good/great option Commented Feb 6, 2012 at 13:42

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);
   success: boolean;
   if Not FileExists(sOSdrive+'\'+filename') then
      success := CopyFile(PChar(sTxtpath+filename), PChar(sOSdrive+filename), False);

      Log(filename, success);

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

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

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.

  • +1, white spaces is nice way..success := CopyFile() thanks for the idea, this will reduce some unnecessary lines of code in my case Commented Feb 6, 2012 at 13:26
  • @S.Robins did I read your code correctly? your method called LogIfFileDoesNotExist copies files? Commented Feb 6, 2012 at 14:15
  • 1
    @JoãoPortela Yeah... it's not very pretty and doesn't stick to the single responsibility principle. Bear in mind this was a first pass refactoring off the top of my head and aimed at helping the OP to satisfy his aim to reduce some of the clutter in his code. It's probably a poor choice of name for the method in the first place. I'll tweak it a bit to improve. :)
    – S.Robins
    Commented Feb 6, 2012 at 21:02
  • good to see that you took the time to address that issue, +1. Commented Feb 6, 2012 at 22:33

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.


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

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.