goshoom: Refactoring of financial reasons

[Blog bot]

Источник: http://dev.goshoom.net/en/2015/03/re...ncial-reasons/
==============

If you’re not sure what “refactoring” means, note that it is a change of the structure of code without changing how the application behaves. It might sound useless, but it’s actually very important. You must realize that the quality of code doesn’t depend only on its runtime behavior – whether you can easily maintain and extend the code is equally important.

I tend to do refactoring and implementation of new features as two distinct activities, therefore I always know whether I’m trying to change the behavior or keep it the same, and I can test the code accordingly. If you’re lucky enough to have automated tests, they will help you to check whether your refactoring didn’t accidentally change the behavior.

I often spend more time refactoring existing code than implementing something new. That doesn’t mean that I’m wasting time. It often means that the change itself is a single line of code – as soon as the code is made easily extensible.

I work on improving and extending existing code bases very often, therefore I could show many typically problems and refactorings, nevertheless I obviously don’t want to discuss my clients’ code publicly. What pushed me to write this blog post was an interesting refactoring of a class from the standard AX application.

My goal was adding a new module to financial reasons in AX 2012:



I thought it would be pretty easy, because it looks like a type of change that original developers could expect and prepare for. Unfortunately the implementation makes this kind of change quite difficult and error prone.

Let’s take one specific method as an example. This is validateDelete() method of ReasonFormTable class, which is called from Reasons form:

public boolean validateDelete(ReasonTable _reasonTable){ boolean ret = true; switch(reasonCodeAccountType) { case ReasonCodeAccountTypeAll::FixedAssets: // Validation fails if any fields are checked except the fixed asset field. // if (_reasonTable.Bank || _reasonTable.Cust || _reasonTable.Ledger || _reasonTable.Vend || _reasonTable.rCash || _reasonTable.rAsset) // { ret = false; } break; case ReasonCodeAccountTypeAll::Bank: // Validation fails if any fields are checked except the bank field. // if (_reasonTable.Asset || _reasonTable.Cust || _reasonTable.Ledger || _reasonTable.Vend || _reasonTable.rCash || _reasonTable.rAsset) // { ret = false; } break;






I’m showing just first two cases, but there are eight in total, all with almost identical code. The duplicity itself tells me that the code has a couple of problems. If you want to add a new module, you’ll have to change all existing cases. You’ll have to change a lot of code in many places, which means many opportunities for errors. And it’s not very readable – it doesn’t express the intention (“check if the reason isn’t used in other modules”) very well and it would be difficult to spot the error if you incidentally used a wrong field somewhere.

You probably noticed that the developer implementing Russian functionality must have already modified every case. He didn’t refactor the solution to something better, he blindly followed the existing pattern. But it doesn’t mean that the problem remained exactly the same as before – it actually become much worse, because the number of modules to handle increased from five to seven.

Instead of changing a few dozen places in the class, the developer should have refactored the solution to be more extensible and then modified just a few places. That’s the approach that all developers should follow – you can start with a simple solution, but when it becomes causing problems, don’t wait and refactor it. It will help not only with the current task, but also with all later extensions and bug fixes.

Let’s look at my refactoring. First of all, I realized that I would need some mapping between modules and table fields. I used a similar switch as in the original code (although I had more options, such as using a map); the difference is that it’s now encapsulated in a separate method that doesn’t do anything else. It can be used from many places and if I add a new module, I’ll know where to go and simply add one more scenario there.

private FieldId typeToFieldId(ReasonCodeAccountTypeAll _type){ switch (_type) { case ReasonCodeAccountTypeAll::FixedAssets: return fieldNum(ReasonTable, Asset); case ReasonCodeAccountTypeAll::Bank: return fieldNum(ReasonTable, Bank); case ReasonCodeAccountTypeAll::Cust: return fieldNum(ReasonTable, Cust); case ReasonCodeAccountTypeAll::Ledger: return fieldNum(ReasonTable, Ledger); case ReasonCodeAccountTypeAll::Vend: return fieldNum(ReasonTable, Vend); case ReasonCodeAccountTypeAll::RCash: return fieldNum(ReasonTable, rCash); case ReasonCodeAccountTypeAll::RAsset: return fieldNum(ReasonTable, rAsset); default: return 0; }}






To be able to work with “all module fields except a specific one”, I need a list of all fields representing module check boxes. I could define it in code, but the application already contains such a definition – in a field group on the table. Therefore I can use reflection to read the list of fields from there (and I cache it to a member variable, to avoid running the same code too often).

<div class="wp-geshi-highlight-wrap5"><div class="wp-geshi-highlight-wrap4"><div class="wp-geshi-highlight-wrap3"><div class="wp-geshi-highlight-wrap2"><div class="wp-geshi-highlight-wrap"><div class="wp-geshi-highlight"><div class="xpp">private Set accountFields(){ DictFieldGroup fieldGroup; int i; if (!accountFields) { accountFields = new Set(Types::Integer); fieldGroup = new DictFieldGroup(tableNum(ReasonTable), tableFieldGroupStr(ReasonTable, AccountType)); for (i = 1; i <span class="sy0">