KATA REFACTORING

Last Koding dojo in BecodeMyFriend, was about the kata
https://github.com/wouterla/GildedRose/
and all of us where jumping between solutions as with deep in the code.
When i go back to home i decided to did that kata with an stricted procedure , and this is the results, i had expend 5 pomodoros of 30 minutes

The flow was :
You could follow all my keystrokes with the file of MoonEdit soft , KataGildedRose

1. prepare test over Item Class
   


2. prepare test for GildeRose Class.


3. replace magic number with symbolic constant


4. write code to complete the conjured item test


5. consolidate conditional expresions


6. extract method for increments , decrements of values, and expired()
   
7. Split conditionals, substitute else, split nested if clauses, change sucesive if with && operator.


8. noTopQuality() and hasQuality() conditions, send to increaseQuality and decreaseQuality method couse they always are checked before those methods


9. solve double negations


10. erase inclusive at sentences with exclusive clauses ( if (isA) && (!isB) ==> so isA


11. try to extract common clauses, (ad !sufuras if posible) then extract it


12. extract method of each item behaviour


13. join the code of conjured item to the new behaviour sistem


14. replace conditional with polimorphism





1. prepare test over Item Class
   download the file
   
import junit.framework.TestCase;


public class ItemTest extends TestCase {
//All items have a SellIn value
public void testHasSellInValue()
{
int expectedResult =23;
int myItemSellIn=23;
int myItemQuality = 55;
Item myItem= new Item("myName",myItemSellIn,myItemQuality);
assertEquals(expectedResult, myItem.getSellIn());
}
//All items have a Quality value
public void testHasQualityValue()
{
int expectedResult =55;
int myItemSellIn=23;
int myItemQuality = 55;
Item myItem= new Item("myName",myItemSellIn,myItemQuality);
assertEquals(expectedResult, myItem.getQuality());
}
//At the end of each day our system lowers both values for every item
public void testCouldBeDecreasedSellInValue()
{
int expectedResult =22;
int myItemSellIn=23;
int myItemQuality = 55;
Item myItem= new Item("myName",myItemSellIn,myItemQuality);
myItem.setSellIn(myItem.getSellIn()-1);
assertEquals(expectedResult, myItem.getSellIn());
}
public void testCouldBeDecreasedQuality()
{
int expectedResult =54;
int myItemSellIn=23;
int myItemQuality = 55;
Item myItem= new Item("myName",myItemSellIn,myItemQuality);
myItem.setQuality(myItem.getQuality()-1);
assertEquals(expectedResult, myItem.getQuality());
}


}

   


2. prepare test for GildeRose Class. here i make a little change to avoid use of reflection for test private fields. i 'd change items from private to public scope
   download the testClass file of GildedRose
   
import junit.framework.TestCase;


public class GildedRoseTest extends TestCase {
private static final int DEXTERY_VEST = 0;
private static final int AGED_BRIE = 1;
private static final int ELIXIR_MONGOOSE = 2;
private static final int SULFURAS = 3;
private static final int BACKSTAGE_PASSES = 4;
private static final int CONJURED = 5;

// Once the sell by date has passed, Quality degrades twice as fast
public void testDegradeTwiceWhenSellInDatePassed (){
initializeGildedRose();
int valueBeforeLastNormalDegradation =0;
int valueAfterLastNormalDegradation =0;

while (GildedRose.items.get(DEXTERY_VEST).getSellIn()>0) {
valueBeforeLastNormalDegradation = GildedRose.items.get(DEXTERY_VEST).getQuality();
GildedRose.updateQuality();
valueAfterLastNormalDegradation = GildedRose.items.get(DEXTERY_VEST).getQuality();
}
int qualityNormalDegradationRate = valueBeforeLastNormalDegradation-valueAfterLastNormalDegradation;
int qualityBeforeDegradeAfterSellInPassed = GildedRose.items.get(DEXTERY_VEST).getQuality();

GildedRose.updateQuality();

assertEquals(qualityBeforeDegradeAfterSellInPassed - (qualityNormalDegradationRate*2),GildedRose.items.get(DEXTERY_VEST).getQuality());
}
// The Quality of an item is never negative
public void testNeverNegativeQuality(){
initializeGildedRose();
updateTillReachQuality(ELIXIR_MONGOOSE, 0);

GildedRose.updateQuality();

assertTrue(GildedRose.items.get(ELIXIR_MONGOOSE).getQuality()>=0);
}
// "Aged Brie" actually increases in Quality the older it gets
public void testAgedBrieIncrementsQualiyAtUpdate(){
initializeGildedRose();
int beforeQuality = GildedRose.items.get(AGED_BRIE).getQuality();
GildedRose.updateQuality();
int afterQuality = GildedRose.items.get(AGED_BRIE).getQuality();

assertTrue(afterQuality>beforeQuality);
}
// The Quality of an item is never more than 50
public void testqualityNeverMoreThanFifty(){
initializeGildedRose();
updateTillReachQuality(AGED_BRIE, 45);
for (int timesToGuaranteeAsMuchAsCanFifty = 0; timesToGuaranteeAsMuchAsCanFifty < 5; timesToGuaranteeAsMuchAsCanFifty++) { GildedRose.updateQuality(); } GildedRose.updateQuality(); assertTrue(GildedRose.items.get(AGED_BRIE).getQuality()<=50); } // "Sulfuras", being a legendary item, never has to be sold or decreases in Quality public void testSulfurasNoSellInAndNoLooseQuality(){ initializeGildedRose(); int beforeQuality = GildedRose.items.get(SULFURAS).getQuality(); int beforeSellIn = GildedRose.items.get(SULFURAS).getSellIn(); GildedRose.updateQuality(); int afterQuality = GildedRose.items.get(SULFURAS).getQuality(); int afterSellIn = GildedRose.items.get(SULFURAS).getSellIn(); assertTrue(beforeSellIn == afterSellIn); assertTrue(beforeSellIn==0); assertTrue(afterQuality==beforeQuality); } // "Backstage passes", like aged brie, increases in Quality as it's SellIn 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 public void testBackStageBehaviour(){ initializeGildedRose(); updateTillSellIn(BACKSTAGE_PASSES,11); int qualityBeforeAnUpdateToTenDays = GildedRose.items.get(BACKSTAGE_PASSES).getQuality(); GildedRose.updateQuality(); int qualityAfterAnUpdateToTenDays = GildedRose.items.get(BACKSTAGE_PASSES).getQuality(); updateTillSellIn(BACKSTAGE_PASSES,6); int qualityBeforeAnUpdateToFiveDays = GildedRose.items.get(BACKSTAGE_PASSES).getQuality(); GildedRose.updateQuality(); int qualityAfterAnUpdateToFiveDays = GildedRose.items.get(BACKSTAGE_PASSES).getQuality(); updateTillSellIn(BACKSTAGE_PASSES,1); GildedRose.updateQuality(); int qualityCeroDaysLeft = GildedRose.items.get(BACKSTAGE_PASSES).getQuality(); int qualityGapAtTen = qualityAfterAnUpdateToTenDays-qualityBeforeAnUpdateToTenDays; int qualityGapAtFive = qualityAfterAnUpdateToFiveDays - qualityBeforeAnUpdateToFiveDays; assertEquals(qualityGapAtTen, 2); assertEquals(qualityGapAtFive, 3); assertTrue(qualityCeroDaysLeft==0); } // "Conjured" items degrade in Quality twice as fast as normal items public void testConjuredItemDegradesTwiceAsNormal (){ initializeGildedRose(); int valueBeforeLastNormalDegradation = GildedRose.items.get(DEXTERY_VEST).getQuality(); int valueBeforeDegradation = GildedRose.items.get(CONJURED).getQuality(); GildedRose.updateQuality(); int valueAfterLastNormalDegradation = GildedRose.items.get(DEXTERY_VEST).getQuality(); int valueAfterDegradation = GildedRose.items.get(CONJURED).getQuality(); int qualityNormalDegradationRate = valueBeforeLastNormalDegradation - valueAfterLastNormalDegradation; int qualityConjuredDegradationRate = valueBeforeDegradation - valueAfterDegradation; assertEquals(qualityNormalDegradationRate*2 , qualityConjuredDegradationRate); } public void testConjuredDegradeTwiceWhenSellInDatePassed (){ initializeGildedRose(); int valueBeforeLastNormalDegradation =0; int valueAfterLastNormalDegradation =0; while (GildedRose.items.get(CONJURED).getSellIn()>=0) {
valueBeforeLastNormalDegradation = GildedRose.items.get(CONJURED).getQuality();
GildedRose.updateQuality();
valueAfterLastNormalDegradation = GildedRose.items.get(CONJURED).getQuality();
}
int qualityNormalDegradationRate = valueBeforeLastNormalDegradation-valueAfterLastNormalDegradation;
int qualityBeforeDegradeAfterSellInPassed = GildedRose.items.get(CONJURED).getQuality();

GildedRose.updateQuality();

assertEquals(qualityBeforeDegradeAfterSellInPassed - (qualityNormalDegradationRate*2),GildedRose.items.get(CONJURED).getQuality());
}


