Code commenting in C/AL (NAV) - Pros and cons

Bogdana

I have seen the question of code commenting in NAV application code, being asked again and again. There are pros and cons to it. Maybe this topic deserves to have a place of its own.

This is not a promise that the results of this conversation will influence production code. That is not the goal. The goal is for NAV developers in general to learn from others' experiences, arguments they didn't think of etc. There is often a side we haven't seen, to any story.

Please add your oppinions. To keep the debate in a positive note, try to
1. not repeat an idea already stated (you can +1 it though)
2. add (pro) (con) in front of each argument, like this
- (pro) my 1st argument
- (con) my 2nd argument, etc.

Bogdana

kine

At first, I will begin with the main question: comment or not to comment

Two extreme environments:
A) Company is using some version control to keep tracks of what was done in some driven way, company is using different patterns like hooks etc. is using automatic tests...
No version control, no rules, no patterns, no automatic testing

Tracking comments (who, when, why, what):
In case of company A, you do not need to comment the code to know who changed what and why, because you can get it from the version system (e.g. Blame function in git etc.).
In case of company B, comments are only tracks on who changed what and why and without it you have only mess without possibility to merge.

Explaining comments (why):
In case of company A, in most cases you do not need to explain the code itself, because the code is "selfdescribing" (because you are using short functions with self-explanatory names etc.)
In case of company B, you need to explain the spagetty code to other readers because it will cost them too much time to understand it etc.

Documentation comments (instructions for possible readers, like "do not do this and this, be aware of..."):
Company A: You do not need that, because if you break something, automatic testing will tell you...
Company B: You need them to prevent possible breaks, but even the comments will not protect you.


And because I think that any company is not only A or B, but rather it is on some way between A and B, it is question of level of each part (version control, connection between VC and other systems like change request evidence etc.) on which depends, what you really need and what is not needed.

From Merge point of view:

Usage of // vs {}:

{}
(pro) - only two lines added - before and after - the standard code you want to "replace". Easy for merge.
(pro) - simple to do in older version where comment/uncomment block of code doesn't exists (it means before NAV 2015)
(cons) - automatic merge will not detect change inside the commented code as conflict
(cons) - hard to see, because syntax highlighting is not taking it into account

//
(pro) - will rise conflict when code is changed
(pro) - easy to spot
(cons) - hard to commend many lines in older versions (before NAV 2015)

Marking beginning and end of the change:
regardless previous text, I will recommend to use the tracking comment in the standard objects to mark changed blocs (and it is needed to have CfMD). It is easy to spot when merging and there is some conflict. Than it is help for taking only your part and transfer it to the target. Without it you will need to go into version control system and check it.

wakestar

Kine pretty much nailed it.

In our core add-on we do not comment changes. - We only comment code which is not selfexplaining.

However we have two customized add-on's (branch add-on) based on the core add-on.
The changes in the customized add-on's are visible through comments in the code, text in documentation trigger and some "dummy" variables which we use as separators. Doing this helps us to update the customized add-on easier (manually or with merge tools).
(all add-ons have to be in-sync all the time)

kriki

kine wrote:

{}
(cons) - automatic merge will not detect change inside the commented code as conflict

(pro) - automatic merge will not give a conflict when it shouldnt.

Ex. When you need to change some standard code (sometimes you don't have a choice...), I keep the original code and then add lines with the code I want it to be.

CRONUS Code:

IF Something THEN

My changes:

{OLD CODE
IF Something THEN
}
IF Something AND SomethingElse THEN

=> The automatic merge tool sees as NEW lines "{OLD CODE" and "}" and "IF Something AND SomethingElse THEN" but considers "IF Something THEN" As not changed. So this is ok. Of course my code must come AFTER the original code.
If I would use

// IF Something THEN

A mergetool would consider the line changed and maybe even give some conflict.

Miklos_Hollender

Interesting how in all these discussions it is always an add-on focus, a product focus, not a customization (service) focus... maybe in the longer run companies that focus on making products survive better.

At any rate, customizing the standard almost certainly should be commented, and the question is now. I have seen two patterns:

//-MH 12.12.2009

//+MH 12.12.2009

and

//MH 12.12.2009 BEGIN

//MH 12.12.2009 END


Also when replacing standard code, commeting it out and adding a new one immediately below so the change is visible

I have one generic rule that makes things pretty self-documenting: so far as possible, no code change without making a new field. Or Table. About 70% of the changes involve new fields anyway, and for the rest, which is largely about changing how standard data is processed, I make a checkmark in a Customizations Setup table to turn it on or off because it will be different per company anyway, usually. This way just pulling the list of 5000-range fields + fields in 50000-range tables from the Field table gives me a very good high level overview what the changes in the system are generally doing, what the important customizations are.

I do this because I STRONG disagree with doing upgrades via merging everything in text. You will end up with a system in 10 years that is customized to death. Rather it should be reviewed with the users what changes they need, and dropping everything unnecessary! Typically a customization is requested because someone likes to work a certain way then they leave the company and the replacement likes to work in a different way. E.g. we have a fairly big thing where G/L Entries can be applied through detailed entries to each other, similar to customer and vendor entries. It was requested by the previous chief accountant and our current chief accountant has no idea what the hell could it be useful for and me neither. So it is not going in the next version.

Sorry, I drifted offtopic My point is more like that it is important to comment customizations, but it more important to do things in a way that you see the major direction of changes right from the Field table.