logo
Articletdd

TDD refactoring phase

Views: 496 Created: 2019-05-22 Read time: 10 minutes
Post Preview
Tags:

1)     Preface

You finally bought that gym membership and you are fired up to get into shape. You have been coding five years straight, with no to little exercise or movement whatsoever. To keep the morale high, you have signed with your work buddy (Johnny) that wants to prepare for that upcoming summertime the same as you.

The start is rough, but you get used to the barbell and different machines available at the site. Although you train the same movements together, your buddy seems to always find some alternative way of performing the exercise. Thanks to that he engages more muscle groups. You keep it nice and easy and do the same exact couple of exercises you were shown by the trainer during your first day in.

Besides that, Johnny is trying to push himself just a bit every time in terms of the weights themselves. He might also do more repetitions if he is not comfortable yet to put that extra plate on the barbell. All in all, he is pushing his limits. You, on the other hand, have raised the weight just once on each exercise in the past three months since you signed up.

 After the hard work is done and you both left the gym, you offer Johnny to go to the local pizzeria. You earned it, right? Johnny thanks you kindly, sits on the nearest bench, takes out his prepared healthy meal and calmly eats it enjoying the sun. You shrug your shoulders and head for that chefs special with extra toppings. I earned it you repeat to yourself.

It is not hard to imagine what both men might be thinking after 4-5 months when standing in front of the mirror. Johnny might think: hmm, it did not know I had these muscles over here? What shall I do with all those 36 trousers as I am 32 right now? How come by lower back does not cause me problems even after sitting at my desk for hours straight? You, on the other hand, do not see any difference at all.. both visually and mentally. You even gained an inch in your waist thanks to all those earned pizzas.

What made Johnny succeed over you were adjustments, minimal adjustments. Changes that were unnoticeable on day by day basis. Over 5 months, though, Johnny could see the accumulated benefits and enjoy that holiday in Spain to the fullest (oh he did).

Code refactoring is doing precisely what Johnny was doing every time in the gym (and outside) that you were not. He was adding that extra 5 pounds, running on a steeper and steeper surface, doing that extra set of reps when you were not; tweaking the diet just a bit every week. Programming-wise these are the similar small things as changing a name of a variable, so it is more self-explanatory; putting a particular block of code in a private method and naming it clearly; replacing that anonymous class with a much more concise lambda expression. If done every time at the end of your TDD cycle, your code will be ripped, shiny, positive and ready for any challenge that lies in front of it.

 

Note
Note

 

2)     TDD refactoring phase: gaining perspective

First of all will try to understand what are we losing when we do not refactor our code at all. We will take a look at a utility method for time processing that has not seen any refactoring to it yet:

 

SUT Code


public static String toTime(int hours, int minutes, int seconds)
{
    if(hours < 0 || hours > 23){
        throw new RuntimeException("Not proper hour value");
    }

    if(minutes < 0 || minutes > 59){
        throw new RuntimeException("Not proper minutes value");
    }

    if(seconds < 0 || seconds > 59){
        throw new RuntimeException("Not proper seconds value");
    }

    String time = new String();
    if( hours < 10 ){
        time += "0" + hours;
    }else{
        time += "" + hours;
    }

    buf.append( ":" );

    if( minutes < 10 ){
        time += "0" + minutes;
    }else{
        time += "" + minutes;
    }

    time += ":";

    if( seconds < 10 ){
        time += "0" + seconds;
    }else{
        time += "" + seconds;
    }

    return time;
}

Do you know straight away what this piece of code is doing? I am quite sure you did figure it out, but it took you way more time than it should. The above method has some serious flaws, which could be listed as:

       The name of the method itself is not very informative and meaningful

       There is a lot of duplication

       It would get more time and money consuming to maintain with every change applied to it in this state.

       Quite a bit of magic numbers is present.

 

Now let us take a look at how this method might look like if we spent just a little bit of time on making it better. Of course, we would keep the original functionality intact:

 

SUT Code


static final String TIME_PART_SEPARATOR = ":";
static final int MAX_HOUR = Integer.valueOf(23);
static final int MAX_MINUTE_SECOND = Integer.valueOf(59);
static final int UNNECESSARY_NON_LEADING_ZERO_MIN_VALUE = Integer.value(10);

public static String formatTimeWithoutOffset(int hours, int minutes, int seconds){
    validateHourPart(hours);
    validateMinutePart(minutes);
    validateSecondsPart(seconds);

    return new StringBuilder()
             .append(addLeadingZero(hours))
             .append(TIME_PART_SEPARATOR)
             .append(addLeadingZero(minutes))
             .append(TIME_PART_SEPARATOR)
             .append(addLeadingZero(seconds));
			 .toString();
}

public static String addLeadingZero(int timePart){
    if(timePart < UNNECESSARY_NON_LEADING_ZERO_MIN_VALUE){
        return "0" + timePart;
    }else{
        return "" + timePart;
    }
}

private static void validateHourPart(int hours){
    if(hours < 0 || hours > MAX_HOUR){
        throw new RuntimeException("Not a proper hour value");
    }
}