private void updateTillSellIn(int itemNumber, int sellInBottom) {
while (GildedRose.items.get(itemNumber).getSellIn() >= sellInBottom) {
GildedRose.updateQuality();
}
}
private void initializeGildedRose() {
GildedRose.main(null);
}

private void updateTillReachQuality(int itemNumber, int qualityToReach) {
while (GildedRose.items.get(itemNumber).getQuality() <= qualityToReach) { GildedRose.updateQuality(); } } }
   


3. replace magic number with symbolic constant


4. write code to complete the conjured item test


5. consolidate conditional expresions


6. extract method for increments , decrements of values, and expired()
   after this we gat the code
   
import java.util.ArrayList;
import java.util.List;


public class GildedRose {

private static List items = null;

/**
* @param args
*/
private final static String DEXTERY_VEST = "+5 Dexterity Vest";
private final static String AGED_BRIE = "Aged Brie";
private final static String ELIXIR_MONGOOSE = "Elixir of the Mongoose";
private final static String SULFURAS = "Sulfuras, Hand of Ragnaros";
private final static String BACKSTAGE_PASSES = "Backstage passes to a TAFKAL80ETC concert";
private final static String CONJURED = "Conjured Mana Cake";
private final static int MAX_QUALITY = 50;
private final static int MIN_QUALITY = 0;

public static void main(String[] args) {

System.out.println("OMGHAI!");

items = new ArrayList();
items.add(new Item(DEXTERY_VEST, 10, 20));
items.add(new Item(AGED_BRIE, 2, 0));
items.add(new Item(ELIXIR_MONGOOSE, 5, 7));
items.add(new Item(SULFURAS, 0, 80));
items.add(new Item(BACKSTAGE_PASSES, 15, 20));
items.add(new Item("Conjured Mana Cake", 3, 6));

updateQuality();
}


public static void updateQuality()
{
int conjuredQualityBeforeUpdate = getConjuredQuality();
for (int i = 0; i < items.size(); i++) { if ((!isAged(i) && !isBackStage(i)) { if (hasQuality(i)) { if (!isSulfuras(i)) { decrementQuality(i); } } } else { if (noTopQuality(i)) { incrementQuality(i); if (isBackStage(i)) { if (items.get(i).getSellIn() < 11) { if (notTopQuality(i)) { incrementQuality(i); } } if (items.get(i).getSellIn() < 6) { if (noTopQuality(i)) { incrementQuality(i); } } } } } if (!isSulfuras(i)) { decrementSellIn(i); } if (expired(i)) { if (!isAged(i)) { if (!isBackStage(i)) { if (hasQuality(i)) { if (!isSulfuras(i)) { decrementQuality(i); } } } else { looseAllQuality(i); } } else { if (noTopQuality(i)) { incrementQuality(i); } } } } int conjuredQualityAfterUpdate = getConjuredQuality(); int normalItemQualityChange = conjuredQualityAfterUpdate - conjuredQualityBeforeUpdate); setConjuredQuality(getConjuredQuality() + normalItemQualityChange); } private static int getConjuredQuality(){ return getQualityByName(CONJURED); } private static void setConjuredQuality(int amount){ setConjuredQualityByName(CONJURED,amount); } private static void setConjuredQualityByName(String name,int amount){ for (int i = 0; i < items.size(); i++) { if (name.equals(items.get(i).getName())) items.get(i).setQuality(amount); } } private static int getQualityByName(String name){ for (int i = 0; i < items.size(); i++) { if (name.equals(items.get(i).getName())) return items.get(i).getQuality(); } return 0; } private static boolean isAged(int i) { return AGED_BRIE.equals(getNameByIndex(i)); } private static boolean isBackStage(int i) { return BACKSTAGE_PASSES.equals(getNameByIndex(i)); } private static boolean isSulfuras(int i) { return SULFURAS.equals(getNameByIndex(i)); } private static String getNameByIndex(int i) { return items.get(i).getName(); } private static void decrementQuality(int i) { changeQuality (i,- 1); } private static void incrementQuality(int i) { changeQuality(i,1); } private static void changeQuality(int i,int amount) { items.get(i).setQuality(items.get(i).getQuality() + amount); } private static boolean hasQuality(int i) { return (items.get(i).getQuality() > MIN_QUALITY);
}
private static boolean noTopQuality(int i)
{
return (items.get(i).getQuality() < MAX_QUALITY); } private static void looseAllQuality(int i) { items.get(i).setQuality(MIN_QUALITY); } private static void decrementSellIn(int i) { items.get(i).setSellIn(items.get(i).getSellIn() - 1); } private static boolean expired(int i) { return (items.get(i).getSellIn() < 0); } }
   
