Foundations of Software Engineering Refactoring Code Demonstration 1

Foundations of Software Engineering Refactoring: Code Demonstration 1

WHAT WE’RE DOING TODAY Today we will do a code kata called “The Gilded Rose”. A code kata is an exercise you practice over and over to hone a specific skill. The skill in question for “The Gilded Rose“ kata is refactoring. Link to the original kata (in C#) and a Git. Hub repository with the kata in many different languages are on the course website. 2

THE GILDED ROSE Hi and welcome to team Gilded Rose. As you know, we are a small inn with a prime location in a prominent city ran by a friendly innkeeper named Allison. We also buy and sell only the finest goods. Unfortunately, our goods are constantly degrading in quality as they approach their sell by date. 3

THE GILDED ROSE We have a system in place that updates our inventory for us. It was developed by a nononsense type named Leeroy, who has moved on to new adventures. Your task is to add the new feature to our system so that we can begin selling a new category of items. First an introduction to our system: 4

THE GILDED ROSE • All items have a sell. In value which denotes the number of days we have to sell the item. • All items have a quality value which denotes how valuable the item is. • At the end of each day our system lowers both values for every item. 5

THE GILDED ROSE Pretty simple, right? Well this is where it gets interesting: 6

THE GILDED ROSE • Once the sell by date has passed, quality degrades twice as fast. • The quality of an item is never negative. • “Aged Brie” actually increases in quality the older it gets. • The quality of an item is never more than 50. 7

THE GILDED ROSE • “Sulfuras”, being a legendary item, never has to be sold or decreases in quality. • “Backstage passes”, like “Aged Brie”, increases in quality as its sell. In value approaches; quality increases by 2 when there are 10 days or less and by 3 when there are 5 days or less but quality drops to 0 after the concert. 8

THE GILDED ROSE 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. 9

THE GILDED ROSE Feel free to make any changes to the update. Quality method and add any new code as long as everything still works correctly. 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 update. Quality method and items property static if you like, we’ll cover for you). 10

THE GILDED ROSE Just for clarification, an item can never have its quality increase above 50, however “Sulfuras” is a legendary item and as such its quality is 80 and it never alters. 11

THE GILDED ROSE Now that we know the setting, let’s see the code: 12

public class Item { public String name; public int sell. In; public int quality; public Item(String name, int sell. In, int quality) { this. name = name; this. sell. In = sell. In; this. quality = quality; } } 13

public class Gilded. Rose { Item[] items; public Gilded. Rose(Item[] items) { this. items = items; } public void update. Quality() { /* Next slides */ } } 14

