Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I am working a project in delphi 7 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 logger which will log every step of the process,
like this

LogBook.Log('[POSTGRESQL INSTALLATION]  :  [ACTION]:Postgres installation started');

The function LogBook.Log() 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 LogBook.Log() function call everywhere in the code

an example

 if Not FileExists(sOSdrive+'\Mapannotation.txt') then
 begin
    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');
 end;
 if Not FileExists(sOSdrive+'\Mappoint.txt')      then
   begin
     if CopyFile(PChar(sTxtpath+'Mappoint.txt'), PChar('c:\Mappoint.txt'), False) then
       LogBook.Log(2,'[POSTGRESQL INSTALLATION] :  [ACTION]:copying Mappoint.txt to '+sOSdrive+'\ sucessful')
   else
       LogBook.Log(2,'[POSTGRESQL INSTALLATION] :  [ACTION]:copying Mappoint.txt to '+sOSdrive+'\ Failed');
 end;

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

 if Not FileExists(sOSdrive+'\Mapannotation.txt') then
    CopyFile(PChar(sTxtpath+'Mapannotation.txt'), PChar(sOSdrive+'\Mapannotation.txt'), False) 
 if Not FileExists(sOSdrive+'\Mappoint.txt')      then
   CopyFile(PChar(sTxtpath+'Mappoint.txt'), PChar('c:\Mappoint.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 log?

like

  1. Indenting the ' LogBook.Log()` call
    like this

       if Not FileExists(sOSdrive+'\Mapannotation.txt') then
         begin
             if CopyFile(PChar(sTxtpath+'Mapannotation.txt'), PChar(sOSdrive+'\Mapannotation.txt'), False) then
            {Far away--->>}                   LogBook.Log(2,'[POSTGRESQL INSTALLATION] :  [ACTION]:copying Mapannotation.txt to '+sOSdrive+'\ sucessful')
       else
            {Far away--->>}                   LogBook.Log(2,'[POSTGRESQL INSTALLATION] :  [ACTION]:copying Mapannotation.txt to '+sOSdrive+'\ Failed');
       end;
    
  2. Separate unit like logger
    This unit will have all the log messages in a switch case like this

     Function LoggingMyMessage(loggMessage : integer)
    
     begin
    case  loggMessage of
    
    1         :  LogBook.Log(2,'[POSTGRESQL INSTALLATION] :  [ACTION]:copying Mapannotation.txt to '+sOSdrive+'\ sucessful');
    2         :  LogBook.Log(2,'[POSTGRESQL INSTALLATION] :  [ACTION]:copying Mapannotation.txt to '+sOSdrive+'\ Failed');
       150        :  LogBook.Log(2,'[somthing] :  [ACTION]: somthing important);
    
    end;
    

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

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

share|improve this question
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 Feb 6 '12 at 12:52
    
@Spoike:i did ask, its little difficult to read, as everywhere the logBook.log() is encountered. –  cod3r Feb 6 '12 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 Feb 6 '12 at 14:02
    
@S.Lott i edited the 'two' to 'three' ..sory for the mistake –  cod3r Feb 7 '12 at 4:37
    
You might want to also try codereview.stackexchange.com –  Kirk Broadhurst Feb 7 '12 at 4:46

6 Answers 6

up vote 8 down vote accepted

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.

share|improve this answer
    
+1, can you tell me more about you could pass in enumerations values this? –  cod3r Feb 6 '12 at 13:35
    
@PresleyDias: updated post –  DXM Feb 6 '12 at 13:46
    
ok got it, yes less repetitive->more readable code –  cod3r Feb 6 '12 at 13:51
1  
+1 "Break the code up into smaller functions." You can't stress that enough. It just makes so many problems just disappear. –  Oliver Weiler Feb 6 '12 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)
{
    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.

share|improve this answer
    
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 Feb 6 '12 at 13:26
    
+1, LoggableAction() this is nice, i can directly write the returned value instead of checking and the writing. –  cod3r Feb 6 '12 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 –  cod3r Feb 8 '12 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')
   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?

share|improve this answer
    
+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] ? –  cod3r Feb 6 '12 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. –  MainMa Feb 6 '12 at 13:41
    
single format in your team is a good/great option –  cod3r Feb 6 '12 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);
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.

share|improve this answer
    
+1, white spaces is nice way..success := CopyFile() thanks for the idea, this will reduce some unnecessary lines of code in my case –  cod3r Feb 6 '12 at 13:26
    
@S.Robins did I read your code correctly? your method called LogIfFileDoesNotExist copies files? –  João Portela Feb 6 '12 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 Feb 6 '12 at 21:02
    
good to see that you took the time to address that issue, +1. –  João Portela Feb 6 '12 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.

share|improve this answer

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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