Good programming practice

Michael Cronqvist

Hey guys

I have a continuous dialog with my developers about Good programmering practice and not all get it.

Please take a look at this piece of code below. I my oppinion this is badly written code because:

Unnessecary negation of the conditional statement can lead to confusions.
The negation makes an ekstra operation.
Obviously dead code since "return" can be avoided.

Obviously you should not negate and instead do the "this.updateTrans(); ok = true;" inside the if statement. This way it's much more readable by human eyes.

What are you guys oppinion and arguments on this?

Thank in advance.

3 Replies

Know the answer? Post it — somebody with the same question will find it here.

Guest User6y ago
Point taken. That was not obvious from the code. In that case I'd agree with your rewrite as suggested in your bullet list ;)
Michael Cronqvist6y ago
@pritaeas

First of all, this is x++ (Microsoft Dynamics 365FO) and some things are done differently of what you would expect of eg. C# (camelCase vs PascalCase etc.). Also x++ code are often extensions of standard MS code. Ex: updateTrans is a standard method and does not have a return value.

But that's not the issue here and I will not imaging if there are hundreds of lines, because there isn't in this case :-)
Guest User6y ago
Very subjective.

In this case there is only one extra line, but imagine there are hundreds. In that case the term "break early, break often" comes to mind.

Another thing is why is this code inside a try? If the try is only to handle exceptions from the update method, then you should use validate before the try (try/catch is much more expensive than a not (!)).

Personally I'd have the update method return a boolean, so you can write:

ok = validateSelectedWorker() && updateTrans();

That adds yet another personal favourite: why use this at all (it can be omitted), and why prefer camelCase over PascalCase.