When management says "don't refactor now" and you see this

While coding we meet sometimes one of our deamons: should I refactor what I see? When the dilemma grows it goes into: develop as it is or completely “rewrite” it?

A short story of one small refactor

Dilemmas, of course, yeah. Let’s just make it working!

Let’s look at this snippet:

// PREVIOUS CODE
var player:Object = guildUpdateFloor.player;
if (player != null && guildUpdateFloor.guild_id == _enemyGuild.name) {
	if (player.defeated == false) {
		(_enemyGuild.members[player.name] as GuildMember).cooldown = -1;
	} else {
		(_enemyGuild.members[player.name] as GuildMember).cooldown = int(player.defeated);
	}
}

It stood untouched for some longer time in there. And then another developer adds this:

else if (player && guildUpdateFloor.guild_id == _playerGuild.name) {
	if (player.defeated == false) {
		(_playerGuild.members[player.name] as GuildMember).cooldown = -1;
	} else {
		(_playerGuild.members[player.name] as GuildMember).cooldown = int(player.defeated);
	}
}

I couldn’t resist to rewrite all of that by actually understanding what it does.

// LITTLE BETTER
var playerData:Object = guildUpdateFloor.player;
if (playerData != null) {
	var guildMember:GuildMember = _enemyGuild.members[playerData.name] as GuildMember;

	if (guildUpdateFloor.guild_id == _enemyGuild.name || guildUpdateFloor.guild_id == _playerGuild.name) {
		guildMember.cooldown = player.defeated ? int(player.defeated) : -1;
	}
}

What have I changed? It’s shorter and some variables are introduced / changed.

(Asking about the language? ActionScript 3.)

Refactor and afterthoughts

Comparing the previous and “better” version it may look like - hey, yeah, code is shorter but now we can’t see logical paths at first look.

However, after deep insight you’ll notice this fact: previous snippet had 4 cases where 2 were rather virtual. In reality, it happened to have only two different cases which were easy and quite readable to be rewritten as one logical condition:

if (guildUpdateFloor.guild_id == _enemyGuild.name || guildUpdateFloor.guild_id == _playerGuild.name) {
    guildMember.cooldown = player.defeated ? int(player.defeated) : -1;
}

If you would be interested making those cases independent then why not:

if (guildUpdateFloor.guild_id == _enemyGuild.name) {
    guildMember.cooldown = player.defeated ? int(player.defeated) : -1;
}
else if (guildUpdateFloor.guild_id == _playerGuild.name) {
    guildMember.cooldown = player.defeated ? int(player.defeated) : -1;
}

But how do you feel now? Do you expect cooldown  to be calculated differently in some longer time?

If not, then why not doing another trick?

if (guildUpdateFloor.guild_id != _enemyGuild.name) { }
else if (guildUpdateFloor.guild_id != _playerGuild.name) { }
else {
    guildMember.cooldown = player.defeated ? int(player.defeated) : -1;
}

Notice the != in place of ==. It may look as a stupid trick. But note the fact - two first conditions (those != IFs) are splitted and cooldown calculation is written only in one place. In case of changing only one case you would probably remove the “else” part.

Much more cases?

Your first think may be “use switch”. But what would you change that way?

var passEvent: boolean = false;

if (eventType == Events.PLAYER_KILLED) {
    passEvent = true;
}
else if (eventType == Events.ENEMY_KILLED) {
    passEvent = true;
}
else if (eventType == Events.GOLD_ACCLAIMED) {
    passEvent = true;
}
else if (eventType == Events.GOLD_SPENT) {
    passEvent = true;
}

if (passEvent) {
    dispatchEvent(event);
}

Instead, let’s try inverting checks:

var passEvent: boolean = true;

if (eventType == Events.PLAYER_KILLED) { }
else if (eventType == Events.ENEMY_KILLED) { }
else if (eventType == Events.GOLD_ACCLAIMED) { }
else if (eventType == Events.GOLD_SPENT) { }
else {
    // now it's easier to not omit this line in all needed cases!
    passEvent = true;
}

if (passEvent) {
    dispatchEvent(event);
}

Or if your code is really as simple as the above you could indeed introduce a switch expression.

var passEvent: boolean = false;
switch (eventType) {
    case Events.PLAYER_KILLED:
    case Events.ENEMY_KILLED:
    case Events.GOLD_ACCLAIMED:
    case Events.GOLD_SPENT:
        passEvent = true;
}

if (passEvent) {
    dispatchEvent(event);
}

Sometimes just looking at code in an inverted way than it is written helps to keep it more clean and less buggy when another person develops it more in a hurry.

Summary

My message here is only this - developers, look at and look into the code of your fellows! You are as responsible for old production code snippets as the authors.