2015 m. gegužės 15 d., penktadienis

AX Source Code Quality: What is a Bad Source Code?

Hello AX World,

During my short career I have had a chance to see lots (50+) of different AX implementations and solutions from various countries. I have seen both good and bad code. Unfortunately, in most of the cases it was a bad code. And its a problem, because a person who is reading a bad source code can get frustrated quickly.

But what is a bad code? Why it's bad? How to make it better? To answer these questions and to help you to write a better code I am going to start a series of blog post regarding AX source code quality. I am also open for discussions as there are many arguable topics (e.g. code commenting).

I will start my first post with an example to consider:

Keep in mind, this is an overridden method in a class extending RunBase class (from RunBase framework. Ignore the fact that RunBase is an obsolete framework in AX 2012.

How many bad practices can you notice? Before reading it further try to test yourself.
So, how many issues you have noticed? 1, 2, 3,... more?

Let's try with some bad practice basics.

1. Application elements should be started with a capital letter to easily distinguish them among variables.
2. Brackets missing around if statement even if its a single line for a better readability.
3. There should be a single space between addition operators.
4. There should be a single space after a separator.
5. Redundant empty line.

Got the idea? Now lets move on with more sophisticated issues.

6.There should be a single label and preferably strfmt function be used instead  of two labels and a number of adding operators.

7. Instead of calling infolog.add method, error method could be called as it contains the same code and requires only one parameter, the text.

8. Validation logic should be moved to a validate method instead. The method getFromDialog is used to parse values from the dialog after it has been closed by clicking OK. This method has the code which calls validate method afterwards.
9. Validation code could be optimized. Well, this can be arguable.
So I have found 9 problems. Is that a lot for 20 lines of code? Maybe an eagle-eye professional could add even more. All thoughts are welcome.

So to finalize the first post I strongly believe that the code should look like this:

If you can see any issues with my suggested code, feel free to comment.

Be aware and take care!

2 komentarai:

  1. Infolog Messages in validation methods are usually created with the ret = checkFailed("Some error message"); method. Other than that, your code looks fine. Personally, I would make the if expressions easier to read with if(projTable.RecId == 0) and else if (projTable.Template == NoYes::No), but that's just me. Thanks for doing a blog series highlighting bad code, I've seen my fair share of that and would like to see less of it in the future.

  2. I agree, checkFailed could be used instead of error. The only difference thou is that checkFailed gives you a warning instead of an error.

    Regarding easier to read expressions, I just love the automatic value conversion to boolean. Neither Java nor C# have this feature. It might be difficult for a junior in the beginning but when you advance, it's just a great feature