Kotlin 81 - A new Feature. And Assessing.
We are supposed to implement a new feature in the Gilded Rose program. Let’s do that … and let’s take a look at where we are … and where we might go.
Here are the requirements for the new feature:
We have recently signed a supplier of conjured items. This requires an update to our system:
“Conjured” items degrade in Quality twice as fast as normal items.
I can see two readings of this, one easier than the other.
It might mean that there is just one kind of “Conjured” item, so that we can just create another suitable const and a branch in our when.
But it might mean that there will be many different conjured items and that the program should somehow deal with any that arise. That would require a more sophisticated match than our current check for equality. You may already have noticed that backstage passes were oddly specific:
const val PROD_PASS = "Backstage passes to a TAFKAL80ETC concert"
As written, and as refactored and as strangled, the current program would treat backstage passes to the Beatles reunion as “normal”. What you’d think would be a better design would be for there to be some kind of Quality Adjustment Strategy, and a separate field in Item for the strategy. Or some such arrangement: we’d want to know more about how the sop people want to work to make the best choice.
For now, we’ll just suppose there’s one kind of Conjured Item, but flag the issue for discussion with Allison.
Let’s get to it. We’ll start with a new constant:
const val PROD_BRIE = "Aged Brie"
const val PROD_CONJ = "Conjured Item"
const val PROD_ELIX = "Elixir of the Mongoose"
const val PROD_PASS = "Backstage passes to a TAFKAL80ETC concert"
const val PROD_SULF = "Sulfuras, Hand of Ragnaros"
const val PROD_VEST = "+5 Dexterity Vest"
And a new branch in the when:
PROD_CONJ -> {
}
We could have waited for the branch. I got ahead of myself. We need, of course, some tests.
@Test
fun `conjured decrease two per day`() {
checkUpdate(PROD_CONJ, 20, 30, 28)
}
This is enough to drive out the behavior. Test. Fails, but with what seems to me to be an impossible answer:
expected: <19> but was: <20>
Expected :19
Actual :20
After way too much confusion, I realize that this is complaining, not about quality but about the sellIn date not declining.
PROD_CONJ -> {
sellIn -= 1
lowerQuality()
lowerQuality()
}
That should do it. Yes. I feel like such a fool. I really do want that check in checkUpdate:
fun checkUpdate(prod: String, sellInStart: Int, qualityStart: Int, qualityEnd: Int) {
val item = Item(prod, sellInStart, qualityStart)
val gr = GildedRose(arrayOf(item))
if (true)
gr.updateQuality()
else
gr.oldUpdateItem(item)
assertEquals(sellInStart-1, item.sellIn)
assertEquals(qualityEnd, item.quality)
}
That if(true), by the way, is there to allow me to run my tests against the old update method. I put that in while looking for the bug that wasn’t a bug, yesterday. Which gives me an idea:
We could, as part of these tests, run both the new and the old method, and check whether they agree. That might be quite a good way to include characterization testing with our more direct microtests. Worth thinking about. I’m almost glad I became confused.
Feature Done …
I claim the feature is done. However, in thinking about all this, I can imagine some tests that we could have, if only to make certain facts more explicit. Let me write them: I think it’ll become clear.
@Test
fun `quality does not go negative`() {
checkUpdate(PROD_VEST, 5, 0, 0)
}
@Test
fun `quality does not exceed 50`() {
checkUpdate(PROD_BRIE, 5, 50, 50)
}
- Hmmm
- Did you know that if you type “kotlin” with your right hand off the home keys, it is “lpt;om”? Well, now you do. And so do I.
We could implement those tests differently, but I think they’ll do the job. I have them there for the names more than the content.
- More Seriously …
- It was too easy to skip that
sellIndecrease line in my implementation. Let’s review all the whens:
private fun updateItem(item: Item) {
with (item) {
when (name) {
PROD_BRIE -> {
sellIn -= 1
raiseQuality()
}
PROD_CONJ -> {
sellIn -= 1
lowerQuality()
lowerQuality()
}
PROD_PASS -> {
sellIn -= 1
raiseQuality()
if (sellIn<10) raiseQuality()
if (sellIn<5) raiseQuality()
if (sellIn<0) quality = 0
}
PROD_SULF -> {
/* nothing */
}
else -> {
sellIn -= 1
lowerQuality()
if (sellIn<0 ) lowerQuality()
}
}
}
}
I would very much like to move the adjustment of sellIn up outside the when, so that we can’t forget it again. But Sulfuras doesn’t want to be adjusted. Let’s extract a method and then guard it internally.
private fun updateItem(item: Item) {
with (item) {
when (name) {
PROD_BRIE -> {
adjustSellIn()
raiseQuality()
}
PROD_CONJ -> {
adjustSellIn()
lowerQuality()
lowerQuality()
}
PROD_PASS -> {
adjustSellIn()
raiseQuality()
if (sellIn<10) raiseQuality()
if (sellIn<5) raiseQuality()
if (sellIn<0) quality = 0
}
PROD_SULF -> {
/* nothing */
}
else -> {
adjustSellIn()
lowerQuality()
if (sellIn<0 ) lowerQuality()
}
}
}
}
private fun Item.adjustSellIn() {
sellIn -= 1
}
I do love this IDEA and its refactoring help. Test, just because we like reassurance. Green. Modify the adjust:
private fun Item.adjustSellIn() {
if(name!= PROD_SULF) sellIn -= 1
}
Test, despite that this obviously works. Green.
Now move the call out of all the when branches, and up to the top:
private fun updateItem(item: Item) {
with (item) {
adjustSellIn()
when (name) {
PROD_BRIE -> {
raiseQuality()
}
PROD_CONJ -> {
lowerQuality()
lowerQuality()
}
PROD_PASS -> {
raiseQuality()
if (sellIn<10) raiseQuality()
if (sellIn<5) raiseQuality()
if (sellIn<0) quality = 0
}
PROD_SULF -> {
/* nothing */
}
else -> {
lowerQuality()
if (sellIn<0 ) lowerQuality()
}
}
}
}
Test. Green. Commit: Implement Conjured Items; sellIn adjustment now done just once at top.
The long message tells me that I should have committed before making this little refactoring. I am imperfect.
While we’re here, we’ll take a look around for things to improve, but my overall feeling is that this is in good shape for what it does.
I emphasize: for what it does. The program seems not to generalize readily to a “product type” notion … though I do have an idea about that. Let’s do a quick scan … I’ll put all the code below. We’ll all, you and I, just scan for things we might improve:
const val PROD_BRIE = "Aged Brie"
const val PROD_CONJ = "Conjured Item"
const val PROD_ELIX = "Elixir of the Mongoose"
const val PROD_PASS = "Backstage passes to a TAFKAL80ETC concert"
const val PROD_SULF = "Sulfuras, Hand of Ragnaros"
const val PROD_VEST = "+5 Dexterity Vest"
class GildedRose(var items: Array<Item>) {
fun updateQuality() {
for (i in items.indices) {
val item = items[i]
updateItem(item)
}
}
private fun updateItem(item: Item) {
with (item) {
adjustSellIn()
when (name) {
PROD_BRIE -> {
raiseQuality()
}
PROD_CONJ -> {
lowerQuality()
lowerQuality()
}
PROD_PASS -> {
raiseQuality()
if (sellIn<10) raiseQuality()
if (sellIn<5) raiseQuality()
if (sellIn<0) quality = 0
}
PROD_SULF -> {
/* nothing */
}
else -> {
lowerQuality()
if (sellIn<0 ) lowerQuality()
}
}
}
}
private fun Item.adjustSellIn() {
if(name!= PROD_SULF) sellIn -= 1
}
private fun Item.lowerQuality() {
quality -= 1
quality = max(0, quality)
}
private fun Item.raiseQuality() {
quality += 1
quality = min(50, quality)
}
I can imagine wanting something more clever than two copies of lowerQuality() to lower it doubly fast. It could take a parameter. We could do the same with raiseQuality and perhaps make the backstage passes more clear.
Maybe we’ll try those in the future, just to see how we like it.
- Surprise!
- Just a few paragraphs below, he actually does it!
We could imagine extracting those little patches of code inside the selectors as methods or functions or something. Honestly, I’d rather see them here but again, we might try it to see if we prefer it.
- N.B.
- Doing the one change below triggers a number of nice improvements. This is often the thing with doing a small change: one leads to another, and in a short period of time, the code improves substantially.
Ah. Here is something that we definitely should do. The updateItem method doesn’t meet our preferred standard of either doing some one thing, or calling things. We can extract the entire when off into a method. Let’s see if we like that:
private fun updateItem(item: Item) {
with (item) {
adjustSellIn()
updateItemQuality()
}
}
private fun Item.updateItemQuality() {
when (name) {
PROD_BRIE -> {
raiseQuality()
}
PROD_CONJ -> {
lowerQuality()
lowerQuality()
}
PROD_PASS -> {
raiseQuality()
if (sellIn < 10) raiseQuality()
if (sellIn < 5) raiseQuality()
if (sellIn < 0) quality = 0
}
PROD_SULF -> {
/* nothing */
}
else -> {
lowerQuality()
if (sellIn < 0) lowerQuality()
}
}
}
I think that’s better by a small margin, and not everyone would agree. I did notice something else, however:
fun updateQuality() {
for (i in items.indices) {
val item = items[i]
updateItem(item)
}
}
private fun updateItem(item: Item) {
with (item) {
adjustSellIn()
updateItemQuality()
}
}
The top method, updateQuality should really be updateItems, I think. It updates items, not just quality. Rename it.
Now, I think I’ll rename the new updateItemQuality to updateQuality. It was my desire to use that name for that method that caused me to recognize the updateItems issue. Rename.
I’m on a roll now. Let’s change lowerQuality to take a parameter.
- See?
- We build up momentum as we think about the code and transforming it.
private fun Item.lowerQuality(change: Int) {
quality -= change
quality = max(0, quality)
}
Now IDEA flags all my callers, so I change them:
private fun Item.updateQuality() {
when (name) {
...
PROD_CONJ -> {
lowerQuality(2)
}
...
else -> {
lowerQuality(1)
if (sellIn < 0) lowerQuality(1)
}
}
}
Now, what can we do about that else? I’d prefer it to be more explicit.
else -> {
if (sellIn < 0 )
lowerQuality(2)
else
lowerQuality(1)
}
Better, I think. Better still, though, is this:
else -> {
when {
sellIn < 0 -> lowerQuality(2)
else -> lowerQuality(1)
}
}
Oh yes, I do like that one! Green. Commit: lowerQuality takes change parameter. Refactor to use it.
Let’s do raise as well:
private fun Item.raiseQuality(change: Int) {
quality += change
quality = min(50, quality)
}
Now again I have to provide the parameter. (We could have defaulted it. Would that be better? I’m not sure. This is more explicit.)
PROD_BRIE -> {
raiseQuality(1)
}
...
PROD_PASS -> {
raiseQuality(1)
if (sellIn < 10) raiseQuality(1)
if (sellIn < 5) raiseQuality(1)
if (sellIn < 0) quality = 0
}
Test. Green. Commit: add and use change parameter in raiseQuality.
Now let’s improve the PASS:
private fun Item.updateQuality() {
when (name) {
PROD_SULF -> { /* nothing */ }
PROD_BRIE -> { raiseQuality(1) }
PROD_CONJ -> { lowerQuality(2) }
PROD_PASS -> {
when {
sellIn < 0 -> { quality = 0 }
sellIn < 5 -> { raiseQuality(3) }
sellIn < 10 -> { raiseQuality(2) }
else -> { raiseQuality(1) }
}
}
else -> {
when {
sellIn < 0 -> lowerQuality(2)
else -> lowerQuality(1)
}
}
}
}
Green. Commit: Refactor Passes to when.
Now I do think I’d like to pull out a method for those two more complex ones.
- Note:
- Earlier, the need was there but less obvious. Other improvements make the need more clear, and we have momentum in improving the code, which makes it easy to take a few more steps.
That gives me this:
private fun Item.updateQuality() {
when (name) {
PROD_SULF -> { /*nothing*/ }
PROD_BRIE -> raiseQuality(1)
PROD_CONJ -> lowerQuality(2)
PROD_PASS -> adjustPassQuality()
else -> adjustNormalQuality()
}
}
private fun Item.adjustPassQuality() {
when {
sellIn < 0 -> quality = 0
sellIn < 5 -> raiseQuality(3)
sellIn < 10 -> raiseQuality(2)
else -> raiseQuality(1)
}
}
private fun Item.adjustNormalQuality() {
when {
sellIn < 0 -> lowerQuality(2)
else -> lowerQuality(1)
}
}
YMMV on the tight formatting: I like it. I also learned that the braces were redundant, except for the “nothing” one.
Summary
Once we get to tidying with small changes, we can get quite a ways toward the code we prefer in a very short time, especially with the aid of a powerful refactoring tool like IDEA’s, and, of course, supported by enough tests to give us confidence that neither it (very unlikely) or we ourselves (quite a bit more likely) have broken something.
I am pleased with where we are. Smaller methods, more clarity. I shall leave you here to enjoy your weekend. See you next time!