Can we change CodeCop rule AA0005?

[DavidRoys]

Can we change CodeCop rule AA0005?

The CodeCop rules can be found at https://docs.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-codeanalyzer-codecop-rules and CodeCop rule AA0005 is as follows:

AA0005	Only use BEGIN..END to enclose compound statements.	Only use BEGIN..END to enclose compound statements.	Only use BEGIN..END to enclose compound statements.	Readability	Warning	true

Is there a way to turn off a specific rule in CodeCop? Or better still, can I convince you to change this rule? I think the above rule is a bad rule and CodeCop should actually be the opposite to this rule. These should be the correct rules:

Description	Category	Default Severity	IsEnabledbyDefault
Always use BEGIN..END following an IF statment.	Mantainability	Warning	True
Always use BEGIN..END following an ELSE statment.	Mantainability	Warning	True

Like most style-related issues, I’m sure opinion will divided on this. However, my goal is to produce code that can be maintained without introducing errors. I do not believe the readability of code with my proposed rules is any worse than the current rule, in fact I think it is easier to see what is going on with my proposed rules. There is a possibility that you do not want to change this rule because so much standard Microsoft code will violate this rule, but I would ask you to question whether that would be the correct motivation behind which rules should be included. The rules are there to try to ensure that the code we produce is robust (maintainable, upgradable, reusable).

It’s not hard to find discussions on this topic, so I took the first hit from a search as an example and changed the syntax of the examples given to be al. The original topic can be found at https://stackoverflow.com/questions/2125066/is-it-bad-practice-to-use-an-if-statement-without-brackets

Let’s look at both options.

// 1 - Code is compliant with current rule
if SomethingIsTrue() then
  DoSomethingImportant()
else
  DoSomethingLessImportant();

// 2 - Code is compliant with proposed new rule changes 
if SomethingIsTrue() then begin
  DoSomethingImportant();
end else begin
  DoSomethingLessImportant();
end;

The problem with the first version is that if you go back and add a second statement to the if or else clauses without remembering to add the begin and end, your code will break in unexpected and amusing ways.

Maintainability-wise, it's always smarter to use the second form.

For an interesting real world example of why omitting compound statements following an if or else can cause problems, see this discussion of the “Go to fail” bug that caused serious security vulnerabilities in 2014: https://www.imperialviolet.org/2014/02/22/applebug.html

[erikrijn]

I would actually propose to completely remove begin's and end's. All this unnecessary typing all day ;) In my opinion, curly brackets would make AL code much more readable.

if (true)
  DoSomethingImportant();
else
  DoSomethingLessImportant();

if (true)
{
  DoSomethingImportant();
  DoSomethingEvenMoreImportant();
}
else
  DoSomethingLessImportant();

[Gallimathias]

The bracketed question is already very much under discussion in #63. Please join us there instead of new discussion threads ^^. @erikrijn

Currently there is no Microsoft statement.

[DavidRoys]

I’m not tying to change the language syntax. I just want to get rid of my CodeCop warnings. I’m hoping someone can either convince me the begin and end make the code less readable or agree to remove this rule, or at the very least, tell me how I can turn it off. :-( I appreciate the input, but this issue is really about wanting to get my code down to zero warnings.

[vytjak]

I agree with David here - there should be a way to disable certain rules/warnings, should we want to.

Our reasoning, which is probably subjective: while the official Microsoft style for AL might insist on having BEGIN/END only on compound statements, we always code our solutions for maintainability/ease of upgrade. If we need to add more lines of code to an IF branch in the future, having enclosing BEGIN/END from the start make for a cleaner modification code.

[thpeder]

Hi @DavidRoys, With the use of rulesets you can hide that warning. Ruleset for the Code Analysis Tool

[qutreson]

You can also have a look at: Using the Code Analysis Tools with the Ruleset in order to set your ruleset.

[DavidRoys]

Those answers helped me to achieve what I needed thank you. I have another problem now with rule AA0139:

AA0139	Do not assign a text to a target with smaller size.	Do not assign a text to a target with smaller size.	Possible overflow assigning '{0}' to '{1}'.	Design	Warning	true

I guess this is just a case of the rule not being smart enough. The following code triggers this rule:

Value := CopyStr(NewValue,1,MaxStrLen(Value));

I don't see any way of circumventing this rule. I guess I can change it to an info type rule so it doesn't get too annoying.

[qutreson]

The issue on rule AA0139 has already been reported in #2291. I guess turning into an info could be a good workaround for now.