1 2 3 4 5 6 7 8 9 10 11 for (int i = 0; i < items. length; i++) { if (!items[i]. name. equals("Aged Brie") && !items[i]. name. equals("Backstage passes to a TAFKAL 80 ETC concert" )) { if (items[i]. quality > 0) { if (!items[i]. name. equals("Sulfuras, Hand of Ragnaros" )) { items[i]. quality = items[i]. quality - 1; } } } else { if (items[i]. quality < 50) { items[i]. quality = items[i]. quality + 1; 12 13 14 15 16 17 18 19 20 21 22 23 if (items[i]. name. equals("Backstage passes to a TAFKAL 80 ETC concert" )) { if (items[i]. sell. In < 11) { if (items[i]. quality < 50) { items[i]. quality = items[i]. quality + 1; } } if (items[i]. sell. In < 6) { if (items[i]. quality < 50) { items[i]. quality = items[i]. quality + 1; }}}}}. . . 15

28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 . . . if (!items[i]. name. equals("Sulfuras, Hand of Ragnaros" )) { items[i]. sell. In = items[i]. sell. In - 1; } if (items[i]. sell. In < 0) { if (!items[i]. name. equals("Aged Brie")) { if (!items[i]. name. equals("Backstage passes to a TAFKAL 80 ETC concert" )) { if (items[i]. quality > 0) { if (!items[i]. name. equals("Sulfuras, Hand of Ragnaros" )) { items[i]. quality = items[i]. quality - 1; } } } else { items[i]. quality = items[i]. quality - items[i]. quality; } } else { if (items[i]. quality < 50) { items[i]. quality = items[i]. quality + 1; } } 16

CODE REVIEW As we can see, the update. Quality method is huge! Long method, deeply nested conditionals, magic strings. It’s definitely not readable, and figuring out how to add the new “Conjured item” feature will be a nightmare. 17

CODE REVIEW We need a refactor ASAP. But the code already works, how can we make sure we’re not ruining working code? 18

CODE REVIEW Tests! 19

CODE REVIEW We will not show them here (they are all simple assert. Equals according to the rules above), but from here on, we are going to run these tests and make sure they are green before each refactoring step. 20

THE REFACTORING Let’s begin with the low-hanging fruit: the for-loop can definitely be rewritten as a for-each loop, and then we don’t have items[i] everywhere. Our IDE can do that for us easily. 21

THE REFACTORING Next is line 43: items[i]. quality = items[i]. quality - items[i]. quality; That’s a long way of writing zero, so let’s change that: items[i]. quality = 0; Tests are green, let’s continue. 22

THE REFACTORING The last easy one is extracting the entire body of the for loop to a different method. This way we can concentrate only on the single item logic, and ignore the loop. 23

THE REFACTORING This is how the code looks like now, after replacing the regular for with a for-each, changing line 43 above, and extracting the body of the loop: 24

public void update. Quality() { for (Item item : items) { update. Item. Quality. And. Sell. In(item); } } 25

1 2 3 4 5 6 7 8 9 10 11 private void update. Item. Quality. And. Sell. In(Item item) { if (!item. name. equals("Aged Brie") && !item. name. equals("Backstage passes to a TAFKAL 80 ETC concert" )) { if (item. quality > 0) { if (!item. name. equals("Sulfuras, Hand of Ragnaros" )) { item. quality = item. quality - 1; } } } else { if (item. quality < 50) { item. quality = item. quality + 1; 12 13 14 15 16 17 18 19 20 21 22 23 24 if (item. name. equals("Backstage passes to a TAFKAL 80 ETC concert" )) { if (item. sell. In < 11) { if (item. quality < 50) { item. quality = item. quality + 1; } } if (item. sell. In < 6) { if (item. quality < 50) { item. quality = item. quality + 1; } } } 26

29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 if (!item. name. equals("Sulfuras, Hand of Ragnaros" )) { item. sell. In = item. sell. In - 1; }. . . if (item. sell. In < 0) { if (!item. name. equals("Aged Brie")) { if (!item. name. equals("Backstage passes to a TAFKAL 80 ETC concert" )) { if (item. quality > 0) { if (!item. name. equals("Sulfuras, Hand of Ragnaros" )) { item. quality = item. quality - 1; } } } else { item. quality = 0; } } else { if (item. quality < 50) { item. quality = item. quality + 1; } } 27

THE REFACTORING Now it’s time to think. We have all these nested conditionals, but essentially, we see three types of “special” items: Aged Brie, Sulfuras, and Backstage Passes, and then there’s the case of a regular item which decreases in 1 quality and 1 sell. In every call to update. Item. Quality. And. Sell. In. 28

THE REFACTORING Let’s extract methods based on these four cases (three special, one regular). This isn’t a mechanical refactoring that our IDEs can accomplish, as the code is all jumbled up in the conditionals. We will have to fish the relevant code out. First, we will invert the negative if statements to better see the cases: 29

1 2 3 4 5 private void update. Item. Quality. And. Sell. In(Item item) { if (item. name. equals("Aged Brie") || item. name. equals("Backstage passes to a TAFKAL 80 ETC concert" )) { if (item. quality < 50) { item. quality = item. quality + 1; 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 if (item. name. equals("Backstage passes to a TAFKAL 80 ETC concert" )) { if (item. sell. In < 11) { if (item. quality < 50) { item. quality = item. quality + 1; } } if (item. sell. In < 6) { if (item. quality < 50) { item. quality = item. quality + 1; } } } else { if (item. quality > 0 && !item. name. equals("Sulfuras, Hand of Ragnaros" )) { item. quality = item. quality - 1; } } 30

27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 if (!item. name. equals("Sulfuras, Hand of Ragnaros" )) { item. sell. In = item. sell. In - 1; }. . . } if (item. sell. In < 0) { if (item. name. equals("Aged Brie")) { if (item. quality < 50) { item. quality = item. quality + 1; } } else { if (item. name. equals("Backstage passes to a TAFKAL 80 ETC concert" )) { item. quality = 0; } else { if (item. quality > 0) { if (!item. name. equals("Sulfuras, Hand of Ragnaros" )) { item. quality = item. quality - 1; } } } 31

THE REFACTORING Tests are still green, let’s continue. 32

THE REFACTORING The easiest case seems to be the Aged Brie. Lines 2 -5 tell us Aged Brie increases in quality by 1. Lines 33 -37 tell us that Aged Brie increases in quality by 1 if the sell. In expired. Let’s carefully extract this logic to a separate method: 33

1 2 3 4 5 6 7 8 9 private void update. Aged. Brie(Item item) { if (item. quality < 50) { item. quality = item. quality + 1; } item. sell. In = item. sell. In - 1; if (item. sell. In < 0 && item. quality < 50) { item. quality = item. quality + 1; } } 34

The Refactoring The update. Item. Quality. And. Sell. In now looks like this: 35

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 private void update. Item. Quality. And. Sell. In(Item item) { if (item. name. equals("Aged Brie")) { update. Aged. Brie(item); } else { if (item. name. equals("Backstage passes to a TAFKAL 80 ETC concert" )) { if (item. quality < 50) { item. quality = item. quality + 1; if (item. name. equals("Backstage passes to a TAFKAL 80 ETC concert" )) { if (item. sell. In < 11) { if (item. quality < 50) { item. quality = item. quality + 1; } } if (item. sell. In < 6) { if (item. quality < 50) { item. quality = item. quality + 1; } } } else { if (item. quality > 0 && !item. name. equals("Sulfuras, Hand of Ragnaros" )) { item. quality = item. quality - 1; } } 36

29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 if (!item. name. equals("Sulfuras, Hand of Ragnaros" )) { item. sell. In = item. sell. In - 1; }. . . if (item. sell. In < 0) { if (item. name. equals("Backstage passes to a TAFKAL 80 ETC concert" )) { item. quality = 0; } else { if (item. quality > 0) { if (!item. name. equals("Sulfuras, Hand of Ragnaros" )) { item. quality = item. quality - 1; } } } 37

THE REFACTORING Tests are green, let’s continue. 38

THE REFACTORING Let’s handle the backstage passes case next. Some of you may have noticed that the if on line 9 is the same as the one in line 5. Let’s unwrap that if and extract the relevant logic to a new method. Again, carefully! 39

1 2 private void update. Backstage. Passes(Item item) { if (item. quality < 50) { 3 item. quality = item. quality + 1; 4 5 6 7 8 if (item. sell. In < 11) { if (item. quality < 50) { item. quality = item. quality + 1; } 9 } 10 11 if (item. sell. In < 6) { 12 if (item. quality < 50) { 13 item. quality = item. quality + 1; 14 15 16 17 18 19 20 21 } } } item. sell. In = item. sell. In - 1; if (item. sell. In < 0) { item. quality = 0; } } 40

The Refactoring The update. Item. Quality. And. Sell. In method now looks nicer, it even fits on one screen! 41

1 2 3 4 5 6 7 8 9 10 private void update. Item. Quality. And. Sell. In(Item item) { if (item. name. equals("Aged Brie")) { update. Aged. Brie(item); } else if (item. name. equals("Backstage passes to a TAFKAL 80 ETC concert" )) { update. Backstage. Passes(item); } else { if (item. quality > 0 && !item. name. equals("Sulfuras, Hand of Ragnaros" )) { item. quality = item. quality - 1; } 11 12 if (!item. name. equals("Sulfuras, Hand of Ragnaros" )) { item. sell. In = item. sell. In - 1; } 13 14 15 16 17 18 19 20 21 22 23 if (item. sell. In < 0) { if (item. quality > 0) { if (!item. name. equals("Sulfuras, Hand of Ragnaros" )) { item. quality = item. quality - 1; } } } 42

THE REFACTORING Tests are (still) green, let’s continue. 43

THE REFACTORING Two cases left: regular and Sulfuras, a legendary item. If we take a look at the code now, we see that Sulfuras just doesn’t do anything: the quality and sell. In don’t decrease or increase. So that’s an easy method, it’s blank. So let’s extract the logic for the regular items: 44

1 2 3 4 5 6 7 8 9 private void update. Regular. Item(Item item) { if (item. quality > 0) { item. quality = item. quality - 1; } item. sell. In = item. sell. In - 1; if (item. sell. In < 0 && item. quality > 0) { item. quality = item. quality - 1; } } 10 11 private void update. Legendary. Item(Item item) {} 45

The Refactoring The update. Item. Quality. And. Sell. In method is now much nicer! 46

1 2 3 4 5 6 7 8 9 10 11 12 private void update. Item. Quality. And. Sell. In(Item item) { if (item. name. equals("Aged Brie")) { update. Aged. Brie(item); } else if (item. name. equals("Backstage passes to a TAFKAL 80 ETC concert")) { update. Backstage. Passes(item); } else if (item. name. equals("Sulfuras, Hand of Ragnaros")) { update. Legendary. Item(item); } else { update. Regular. Item(item); } } 47

The Refactoring We run the tests to make sure everything is still green (and it is), and now we stop and think: 48

The Refactoring This method is short, nice to read, and to the point. We can also now add the conjured item requirement we were given at the beginning (remember that? ), but can we do better? Should we do better? Yes, we can, and we will! 49

The Refactoring First, just to make things visual, let’s turn the if in the update. Item. Quality. And. Sell. In method to a switch: 50

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 private void update. Item. Quality. And. Sell. In(Item item) { switch (item. name) { case "Aged Brie": update. Aged. Brie(item); break; case "Backstage passes to a TAFKAL 80 ETC concert": update. Backstage. Passes(item); break; case "Sulfuras, Hand of Ragnaros": update. Legendary. Item(item); break; default: update. Regular. Item(item); break; } } 51

THE REFACTORING This should give us a hint about how to proceed. A switch hides an excellent opportunity for polymorphism. We will hide this switch in a factory method, create an abstract Item. Wrapper base class (we can’t touch the Item class because of the goblin), and let our items extend that class. 52

1 2 public abstract class Item. Wrapper { protected final Item item; 3 public Item. Wrapper(Item item) { this. item = item; } 4 5 6 7 public abstract void update(); 8 9 } 53

1 2 3 4 5 6 7 8 9 10 11 12 13 14 public class Item. Factory { public static Item. Wrapper create(Item item) { switch (item. name) { case "Aged Brie": return new Aged. Brie(item); case "Backstage passes to a TAFKAL 80 ETC concert": return new Backstage. Pass(item); case "Sulfuras, Hand of Ragnaros": return new Legendary. Item(item); default: return new Regular. Item(item); } } } 54

The Refactoring Each concrete class returned by the factory has exactly the same logic as we’ve seen in our extracted methods. Now the original class looks like this: 55

1 2 public class Gilded. Rose { Item[] items; 3 public Gilded. Rose(Item[] items) { this. items = items; } 4 5 6 7 public void update. Quality() { for (Item item : items) { update. Item. Quality. And. Sell. In(item); } } 8 9 10 11 12 13 private void update. Item. Quality. And. Sell. In(Item item) { Item. Factory. create(item). update(); } 14 15 16 17 } 56

The Refactoring We can even make this nicer by inlining the update. Item. Quality. And. Sell. In method, since it’s a one-liner now: 57

1 2 public class Gilded. Rose { Item[] items; 3 public Gilded. Rose(Item[] items) { this. items = items; } 4 5 6 7 public void update. Quality() { for (Item item : items) { Item. Factory. create(item). update(); } } 8 9 10 11 12 13 } 58

THE REFACTORING That’s the end of the refactoring part of the kata. We can say that the “big refactoring” we did with this code is transform it from a procedural style to an objectoriented style. 59

THE NEW FEATURE Finally, we can do what was asked of us: add support for conjured items. Let’s do just that. First, we’ll add a test. Then, we’ll create a concrete Item. Wrapper class called Conjured. Item, add a case to our switch and call it a day. Absolutely no changes required in our Gilded. Rose class. 60

1 2 3 4 public class Conjured. Item extends Item. Wrapper { public Conjured. Item(Item item) { super(item); } 5 @Override public void update() { if (item. quality > 1) { item. quality = item. quality - 2; } item. sell. In = item. sell. In - 1; if (item. sell. In < 0 && item. quality > 1) { item. quality = item. quality - 2; } } 6 7 8 9 10 11 12 13 14 15 16 } 61

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 public class Item. Factory { public static Item. Wrapper create(Item item) { switch (item. name) { case "Aged Brie": return new Aged. Brie(item); case "Backstage passes to a TAFKAL 80 ETC concert": return new Backstage. Pass(item); case "Sulfuras, Hand of Ragnaros": return new Legendary. Item(item); case "Conjured Mana Cake": return new Conjured. Item(item); default: return new Regular. Item(item); } } } 62

THE END This is the end of this kata. Can it be improved? Absolutely, and each and every one of you is encouraged to try it on their own. Try different languages, different refactorings, and most importantly, have fun! 63