Originally posted on my blog
Developers can sink a lot of hours into fighting business rule code. Spaghetti business rules make it so little chan...
For further actions, you may consider blocking this person and/or reporting abuse
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:
Refactored:
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:
Refactored:
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 notI
.Augmenting the static analysis with some unit tests picked up a few more differences:
The comparator has been switched the wrong way around.
Original:
Refactored:
The error message has changed for the validation of transfer type code 710.
Original:
Refactored:
The
isBlockSize
check has been inverted.Original:
Refactored:
Tests are below.
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.
Pushed out a lunch time fix to the repo, we'll see if I get to the blog posts this evening
Waiting for the part 2
I've published part II
I'm typing furiously as we speak
I liked the Article. Thank you for that.
I also like to mention that introducing instance variables like this could be a source of danger if the used functions have side effects you didn't see.
Something like:
if (isSomethingWithoutSideEffects() && isSomethingWithSideEffects()) {
// stuff
}
Here the isSomethingWithSideEffects() will only be executed if the first isSomethingWithoutSideEffects() evaluates to true
while:
boolean first = isSomethingWithoutSideEffects();
boolean second = isSomethingWithSideEffects();
will both be executed.
Have a nice evening :)