Skip to content

Instantly share code, notes, and snippets.

@qntm
Last active November 15, 2021 19:22
Show Gist options
  • Save qntm/02a262cb692c79441e95f4637e840f3e to your computer and use it in GitHub Desktop.
Save qntm/02a262cb692c79441e95f4637e840f3e to your computer and use it in GitHub Desktop.
My Gilded Rose entry
class Item {
constructor (name, sellIn, quality) {
this.name = name
this.sellIn = sellIn
this.quality = quality
}
}
class Shop {
constructor (items = []) {
this.items = items
}
updateQuality () {
// items are mutated in place!
this.items.forEach(item => {
// this item has fixed `quality` and `sellIn`
if (item.name === 'Sulfuras, Hand of Ragnaros') {
return
}
if (item.name === 'Aged Brie') {
// brie improves in quality with age, twice as fast once past sell-by date
item.quality += item.sellIn <= 0 ? 2 : 1
} else if (item.name === 'Backstage passes to a TAFKAL80ETC concert') {
// backstage passes improve in quality until the concert, then become worthless
if (item.sellIn <= 0) {
item.quality = 0
} else {
item.quality += item.sellIn <= 5 ? 3 : item.sellIn <= 10 ? 2 : 1
}
} else {
// regular items decrease in quality with age, faster once past sell-by date
item.quality -= item.sellIn <= 0 ? 2 : 1
}
// quality is clamped
if (item.quality <= 0) {
item.quality = 0
}
if (item.quality >= 50) {
item.quality = 50
}
item.sellIn--
})
return this.items
}
}
module.exports.Item = Item
module.exports.Shop = Shop
@qntm
Copy link
Author

qntm commented Nov 15, 2021

This is my preferred refactoring of this source file, which is the JavaScript version of the "Gilded Rose" refactoring exercise originally created by Terry Hughes.

Comments in-line are part of my refactoring and are addressed to hypothetical future maintainers of this code. Actual comments from me, the person participating in this exercise, to you, the person reviewing my submission, are as follows:

  • I applied some personal formatting preferences, notably omitting semicolons.
  • This refactoring preserves several assumptions which, in the original Java, are hard truths, such as the fact that quality and sellIn are integers, and that every element of items is an object of class Item.
  • As you can see, I favour branching procedural code over the use of inheritance or object-oriented code.
  • Both before and after refactoring, this code is riddled with magic numbers and magic strings. I've left them all where they are for now because in my opinion this maximises legibility for this specific source file, taken in isolation. As soon as any other code appeared - for displaying item properties to the shopkeeper, for example - they would need to be factored out.
  • Rather than assign a brand new object to module.exports, I prefer to assign properties to the existing module.exports object. This decreases the probability of some very troublesome behaviour in the event of circular imports.
  • I find it a little concerning that the updateQuality method both modifies the items in place and returns this.items. To me, returning this.items gives the false impression that the returned value is a brand new array and that the original this.items was left unchanged. (Array.prototype.sort has the same problem.) I would prefer it if updateQuality returned undefined. However, my refactoring preserves the existing behaviour.
  • I also find it slightly puzzling that theoretically the "Sulfuras" item could start out with a quality outside of the closed integer range [0, 50], 60 say, and is expected to stay that way. (Again, my refactoring preserves this behaviour.) Is quality capped or not? Is this a bug in the game? Note that factoring these values out as, say, const MIN_QUALITY = 0 and const MAX_QUALITY = 50 would be wrong in this scenario. What would we even call these constants, in this case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment