Earlier this week, I shared the ColdFusion version of the GildedRose refactoring kata. That was my translation of Emily Bache’s kata into CFML with a CommandBox test harness. That is, it was just the starting state of the code. This post is a walk-through of my manual refactoring steps. I say “manual” because I think I might try to use Claude Code to run the refactoring in a follow-up post.
View my GildedRose project for CFML on GitHub.
The Original Code
The refactoring focuses on the massively complex updateQuality() method which is intended to be run nightly by the GildedRose Inn. The updateQuality() method walks over the items collection and updates the sellIn and quality properties for each item based on the following rules (extracted from the README.md file):
- All
itemshave aSellInvalue which denotes the number of days we have to sell theitems. - All
itemshave aQualityvalue which denotes how valuable the item is. - At the end of each day our system lowers both values for every item.
- Once the sell by date has passed,
Qualitydegrades twice as fast. - The
Qualityof an item is never negative. - “Aged Brie” actually increases in
Qualitythe older it gets. - The
Qualityof an item is never more than50. - “Sulfuras”, being a legendary item, never has to be sold or decreases in
Quality. - “Backstage passes”, like aged brie, increases in
Qualityas itsSellInvalue approaches;Qualityincreases by2when there are10days or less and by3when there are5days or less but.Qualitydrops to0after the concert.
- An item can never have its
Qualityincrease above50. - “Sulfuras” is a legendary item and as such its
Qualityis80and it never alters.
And here’s the original updateQuality() code (slightly truncated ColdFusion component):
component {
public void function updateQuality() {
for (var item in this.items) {
if (item.name != "Aged Brie" && item.name != "Backstage passes to a TAFKAL80ETC concert") {
if (item.quality > 0) {
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.quality = item.quality - 1;
}
}
} else {
if (item.quality < 50) {
item.quality = item.quality + 1;
if (item.name == "Backstage passes to a TAFKAL80ETC concert") {
if (item.sellIn < 11) {
if (item.quality < 50) {
item.quality = item.quality + 1;
}
}
if (item.sellIn < 6) {
if (item.quality < 50) {
item.quality = item.quality + 1;
}
}
}
}
}
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.sellIn = item.sellIn - 1;
}
if (item.sellIn < 0) {
if (item.name != "Aged Brie") {
if (item.name != "Backstage passes to a TAFKAL80ETC concert") {
if (item.quality > 0) {
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.quality = item.quality - 1;
}
}
} else {
item.quality = item.quality - item.quality;
}
} else {
if (item.quality < 50) {
item.quality = item.quality + 1;
}
}
}
}
}
}
Yikes!
The following sections will provide the individual git commits that I made along the way.
695522 Add Ben-friendly white-space.
The first thing I did was just give all the code some breathing room. I can’t focus when the syntax is ultra-compact. I added the necessary line-breaks and the inter-parenthesis padding. I also separates the collection-processing from the item-processing to give each method a single focus.
component {
/**
* I update the quality of the store items, assuming that 1-day has passed since this
* method was last called.
*/
public void function updateQuality() {
for ( var item in this.items ) {
updateQualityForItem( item );
}
}
/**
* I update the quality for the given item, assuming that 1-day has passed since this
* item was last updated.
*/
public void function updateQualityForItem( required Item item ) {
if (
( item.name != "Aged Brie" ) &&
( item.name != "Backstage passes to a TAFKAL80ETC concert" )
) {
if ( item.quality > 0 ) {
if ( item.name != "Sulfuras, Hand of Ragnaros" ) {
item.quality = item.quality - 1;
}
}
} else {
if ( item.quality < 50 ) {
item.quality = item.quality + 1;
if ( item.name == "Backstage passes to a TAFKAL80ETC concert" ) {
if ( item.sellIn < 11 ) {
if ( item.quality < 50 ) {
item.quality = item.quality + 1;
}
}
if ( item.sellIn < 6 ) {
if ( item.quality < 50 ) {
item.quality = item.quality + 1;
}
}
}
}
}
if ( item.name != "Sulfuras, Hand of Ragnaros" ) {
item.sellIn = item.sellIn - 1;
}
if ( item.sellIn < 0 ) {
if ( item.name != "Aged Brie" ) {
if ( item.name != "Backstage passes to a TAFKAL80ETC concert" ) {
if ( item.quality > 0 ) {
if ( item.name != "Sulfuras, Hand of Ragnaros" ) {
item.quality = item.quality - 1;
}
}
} else {
item.quality = item.quality - item.quality;
}
} else {
if ( item.quality < 50 ) {
item.quality = item.quality + 1;
}
}
}
}
}
cd4649 Simplifying conditional blocks and add guard statements.
I tried to reduce the indentation, simplify the conditional blocks, add guard statements, and use unary operators to reduce the overall ceremony and syntax without moving too much stuff around. By removing the noise, I’m starting to see the patterns more clearly; and have a better sense of where I want to go with the code.
component {
public void function updateQualityForItem( required Item item ) {
// Sulfuras items never have to change. They are legendary!
if ( item.name == "Sulfuras, Hand of Ragnaros" ) {
return;
}
if ( item.name == "Aged Brie" ) {
item.quality = min( ++item.quality, 50 );
} else if ( item.name == "Backstage passes to a TAFKAL80ETC concert" ) {
if ( item.sellIn < 6 ) {
item.quality += 3;
} else if ( item.sellIn < 11 ) {
item.quality += 2;
} else {
item.quality++;
}
item.quality = min( item.quality, 50 );
} else {
item.quality = max( --item.quality, 0 );
}
--item.sellIn;
// If the sell-by date hasn't passed yet, nothing more to process.
if ( item.sellIn >= 0 ) {
return;
}
if ( item.name == "Aged Brie" ) {
item.quality = min( ++item.quality, 50 );
return;
}
if ( item.name == "Backstage passes to a TAFKAL80ETC concert" ) {
item.quality = 0;
return;
}
item.quality = max( --item.quality, 0 );
}
}
3de251 Move to switch statements for processing decisions.
Once I had the conditional logic simplified, it became easier to see how I might move the logic to switch blocks. This gives me a sort of “recipe book” for how each item needs to be processed. Right now, I still have somewhat of a pre/post processing divergence; but, I think I might be able to simplify that as well.
component {
public void function updateQualityForItem( required Item item ) {
var MAX_QUALITY = 50;
var MIN_QUALITY = 0;
switch ( item.name ) {
case "Aged Brie":
item.quality = ++item.quality;
break;
case "Backstage passes to a TAFKAL80ETC concert":
if ( item.sellIn < 6 ) {
item.quality += 3;
} else if ( item.sellIn < 11 ) {
item.quality += 2;
} else {
item.quality += 1;
}
break;
case "Sulfuras, Hand of Ragnaros":
// Sulfuras never have to change. It is legend!
return;
break;
default:
item.quality = --item.quality;
break;
}
--item.sellIn;
// If the sell-by date HAS PASSED, some additional quality tweaks are needed.
if ( item.sellIn < 0 ) {
switch ( item.name ) {
case "Aged Brie":
item.quality = ++item.quality;
break;
case "Backstage passes to a TAFKAL80ETC concert":
item.quality = 0;
break;
default:
item.quality = --item.quality;
break;
}
}
item.quality = clamp( item.quality, MIN_QUALITY, MAX_QUALITY );
}
// ---
// PRIVATE METHODS.
// ---
/**
* I constraint the given value to the given min/max range.
*/
private numeric function clamp(
required numeric value,
required numeric minValue,
required numeric maxValue
) {
return min( max( value, minValue ), maxValue );
}
}
2e7411 Fixing some unary operators.
I had some superfluous unary operator assignments. Just an oversight in the previous refactoring. Again, just trying to simplify and reduce noise as much as possible.
component {
public void function updateQualityForItem( required Item item ) {
var MAX_QUALITY = 50;
var MIN_QUALITY = 0;
switch ( item.name ) {
case "Aged Brie":
++item.quality;
break;
case "Backstage passes to a TAFKAL80ETC concert":
if ( item.sellIn < 6 ) {
item.quality += 3;
} else if ( item.sellIn < 11 ) {
item.quality += 2;
} else {
item.quality += 1;
}
break;
case "Sulfuras, Hand of Ragnaros":
// Sulfuras never have to change. It is legend!
return;
break;
default:
--item.quality;
break;
}
--item.sellIn;
// If the sell-by date HAS PASSED, some additional quality tweaks are needed.
if ( item.sellIn < 0 ) {
switch ( item.name ) {
case "Aged Brie":
++item.quality;
break;
case "Backstage passes to a TAFKAL80ETC concert":
item.quality = 0;
break;
default:
--item.quality;
break;
}
}
item.quality = clamp( item.quality, MIN_QUALITY, MAX_QUALITY );
}
}
f08bd3 Factor-out special edge-case handling for legendary items.
For items that never have to be altered, I’m moving them to a guard statement so that I can do an early return and not worry about their logic ever again. This will allow me to further simplify the logic around products that do need to be changed.
component {
public void function updateQualityForItem( required Item item ) {
var MAX_QUALITY = 50;
var MIN_QUALITY = 0;
// Special handling of legendary items which never have to be changed.
switch ( item.name ) {
case "Sulfuras, Hand of Ragnaros":
return;
break;
}
switch ( item.name ) {
case "Aged Brie":
++item.quality;
break;
case "Backstage passes to a TAFKAL80ETC concert":
if ( item.sellIn < 6 ) {
item.quality += 3;
} else if ( item.sellIn < 11 ) {
item.quality += 2;
} else {
item.quality += 1;
}
break;
default:
--item.quality;
break;
}
--item.sellIn;
// If the sell-by date HAS PASSED, some additional quality tweaks are needed.
if ( item.sellIn < 0 ) {
switch ( item.name ) {
case "Aged Brie":
++item.quality;
break;
case "Backstage passes to a TAFKAL80ETC concert":
item.quality = 0;
break;
default:
--item.quality;
break;
}
}
item.quality = clamp( item.quality, MIN_QUALITY, MAX_QUALITY );
}
}
3b0e97 Minor update to better match user story.
The tickets are described in terms of “5 days or less” and “10 days or less”. I’m just updating the conditional logic to better reflect this thinking / language.
component {
public void function updateQualityForItem( required Item item ) {
var MAX_QUALITY = 50;
var MIN_QUALITY = 0;
// Special handling of legendary items which never have to be changed.
switch ( item.name ) {
case "Sulfuras, Hand of Ragnaros":
return;
break;
}
switch ( item.name ) {
case "Aged Brie":
++item.quality;
break;
case "Backstage passes to a TAFKAL80ETC concert":
if ( item.sellIn <= 5 ) {
item.quality += 3;
} else if ( item.sellIn <= 10 ) {
item.quality += 2;
} else {
item.quality += 1;
}
break;
default:
--item.quality;
break;
}
--item.sellIn;
// If the sell-by date HAS PASSED, some additional quality tweaks are needed.
if ( item.sellIn < 0 ) {
switch ( item.name ) {
case "Aged Brie":
++item.quality;
break;
case "Backstage passes to a TAFKAL80ETC concert":
item.quality = 0;
break;
default:
--item.quality;
break;
}
}
item.quality = clamp( item.quality, MIN_QUALITY, MAX_QUALITY );
}
}
fdc69f Collapsed pre/post sell-in switch statements.
Instead of having two main switch statements, one before and one after the sellIn decrement, which collapses down into a single switch statement. Now, instead of thinking about two phases of processing, there is only a single phase; which may have some conditional logic.
component {
public void function updateQualityForItem( required Item item ) {
var MAX_QUALITY = 50;
var MIN_QUALITY = 0;
// Special handling of legendary items which never have to be changed.
switch ( item.name ) {
case "Sulfuras, Hand of Ragnaros":
return;
break;
}
switch ( item.name ) {
case "Aged Brie":
item.quality += ( item.sellIn > 0 )
? 1
: 2
;
break;
case "Backstage passes to a TAFKAL80ETC concert":
if ( item.sellIn <= 0 ) {
item.quality = 0;
} else if ( item.sellIn <= 5 ) {
item.quality += 3;
} else if ( item.sellIn <= 10 ) {
item.quality += 2;
} else {
item.quality += 1;
}
break;
default:
item.quality -= ( item.sellIn > 0 )
? 1
: 2
;
break;
}
// Sell-in continues to decrement forever.
item.sellIn--;
// Quality is constrained to limits.
item.quality = clamp( item.quality, MIN_QUALITY, MAX_QUALITY );
}
}
a63e13 Add some semantic constants.
This is a final attempt to add a little more semantic insight to the conditions by providing constants for the before-and-after an item passes the sellIn threshold. The ternary operations aren’t always the easiest to read (if you’re not used to them); so, I thought that the big bold constant would reduce the cognitive load.
component {
public void function updateQualityForItem( required Item item ) {
var MAX_QUALITY = 50;
var MIN_QUALITY = 0;
var IS_BEFORE_SELL_IN = ( item.sellIn > 0 );
var IS_AFTER_SELL_IN = ! IS_BEFORE_SELL_IN;
// Special handling of legendary items which never have to be changed.
switch ( item.name ) {
case "Sulfuras, Hand of Ragnaros":
return;
break;
}
switch ( item.name ) {
case "Aged Brie":
item.quality += IS_BEFORE_SELL_IN
? 1
: 2
;
break;
case "Backstage passes to a TAFKAL80ETC concert":
if ( IS_AFTER_SELL_IN ) {
item.quality = 0;
} else if ( item.sellIn <= 5 ) {
item.quality += 3;
} else if ( item.sellIn <= 10 ) {
item.quality += 2;
} else {
item.quality += 1;
}
break;
default:
item.quality -= IS_BEFORE_SELL_IN
? 1
: 2
;
break;
}
// Sell-in continues to decrement forever.
item.sellIn--;
// Quality is constrained to limits.
item.quality = clamp( item.quality, MIN_QUALITY, MAX_QUALITY );
}
}
Adding Future Items
The instructions for the GildedRose refactoring mention that a new item, “Conjured”, has just been added to the store. The Conjured items degrade in Quality twice as fast as normal items. Unfortunately, the “expected output” of the 30-day text-based comparison has the Conjured item as “not yet implemented”; so I had no output against which to compare new logic.
That said, with the new refactoring, adding the logic should be as simple as adding a new case block:
case "Conjured Mana Cake":
item.quality -= IS_BEFORE_SELL_IN
? 2
: 4
;
break;
The default items degrade with 1 and 2 as the before/after sell-in date increments. Since the Conjured products degrade twice as fast, it’s the same ternary operation, just 2x the values.
Refactoring Is So Much Fun
I don’t know about you, but this kind of code refactoring is thrilling! It’s like having your own little murder mystery to solve. Having the test to run against the refactoring is helpful; but, tests or no tests, I think a journey like this underscores how important incremental work is. Instead of swinging for the fences on the first iteration, try to figure out the next smallest thing you can do in order to increase the readability and maintainability of the code. Learning how to think in incremental steps is a super power!
Also, never be afraid to commit and deploy small changes. Deploying 1-line changes to production is one of the safest things you can do as a web developer. That’s a good thing. And if your deployment process makes that challenging, see if you can incrementally improve your deployment process.
And if your manager won’t let you deploy 1-line changes…. see if you can incrementally improve your manager ;P
Want to use code from this post?
Check out the license.
https://bennadel.com/4866
Source link
