Making Code More Readable for Team Members

code-qualityloggingreadability

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

PostgreSQL installation/uninstallation
myapplication ( setup of myapplication is created using nsi) installation/uninstallation.
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?

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;

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?

Best Answer

When you added logging, you introduced two things:

Code became bigger because for almost every action, you added a line that logs that action (or its failure)
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:

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.
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.

Naming conventions

stay lowercase for functions
use - for hyphenation (what would be underscore or camel case in other languages).

(defn add-one [i] (inc i))
Predicates (i.e. functions returning true or false) end with ? Examples: odd? even? nil? empty?
State changing procedures end in !. You remember set! right? or swap!
Choose short variable name lengths depending on their reach. That means if you have a really small auxiliary variable you can often just use a one-letter name. (map (fn [[k v]] (inc v)) {:test 4 :blub 5}) choose longer variable names as needed, especially if they are used for lots of code lines and you cannot immediately guess their purpose. (my opinion).

I feel that a lot of clojure programmers tend to rather use generic and short names. But this is of course not really an objective observation. The point is that a lot of clojure functions are actually quite generic.
- Use meaningful names.Procedures are doing something, therefor you can best describe them by using verbs. Clojure built-in functions should put you on the right track: drop, take, assoc, etc. Then there is a nice article describing ways to choose a meaningful name: http://ecmendenhall.github.io/blog/blog/2013/09/02/clean-clojure-meaningful-names/

Lambda functions

You can actually name lambda functions. This is convenient for debugging and profiling (my experience here is with ClojureScript).

(fn square-em [[k v]] {k (* v v)})
Use inline lambda functions #() as convenient

Whitespace

There should not be parens-only lines. I.e. close the parentheses right away. Remember parens are there for editor and compiler, indentation is for you.
Function parameter lists go on a new line

   (defn cons
     [a b]
     (list a b))

This makes sense if you think about the doc strings. They are between the function name and parameters. The following doc string is probably not the wisest ;)

   (defn cons
     "Pairing up things"
     [a b]
     (list a b))

Paired data can be separated by a new line as long as you retain the pairing

  (defn f 
    [{x :x 
      y :y 
      z :z  
      [a b c] :coll}] 
    (print x " " y  " " z " " a " " b " " c))

(You can also enter , as you like but this feels unclojurish).

For indentation use a sufficiently good editor. Years ago this was emacs for lisp editing, vim is also great today. Typical clojure IDEs should also provide this functionality. Just do not use a random text editor.

In vim in command mode you can use the = command to properly indent.
If command get too long (nested, etc.) you can insert a newline after the first argument. Now the following code is pretty senseless but it illustrates how you can group and indent expressions:

(+ (if-let [age (:personal-age coll)]
     (if (> age 18)
       age
       0))
   (count (range (- 3 b)
                 (reduce + 
                         (range b 10)))))

Good indentation means that you do not have to count brackets. The brackets are for the computer (to interpret the source code and to indent it). Indentation is for your easy understanding.

Higher order functions vs. `for` and `doseq` forms

Coming from a Scheme background I was rather proud to have understood map and lambda functions, etc. So quite often, I would write something like this

(map (fn [[k x]] (+ x (k data))) {:a 10 :b 20 :c 30})

This is quite hard to read. The for form is way nicer:

(for [[k x] {:a 10 :b 20 :c30}]
  (+ x (k data)))

`map has a lot of uses and is really nice if you are using named functions. I.e.

(map inc [12 30 10]

(map count [[10 20 23] [1 2 3 4 5] (range 5)])

Use Threading macros

Use the threading macros -> and ->> as well as doto when applicable.

The point is that threading macros make the source code appear more linear than function composition. The following piece of code is pretty unreadable without the threading macro:

   (f (g (h 3) 10) [10 3 2 3])

compare with

   (-> 
     (h 3)
     (g 10)
     (f [10 3 2 3]))

By using the threading macro, one can typically avoid introducing temporary variables that are only used once.

Other Things

Use docstrings
keep functions short
read other clojure code

Erlang – Function Naming Conventions

From official Erlang Programming Rules:

Function names:

The function name must agree exactly with what the function does. It should return the kind of arguments implied by the function name. It should not surprise the reader. Use conventional names for conventional functions (start, stop, init, main_loop).

Functions in different modules that solves the same problem should have the same name. Example: Module:module_info().

Some kind of naming convention is very useful when writing lots of different functions. For example, the name prefix "is_" could be used to signify that the function in question returns the atom true or false.

Also this project helps you programmatically to be compatible by those rules.

Best Answer

Related Solutions

Clojure – How to Write Readable Code