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.
            // <GEERU>
            if (_reasonTable.Bank || _reasonTable.Cust || _reasonTable.Ledger || _reasonTable.Vend || _reasonTable.rCash || _reasonTable.rAsset)
            // </GEERU>
            {
                ret = false;
            }
            break;
        case ReasonCodeAccountTypeAll::Bank:
 
            // Validation fails if any fields are checked except the bank field.
            // <GEERU>
            if (_reasonTable.Asset || _reasonTable.Cust || _reasonTable.Ledger || _reasonTable.Vend || _reasonTable.rCash || _reasonTable.rAsset)
            // </GEERU>
            {
                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).

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 <= fieldGroup.numberOfFields(); i++)
        {
            accountFields.add(fieldGroup.field(i));
        }
    }
 
    return accountFields;
}

Now I have everything what I need to iterate over module fields and check their values:

public boolean isUsedForAnotherModule(ReasonTable _reasonTable)
{
    FieldId       fieldToCheck;
    FieldId       acceptedField = this.typeToFieldId(reasonCodeAccountType);
    SetEnumerator enumerator    = this.accountFields().getEnumerator();
 
    while (enumerator.moveNext())
    {
        fieldToCheck = enumerator.current();
 
        if (fieldToCheck != acceptedField && _reasonTable.(fieldToCheck) == NoYes::Yes)
        {
            return true;
        }
    }
 
    return false;
}

It should be easy to follow. The acceptedField variable contains the field used for the current module (I used my new method for mapping from module type to the field ID). Then it enumerates module fields and check if any field (excluding the accepted one) has the value equal to NoYes::Yes.

Now the part of validateDelete() that checks for other modules can be reduce to a single method call. This is our new implementation:

public boolean validateDelete(ReasonTable _reasonTable)
{
    boolean ret = !this.isUsedForAnotherModule(_reasonTable);
 
    // Do additional check for Letter of Guarantee fields
    …
 
    return ret;
}

Isn’t it better? It’s easy to read, because you simply read the name of the method and don’t have to deal with dozen lines of code. And if you need to add a new module, you don’t have to change validateDelete() in any way.

I almost hear some developers refusing to start such a refactoring, saying that it takes too much time or it would be too much code. I strongly disagree. It might have taken more time than the mindless copy & paste, but it was still just a few minutes – that’s really nothing to worry about. And it will save time whenever we need to extend or fix the code.

Regarding the length of the code – the solution is already significantly shorter than the original method and the real saving would start only from now. For example, look at datasourceInitValue():

public void datasourceInitValue(ReasonTable _reasonTable)
{
    switch(reasonCodeAccountType)
    {
        case ReasonCodeAccountTypeAll::FixedAssets:
            // default the asset account type
            _reasonTable.Asset = true;
            break;
        case ReasonCodeAccountTypeAll::Bank:
            // default the bank account type
            _reasonTable.Bank = true;
            break;
 
    // … and the same for other modules …
}

You see that there is another instance of the switch statement mapping account types to fields. Because we already extracted the logic to a separate method, we can reuse it and replace the whole method with this:

public void datasourceInitValue(ReasonTable _reasonTable)
{
    if (reasonCodeAccountType != ReasonCodeAccountTypeAll::AllValues)
    {
        _reasonTable.(this.typeToFieldId(reasonCodeAccountType)) = true;
    }
}

Notice that I threw away almost all code of datasourceInitValue() and that’s nothing unusual when refactoring not-so-good code. I often wonder how much time it took to develop several hundred lines of code that I’m removing, because they aren’t actually needed. It’s not refactoring what leads into a lot of code – it’s the lack of refactoring. And having more code than actually needed is bad, because it means more code to read, more code to maintain, more places where bugs can hide and so on.

I didn’t invent refactoring, DRY, SOLID or anything like that; it’s all known for years and you can find many books about these things. But I see that many developers are still not aware of these principles and techniques, therefore it’s important to repeat them. Hopefully seeing real code will help some developers who are willing to improve themselves but who aren’t fans of abstract definitions.