Discussion on: Turn Your Spaghetti Code into Functions - Part 1

[mbtts]

Excellent article and thank you for sharing. I enjoyed following along as this is a good example of believable "real world" code. It is important to develop strategies to manage and transform it such that it is easier to reason about.

TLDR: Before touching any legacy code (no matter how small the change might be) it is also very important to add test coverage first. If this refactor were to have been made on production code it would have improved the quality of the code, but it would have also introduced errors to the validation.

---

From static analysis (admittedly my static analysis skills are not the best) there were a couple of changes which are not logically equivalent and alter the behaviour of the validation:

D Category Transferer can only be transferred in transfer area 213

Original:

if (transfer.getTransferTypeCode().equals("200")) {
    if (!transfer.getAreaCode().matches("907|412|213")) {
        businessRuleErrors += "This area is not a transfer eligible area. ; ";
    } else if (!transfer.getAreaCode().matches("213")) {
        if (transfer.getTransferer().getCategory().equals("D")) {
            businessRuleErrors += "D Category Transferer can only be transferred in transfer area 213. ; ";
        }
    }
}

Refactored:

if (transferTypeCode.equals("200")
    && areaCode.matches("213")
    && category.equals("D")) {
    businessRuleErrors += "D Category Transferer can only be transferred in transfer area 213. ; ";
}

In the original version the message is displayed if the area code is not 213, whereas in the refactored version the message is displayed if the area code is 213.

Amount is too small for I type transfer

Original:

if (transfer.getTypeCode().equals("I")) {
    if (isBlockSize(transfer)) {
        businessRuleErrors += "Amount is too small for I type transfer. ; ";
    }
    if (isTotalOverCap(transfer)) {
        businessRuleErrors += "This transfer is too large. ; ";
    }
}

Refactored:

if (!typeCode.equals("I")
        && !isBlockSize(transfer)) {
    businessRuleErrors += "Amount is too small for I type transfer. ; ";
}


if (!typeCode.equals("I")
        && isTotalOverCap(transfer)) {
    businessRuleErrors += "This transfer is too large. ; ";
}

The original runs block size and cap checks if the type code is I, whereas in the refactored version these checks are run if the type code is not I.

---

Augmenting the static analysis with some unit tests picked up a few more differences:

The comparator has been switched the wrong way around.

Original:

if (transfer.getTransferer().getAccount(transfer.getFromAccount()).getBalance().compareTo(transfer.getAmount()) < 0)

Refactored:

if (balance.compareTo(transferAmount) > 0) {

The error message has changed for the validation of transfer type code 710.

Original:

else if (transfer.getTransferTypeCode().equals("710")) {
    if (!transfer.getAreaCode().matches("574|213|363|510")) {
        businessRuleErrors += "This area is not a transfer eligible area. ; ";
    }
}

Refactored:

if (transferTypeCode.equals("710")
    && !areaCode.matches("574|213|363|510")) {
    businessRuleErrors += "This area is not an eligible area. ; ";
}

The isBlockSize check has been inverted.

Original:

if (transfer.getTypeCode().equals("I")) {
    if (isBlockSize(transfer)) {
        businessRuleErrors += "Amount is too small for I type transfer. ; ";

Refactored:

if (!typeCode.equals("I")
     && !isBlockSize(transfer)) {
    businessRuleErrors += "Amount is too small for I type transfer. ; ";

---

Tests are below.

[Gunnar Gissel]

Good catch! I'm flattered someone would read close enough to catch business logic errors. You're completely right that you should have a test suite before starting a refactor of any significance - I think I provided a pretty good, if inadvertent, example of why!

What you're describing is familiar - I coded the examples in my cms (which is what you're seeing here). When I put them into actual code I noticed exactly what you are describing, thanks to my unit test suite. I made a mental note to change my blog posts, but I guess I never got around to it :)

You inspired me to double check my repo, and I see some optimization in the tests I could make - I've got some duplicated code, and refactor 5 is failing.

It's interesting writing examples where you try to retain all the versions - normally I'd just have a single test class, but here I ended up with multiple test classes all doing roughly the same thing. Looking it over it's because the return type changes slightly over the course of my refactor.

[Gunnar Gissel]

Pushed out a lunch time fix to the repo, we'll see if I get to the blog posts this evening