private static void validateMinutePart(int minutes){
    if(minutes < 0 || minutes > MAX_MINUTE_SECOND){
        throw new RuntimeException("Not a proper minutes value");
    }
}

private static void validateSecondsPart(int seconds){
    if(seconds < 0 || seconds > MAX_MINUTE_SECOND){
        throw new RuntimeException("Not a proper seconds value");
    }
}

This looks a lot better. We have added the following improvements:

       Replaced string concatenation with a more reliable and maintainable StringBuilder.

        Validation has been moved to specialised private methods.

       Descriptive constants have been added, giving greater meaning to the plain symbols and numbers.

       Common code for resolving leading zero has been encapsulated in another public method. Possibly other clients might make use of it also.

 

Thanks to a few tweaks, our method is short, clean and concise. It does not take much time to understand how it works and where the next change should be applied.

 

Tip
Tip

 

3)     The final tdd step

In terms of TDD, refactoring is the last step of each of our cycles. It is the icing on the cake.

Without it, something is lacking in the whole experience. Something is not right. Let us take at the following example. We were given a task of extending the game engine by a feature of levelling our hero. Besides the increase in the level itself, he might gain additional bonuses. We have written a few test cases which led to this implementation:

 

SUT Code


public void levelUp(Hero hero){
	hero.setLevel(hero.getLevel() + 1);

	if(hero.getLevel() % 10 == 0){
		hero.addSpell(gameEngine.generateSpecialSpell());
	}

	if(hero.getLevel() % 5 == 0){
		hero.addMoney(gameEngine.generateBonusMoney());
	}

	if(hero.getActiveBuff() == null){
		hero.setActiveBuff(gameEngine.generateRandomBuff());
	}
}

It is not bad, but we could tune it up a notch for sure:

 

SUT Code


static final int SPECIAL_SPELL_EVERY_OTHER_LEVEL = 10;
static final int BONUS_MONEY_EVERY_OTHER_LEVEL = 5;

public void levelUp(Hero hero){
	hero.gainOneLevel();

	addSpellAtLevelUp(hero);
	addBonusMoneyAtLevelUp(hero);
	generateBuffAtLevelUp(hero);
}

private void addSpellAtLevelUp(Hero hero){
	if(hero.getLevel() % SPECIAL_SPELL_EVERY_OTHER_LEVEL == 0){
		hero.addSpell(gameEngine.generateSpecialSpell());
	}
}

private void addBonusMoneyAtLevelUp(Hero hero){
	if(hero.getLevel() % BONUS_MONEY_EVERY_OTHER_LEVEL == 0){
		hero.addMoney(gameEngine.generateBonusMoney());
	}
}

private void generateBuffAtLevelUp(Hero hero){
	if(hero.getActiveBuff() == null){
		hero.setActiveBuff(gameEngine.generateRandomBuff());
	}
}

That looks better. We have added new private abstractions for different parts of the implementation. Now our public method is a lot more readable.

 

Note
Note

 

4) Refactoring a bigger case

Sometimes it is more natural to extract a piece of code into a separate class rather than putting it inside a private member. Especially if the SUT method is starting to get a bit bloated and the number of tests and their complexity grows to a concerning level.

This time we will try to steal some treasure from a dragon:

 

SUT Code


