I did the kata before watching video. My end result was also split to handle per item logic, but I had no idea that its so simple with inlining if/else statement. Also learned that intellij can convert static to instance method. THANKS!
Cool! I'm really happy to hear you tried the exercise yourself, refactoring really is a skill you need hands-on practice to learn. Glad you also learnt some new things to try from my video.
Sorry, I can imagine that being read in comic book guy's voice. I meant it as a genuine question as now I'm not sure if I've been misunderstanding the requirements
First you should understand why that requirement exists. The rationale of that requirement is that the Item class is being used by some unknown external clients, so it is to prevent you from making broken changes that cascade into the external clients. Next, what are the broken changes that should be avoided in this case? It is changing existing public signatures, such as public fields, properties, and methods. E.g. renaming, changing types, changing parameters (for methods). Internal changes can be acceptable, as long as they do not alter external behaviors of the signatures. What Ms Emily did was adding a new signature (updateItem), and her refactoring did not alter any existing public signatures and behaviors (3 fields and 1 constructor). So you can say literally we violate the requirement, but practically we do not.
In other words, the Item class and that requirement respectively represent real-world public APIs and their rule to maintain backward compatibility: "APIs, once public, should have their signatures treated like diamonds, i.e. never change."
@@petervo224 I understand what you're saying, understand the idea of maintaining backwards compatibility of public APIs, and think that this interpretation makes a lot more sense, especially given the C# origins of the kata. But I do think the wording of the requirements are misleading if that is the case. "However, do not alter the Item class or Items property as those belong to the goblin in the corner who will insta-rage and one-shot you as he doesn't believe in shared code ownership (you can make the UpdateQuality method and Items property static if you like, we'll cover for you)." I think the language suggests you literally do not have ownership of the Item class and cannot make changes. I've seen the kata used in more than a few user groups and workshops over the years and in each case all participants have interpreted "do not alter" as "do not alter" as opposed to "only make additive alterations". I'd always assumed it was an extra constraint to encourage experimentation with approaches like sub-classing, extension methods, composition etc
@@GraanJonlo I see. So that requirement has multiple interpretations, one is literal ("absolutely no alter, even add") and the other is tolerant ("adding fields/methods is accepted"). Both can still let you through the kata. The former is a more difficult constraint (assuming the Goblin would throw tantrum even if you add comments to that code), while the latter makes the kata easier (but requires negotiation with the Goblin: We are adding values to your class!). Then I think it depends on how you want your Kata to be executed. Choose the former when you want to explore techniques to go around (and honor) that politics, and choose the latter when you want to break through and ignore that politics and focus on other matters such as balancing the principles (e.g. when you move the UpdateQuality method into Item class, you are improving Cohesion but also running small risk of SRP violation; when you honor that politics, the effect will be in opposite, SRP over High Cohesion). I guess Ms Emily chose the latter so that we can focus on more on certain refactoring and the polymorphism in this kata run. But yes, it can be fun to try out the former interpretation in another run.
This video of the refactoring is awsome! Thanks! I really wanted to know - are you going to do another short video about the "conjured items" requirement? (I would really like to see your approch to it :) ) Keep being awesome!
The lift-up conditional refactoring technique is great! Of course the caveat is that the extracted values really don't change (another example where immutability makes it easier to reason about code). Using the extract and inline method refactorings for this makes it really half-automatic and sets it apart from the usual extraction of common expressions. Kudos!
Thankyou, I'm glad you like it! Yes you're quite right the extracted values must not change during the following logic - I think I do say that in the video.
@@EmilyBache-tech-coach Yes, you say that in the video. I stressed the point again because I was wondering if this could be an automated refactoring in the IDE. But then, depending on the language, it may be difficult to impossible to make sure this condition holds.
@@christianschafer3724 Good point. If the toolmakers are going to make more automated refactorings available though, there are several higher on my list than this one 🙂 (ie getting as good refactoring support in other languages as we have in java)
I love the Gilded Rose Kata, not only when cleaning it for the "Conjured" requirement, but also inventing new requirements and playing around with them. For example, introducing layers (or test suites) of new requirements: 1. "Slime" item whose Quality represents the size/age of the slime. When Quality reaches 0, "Slime" will disappear. SellIn is always = Quality. To be clear, there is never be a "Slime" whose Quality is 0 in the inventory. 2. "Slime" item is allowed to have Quality more than 50. 3. "Tickling Grass" item is a normal item. However, whenever there is a "Tickling Grass" item in the store whose Quality reaches 0, "Slime" will eat the "Tickling Grass" and increases its Quality 11 then degrade normally (i.e. next date, the "Slime" Quality instead of -1 will be +10). If a "Slime" is about to die (Quality = 1), it will eat a "Tickling Grass" with lowest quality, if any. To be clear, whenever there is a "Slime" in the inventory, there is never be a "Tickling Grass" whose Quality is 0. 4. If there are more than one "Slimes", all "Slimes" will be merged into 1 the next date, then degrade like 1 normal "Slime." For example, Slime 2, Slime 3, Slime 4 will be merged into Slime 9, which degrades into Slime 8. The merge event always happens before the "Tickling Grass" feeding event. 5. "Conjured Slime" inherits both characteristics of "Conjured" and "Slime". Due to its fast degrade, its "about to die" threshold is either Quality 1 or Quality 2. When "Conjured Slime" eats the "Tickling Grass", the Quality increases by 22 (then degrades by 2, making it +20 next date). "Conjured Slime" and "Slime" don't merge with each other.
I do like the idea of items interacting with each other. In a workshop, do you prefer disclosing all requirements up front or introducing them later as a secret requirement (akin to a new business requirement). I feel like with the former people are less likely to get stuck and frustrated but the latter pushes people to reflect more on how to keep their code flexible to account for future uncertainty
There is a constraint specified in the requirement that you're not allowed to modify the Item class (second to last paragraph). Looks like you did that though.
As I understand it the constraint comes from a goblin who is likely to react badly. I am quite happy to talk to the goblin and take a design discussion about this. I think that is a better solution than blindly obeying this design advice.
@@EmilyBache-tech-coach Alternatively, reading the exercise, one might deduce that this, admittedly very strange, requirement has been inserted by its creator to limit its scope. So it makes sense to assume it is a hard requirement. Not because it makes sense in itself, or because one is afraid of a fictional goblin, but because the creator of the exercise put it there.
@@RTRMuizer I already changed this exercise quite a bit when I translated it and added tests to the starting position. I decided to do the design differently too.
![Felix "Unfair" | [Stray Kids : SKZ-PLAYER]](http://i.ytimg.com/vi/Oswujxm2Ag0/mqdefault.jpg)
I did the kata before watching video. My end result was also split to handle per item logic, but I had no idea that its so simple with inlining if/else statement. Also learned that intellij can convert static to instance method. THANKS!
Cool! I'm really happy to hear you tried the exercise yourself, refactoring really is a skill you need hands-on practice to learn. Glad you also learnt some new things to try from my video.
~11:40: Extract method and move to the Item class. Isn't one of the requirements of the kata that you can't modify the Item class?
Sorry, I can imagine that being read in comic book guy's voice. I meant it as a genuine question as now I'm not sure if I've been misunderstanding the requirements
First you should understand why that requirement exists. The rationale of that requirement is that the Item class is being used by some unknown external clients, so it is to prevent you from making broken changes that cascade into the external clients.
Next, what are the broken changes that should be avoided in this case? It is changing existing public signatures, such as public fields, properties, and methods. E.g. renaming, changing types, changing parameters (for methods). Internal changes can be acceptable, as long as they do not alter external behaviors of the signatures.
What Ms Emily did was adding a new signature (updateItem), and her refactoring did not alter any existing public signatures and behaviors (3 fields and 1 constructor). So you can say literally we violate the requirement, but practically we do not.
In other words, the Item class and that requirement respectively represent real-world public APIs and their rule to maintain backward compatibility: "APIs, once public, should have their signatures treated like diamonds, i.e. never change."
@@petervo224 I understand what you're saying, understand the idea of maintaining backwards compatibility of public APIs, and think that this interpretation makes a lot more sense, especially given the C# origins of the kata. But I do think the wording of the requirements are misleading if that is the case.
"However, do not alter the Item class or Items property as those belong to the goblin in the corner who will insta-rage and one-shot you as he doesn't believe in shared code ownership (you can make the UpdateQuality method and Items property static if you like, we'll cover for you)."
I think the language suggests you literally do not have ownership of the Item class and cannot make changes. I've seen the kata used in more than a few user groups and workshops over the years and in each case all participants have interpreted "do not alter" as "do not alter" as opposed to "only make additive alterations". I'd always assumed it was an extra constraint to encourage experimentation with approaches like sub-classing, extension methods, composition etc
@@GraanJonlo I see. So that requirement has multiple interpretations, one is literal ("absolutely no alter, even add") and the other is tolerant ("adding fields/methods is accepted"). Both can still let you through the kata. The former is a more difficult constraint (assuming the Goblin would throw tantrum even if you add comments to that code), while the latter makes the kata easier (but requires negotiation with the Goblin: We are adding values to your class!).
Then I think it depends on how you want your Kata to be executed. Choose the former when you want to explore techniques to go around (and honor) that politics, and choose the latter when you want to break through and ignore that politics and focus on other matters such as balancing the principles (e.g. when you move the UpdateQuality method into Item class, you are improving Cohesion but also running small risk of SRP violation; when you honor that politics, the effect will be in opposite, SRP over High Cohesion).
I guess Ms Emily chose the latter so that we can focus on more on certain refactoring and the polymorphism in this kata run. But yes, it can be fun to try out the former interpretation in another run.
This video of the refactoring is awsome! Thanks!
I really wanted to know - are you going to do another short video about the "conjured items" requirement? (I would really like to see your approch to it :) )
Keep being awesome!
Thanks! That's a great suggestion for another video, too.
The lift-up conditional refactoring technique is great! Of course the caveat is that the extracted values really don't change (another example where immutability makes it easier to reason about code). Using the extract and inline method refactorings for this makes it really half-automatic and sets it apart from the usual extraction of common expressions. Kudos!
Thankyou, I'm glad you like it! Yes you're quite right the extracted values must not change during the following logic - I think I do say that in the video.
@@EmilyBache-tech-coach Yes, you say that in the video. I stressed the point again because I was wondering if this could be an automated refactoring in the IDE. But then, depending on the language, it may be difficult to impossible to make sure this condition holds.
@@christianschafer3724 Good point. If the toolmakers are going to make more automated refactorings available though, there are several higher on my list than this one 🙂 (ie getting as good refactoring support in other languages as we have in java)
I love the Gilded Rose Kata, not only when cleaning it for the "Conjured" requirement, but also inventing new requirements and playing around with them. For example, introducing layers (or test suites) of new requirements:
1. "Slime" item whose Quality represents the size/age of the slime. When Quality reaches 0, "Slime" will disappear. SellIn is always = Quality. To be clear, there is never be a "Slime" whose Quality is 0 in the inventory.
2. "Slime" item is allowed to have Quality more than 50.
3. "Tickling Grass" item is a normal item. However, whenever there is a "Tickling Grass" item in the store whose Quality reaches 0, "Slime" will eat the "Tickling Grass" and increases its Quality 11 then degrade normally (i.e. next date, the "Slime" Quality instead of -1 will be +10). If a "Slime" is about to die (Quality = 1), it will eat a "Tickling Grass" with lowest quality, if any. To be clear, whenever there is a "Slime" in the inventory, there is never be a "Tickling Grass" whose Quality is 0.
4. If there are more than one "Slimes", all "Slimes" will be merged into 1 the next date, then degrade like 1 normal "Slime." For example, Slime 2, Slime 3, Slime 4 will be merged into Slime 9, which degrades into Slime 8. The merge event always happens before the "Tickling Grass" feeding event.
5. "Conjured Slime" inherits both characteristics of "Conjured" and "Slime". Due to its fast degrade, its "about to die" threshold is either Quality 1 or Quality 2. When "Conjured Slime" eats the "Tickling Grass", the Quality increases by 22 (then degrades by 2, making it +20 next date). "Conjured Slime" and "Slime" don't merge with each other.
oh cool! I like the idea of slime and items in the inventory which interact with each other.
I do like the idea of items interacting with each other. In a workshop, do you prefer disclosing all requirements up front or introducing them later as a secret requirement (akin to a new business requirement). I feel like with the former people are less likely to get stuck and frustrated but the latter pushes people to reflect more on how to keep their code flexible to account for future uncertainty
Thank you, you helped me and my friend a lot.
Happy to hear that!
There is a constraint specified in the requirement that you're not allowed to modify the Item class (second to last paragraph). Looks like you did that though.
As I understand it the constraint comes from a goblin who is likely to react badly. I am quite happy to talk to the goblin and take a design discussion about this. I think that is a better solution than blindly obeying this design advice.
That Goblin would be fired in most of the places where I worked
@@EmilyBache-tech-coach Alternatively, reading the exercise, one might deduce that this, admittedly very strange, requirement has been inserted by its creator to limit its scope. So it makes sense to assume it is a hard requirement. Not because it makes sense in itself, or because one is afraid of a fictional goblin, but because the creator of the exercise put it there.
@@RTRMuizer I already changed this exercise quite a bit when I translated it and added tests to the starting position. I decided to do the design differently too.
great stuff, I wonder if VSCode would enable so many refactorings within the tool.
You should definitely try it out :-)
I will@@EmilyBache-tech-coach