7. Split conditionals, substitute else, split nested if clauses, change sucesive if with && operator.

this only the code changed

public static void updateQuality()
{
int conjuredQualityBeforeUpdate = getConjuredQuality();
for (int i = 0; i < items.size(); i++) { //hasquality() and no topQuality are quality pro if ((!isAged(i) && !isBackStage(i))&&(hasQuality(i))&&(!isSulfuras(i))) { decrementQuality(i); } if ((!((!isAged(i) && !isBackStage(i)))&&(noTopQuality(i)))) { incrementQuality(i); } if ((!((!isAged(i) && !isBackStage(i)))&&(noTopQuality(i))&&(isBackStage(i))&&(items.get(i).getSellIn() < 11)&&(noTopQuality(i)))) { incrementQuality(i); } if ((isBackStage(i))&&(items.get(i).getSellIn() < 6)&& (noTopQuality(i))) { incrementQuality(i); } if (!isSulfuras(i)) { decrementSellIn(i); } if ((expired(i))&&(!isAged(i))&&(!isBackStage(i))&&if (hasQuality(i))&&if (!isSulfuras(i))) { decrementQuality(i); } if (((expired(i)&&(!isAged(i)) &&(isBackStage(i)))) { looseAllQuality(i); } if ((isAged(i))&&(noTopQuality(i))) { incrementQuality(i); } } int conjuredQualityAfterUpdate = getConjuredQuality(); int normalItemQualityChange = conjuredQualityAfterUpdate - conjuredQualityBeforeUpdate; setConjuredQuality(getConjuredQuality() + normalItemQualityChange); }

8. noTopQuality() and hasQuality() conditions, send to increaseQuality and decreaseQuality method couse they always are checked before those methods


9. solve double negations


10. erase inclusive at sentences with exclusive clauses ( if (isA) && (!isB) ==> so isA


11. try to extract common clauses, (ad !sufuras if posible) then extract it


public static void updateQuality()
{
int conjuredQualityBeforeUpdate = getConjuredQuality();
for (int i = 0; i < items.size(); i++) { //extrac condition common if (!isSulfuras(i))) { decrementSellIn(i); if (isAged(i) || isBackStage(i)) { incrementQuality(i); } else { decrementQuality(i); } if (isBackStage(i)) { if (expired(i) { looseAllQuality(i); } if ((items.get(i).getSellIn() < 11)) { incrementQuality(i); } if ((items.get(i).getSellIn() < 6)) { incrementQuality(i); } } if ((expired(i))&&(!isAged(i))&&(!isBackStage(i)) { decrementQuality(i); } if ((isAged(i))) { incrementQuality(i); } } } int conjuredQualityAfterUpdate = getConjuredQuality(); int normalItemQualityChange = conjuredQualityAfterUpdate - conjuredQualityBeforeUpdate; setConjuredQuality(getConjuredQuality() + normalItemQualityChange); }

12. extract method of each item behaviour


13. join the code of conjured item to the new behaviour sistem



public static void updateQuality()
{
for (int i = 0; i < items.size(); i++) { if (!isSulfuras(i)) { decrementSellIn(i); updateNormalItem(i); updateConjuredItem(i); if (isAged(i)) updateQualityAged(i); if (isBackStage(i)) updateQualityBackStage(i); } } } private static void updateConjuredItem(int i) { updateNormalItem(i); updateNormalItem(i); } private static void updateNormalItem (int i) { updateQualityNormalItem(i); if (expired(i)) updateQualityExpiredNormalItem(i); } private static void updateQualityExpiredNormalItem(int i) { if ((!isAged(i))&&(!isBackStage(i))) { decrementQuality(i); } } private static void updateQualityNormalItem(int i) { if (!isBackStage(i) && !isAged(i)) { decrementQuality(i); } } private static void updateQualityAged(int i) { incrementQuality(i); incrementQuality(i); } private static void updateQualityBackStage(int i) { incrementQuality(i); if (expired(i)) { looseAllQuality(i); } if ((items.get(i).getSellIn() < 11)) { incrementQuality(i); } if ((items.get(i).getSellIn() < 6)) { incrementQuality(i); } }

14. replace conditional with polimorphism



import java.util.ArrayList;
import java.util.List;


public class GildedRose {

private static List items = null;

/**
* @param args
*/
private final static String DEXTERY_VEST = "+5 Dexterity Vest";
private final static String AGED_BRIE = "Aged Brie";
private final static String ELIXIR_MONGOOSE = "Elixir of the Mongoose";
private final static String SULFURAS = "Sulfuras, Hand of Ragnaros";
private final static String BACKSTAGE_PASSES = "Backstage passes to a TAFKAL80ETC concert";
private final static String CONJURED = "Conjured Mana Cake";
//
public static void main(String[] args) {

System.out.println("OMGHAI!");

items = new ArrayList();
items.add(new Item(DEXTERY_VEST, 10, 20));
items.add(new Item(AGED_BRIE, 2, 0));
items.add(new Item(ELIXIR_MONGOOSE, 5, 7));
items.add(new Item(SULFURAS, 0, 80));
items.add(new Item(BACKSTAGE_PASSES, 15, 20));
items.add(new Item("Conjured Mana Cake", 3, 6));

updateQuality();
}

public static void updateQuality()
{
for (int i = 0; i < items.size(); i++) { if (!isSulfuras) { Article itemToUpdate = factoriaDeItems(i); itemToUpdate.decrementSellIn(); itemToUpdate.updateQuality(); copyValues(items.get(i),itemToUpdate); } } } private static void copyValues(Item item, Article itemToUpdate) { item.setQuality(itemToUpdate.getQuality()); item.setSellIn(itemToUpdate.getSellIn()); } private static Article factoriaDeItems(int i) { if (isAged(i)) return new AgedBrie(items.get(i)); if (isBackStage(i)) return new BackStage(items.get(i)); if (isConjured(i)) return new Conjured(items.get(i)); return new GenericArticle(items.get(i)); } private static boolean isAged(int i) { return AGED_BRIE.equals(getNameByIndex(i)); } private static boolean isBackStage(int i) { return BACKSTAGE_PASSES.equals(getNameByIndex(i)); } private static boolean isSulfuras(int i) { return SULFURAS.equals(getNameByIndex(i)); } private static boolean isConjured(int i) { return CONJURED.equals(getNameByIndex(i)); } private static String getNameByIndex(int i) { return items.get(i).getName(); } } public abstract class Article extends Item { private final static int MAX_QUALITY = 50; private final static int MIN_QUALITY = 0; public Article(String name, int sellIn, int quality) { super(name, sellIn, quality); } public Article (Item item) { super (item.getName(),item.getSellIn(), item.getQuality()); } public abstract void updateQuality(); protected void decrementSellIn() { setSellIn(getSellIn() - 1); } protected void decrementQuality() { if (hasQuality()) changeQuality (- 1); } protected void incrementQuality() { if (noTopQuality()) changeQuality(1); } protected void changeQuality(int amount) { setQuality(getQuality() + amount); } protected boolean hasQuality() { return (getQuality() > MIN_QUALITY);
}
protected boolean noTopQuality()
{
return (getQuality() < MAX_QUALITY); } protected void looseAllQuality() { setQuality(MIN_QUALITY); } protected boolean expired() { return (getSellIn() < 0); } } public class GenericArticle extends Article { public GenericArticle (Item item) { super (item); } public void updateQuality() { //extracted of method updateNormalItem() decrementQuality(); if (expired()) decrementQuality(); } } public class AgedBrie extends Article { public AgedBrie (Item item) { super (item); } public void updateQuality() { incrementQuality(); incrementQuality(); } } public class BackStage extends Article { public BackStage (Item item) { super (item); } public void updateQuality() { incrementQuality(); if ((getSellIn() < 11)) { incrementQuality(); } if ((getSellIn() < 6)) { incrementQuality(); } if (expired()) { looseAllQuality(); } } } public class Conjured extends Article { public Conjured (Item item) { super(item); } public void updateQuality() { decrementQuality(); decrementQuality(); if (expired()) { decrementQuality(); decrementQuality(); } } }

read next post to see
from Is-a to Has-a and from extends to implements