public boolean stealGoldFromDragon(Hero hero, Dragon dragon,
								   Integer amountToSteal, List<Treasure> treasures,
								   GoodType goodType)
		throws HeroSlainedByDragonException, HeroIsAChickenExcpetion {
	if(goodType.equals(GoodType.GOLD){
		if(amountToSteal == null || amountToSteal <= 0){
			throw new RuntimeException("Do not chicken right now!");
		}

		if(amountToSteal > 2000){
			throw new RuntimeException("You cannot try to steal
										more than 2000 gold at a time.");
		}

		if(amountToSteal >= Integer.valueOf(1000)){
			boolean dragonSlained = gameEngine.fightWithDragon(hero, dragon);

			if(!dragonSlained){
				throw new HeroSlainedByDragonException();
			}
		}

		return gameEngine.stealGold(hero, dragon);
	}else{
		if(CollectionUtils.isEmpty(treasures)){
			throw new HeroIsAChickenExcpetion();
		}

		return gameEngine.stealTreasures(hero, dragon, treasures);
	}
}

The method is starting to get hard to read. Also, the number of input params is getting out of control. We have the type of goods, the amount of gold, treasures. We have to do something about it. Also it is quite hard to write any more tests for this class. This is regarding their naming and size. The GoodType gives us a chance to extract and split these methods into per-good-type:

 

SUT Code


static final int MIN_AMOUNT_THAT_WAKES_UP_DRAGON = 1000;
static final int MAX_GOLD_TO_STEAL = 2000;

public boolean stealGoldFromDragon(Hero hero, Dragon dragon,
								   Integer amountToSteal)
		throws HeroSlainedByDragonException {
	validateAllowedAmountOfGold(amountToSteal);

	if(dragonWokeUp(amountToSteal)){
		boolean dragonSlained = gameEngine.fightWithDragon(hero, dragon);

		if(!dragonSlained){
			throw new HeroSlainedByDragonException();
		}
	}

	return gameEngine.stealGold(hero, dragon);
}

private boolean dragonWokeUp(Integer amountToSteal){
	if(amountToSteal >= Integer.valueOf(MIN_AMOUNT_THAT_WAKES_UP_DRAGON)){
		return true;
	}

	return false;
}

private void validateAllowedAmountOfGold(Integer amountToSteal){
	if(amountToSteal == null || amountToSteal <= 0){
		throw new RuntimeException("Do not chicken right now!");
	}

	if(amountToSteal > MAX_GOLD_TO_STEAL){
		throw new RuntimeException("You cannot try to steal more
									than 2000 gold at a time.");
	}
}

 

SUT Code


public boolean stealTreasureFromDragon(Hero hero, Dragon dragon,
									   List<Treasure> treasures)
		throws HeroIsAChickenExcpetion {
	validateHeroDidNotChickenOut(treasures);
	return gameEngine.stealTreasures(hero, dragon, treasures);
}

private void validateHeroDidNotChickenOut(List<Treasure> treasures){
	if(CollectionUtils.isEmpty(treasures)){
		throw new HeroIsAChickenExcpetion();
	}
}

Now we can alter our tests. They will be more specialised, definitely shorter and to the point.

 

Note
Note

 

5) Refactoring the test code itself

Improving the SUT is not everything we can do regarding the refactoring phase. Test code is also a very precious code. If we won't take care of it the same way as we take care of production code, it will rotten away quite soon. And this always ends up in the same way. You can read about it here:

 

Reference
Reference
generalart
Ignoring your tests

 

We will not let this happen. That is why, during every refactoring step of the TDD cycle, we will also revise the test code as well for any duplication or possible enhancements.

This time our Hero will be fighting with a MightyDragon. We will try to make sure he is victorious in the battle:

 

Test Code


@Test
public void heroWinsWithMightyDragon(){
	Hero hero = new Hero();
	hero.setLevel(Hero.EXPERT_MIN_LEVEL);
	hero.addSpell(Spells.WEAKEN_DRAGON);

	MightyDragon dragon = new MightyDragon();

	List<Buff> requiredBuffs = Arrays.asList(new Buff[]{
		  new Buff(BuffType.INCREASED_STRENGTH),
		  new Buff(BuffType.INCREASED_AGILITY),
		  new Buff(BuffType.INCREASED_SPEED),}
		);

	for(Buff buff: requiredBuffs){
		hero.setActiveBuff(buff);

		boolean dragonSlained = gameControllerSUT.fightWithMightyDragon(hero, dragon);

		assertTrue(dragonSlained);
		assertEquals(hero.getFame(), Fame.GODDLY);
	}
}

In this test, we expect Hero to win. For that to happen, specific requirements need to be met. This translates to the minimum level, spells and special buffs. Without these the battle is certain to be lost.

There is a lot of room for improvement here when it comes to the test method. It does it's job, but we can make it even better. Here we go:

 

Test Code


@ParameterizedTest
@EnumSource(value = BuffType.class, mode = EnumSource.Mode.INCLUDE,
		names = {"INCREASED_STRENTH" ,"INCREASED_AGILITY" ,"INCREASED_SPEED"})
public void shouldWinWithMightyDragon_givenHeroPowerfulEnough(BuffType buffType){
	// Given
	Hero hero = new HeroBuilder()
		.withLevel(Hero.EXPERT_MIN_LEVEL);
		.withSpell(Spells.WEAKEN_DRAGON);
		.withActiveBuff(buff)
		.build();

	MightyDragon dragon = new MightyDragon();

	// When
	boolean dragonSlained = gameControllerSUT.fightWithMightyDragon(hero, dragon);

	// Then
	assertThat(dragonSlained)
		.describedAs("Dragon has been slained")
		.isTrue();

	assertThat(hero.getFame())
		.describedAs("Heros fame after defeating the dragon")
		.isEqualTo(Fame.GODDLY);
}

This looks a lot better now. We have improved the following:

       (L2,3):  Instead of a loop that runs for each of the required BuffType's, we have introduced a @ParameterizedTest feature available in Junit 5. This allows better control of the possible params that meet the test. Everything is also more readable and better documented.

       (L5):  The name follows the BDD style of describing the test cases.

       (L7):  Instead of creating the Hero object and using setter explicitly, we have created a builder that makes the process of creation more clear and reusable.

       (L6,15,18):  We have arranged the content of the test method into BDD styled Given, When, Then. That adds to the readability.

       (L19-25):  We have taken advantage of the Assertj library and levelled up the assertions.

 

6)     Conclusion

 

Summary

 

 

There are three significant steps in TDD. Each one of them is equally important. If you won't neglect the refactoring part, your code's evolution will keep advancing and advancing. Who knows where it would end up?

 

Need more insight?
Repository
Repository
Glossary
Glossary
Tags:
Reference
You may also like:
single-responsibility-principle-and-testing
Single responsibility principle and testing
Comments
Be the first to comment.
Leave a comment