You wouldn’t believe how many programming questions I run across, on a daily basis, asking for help with programs that contain repetition, duplication and enough spaghetti code to feed half of Italy. Often times the person asking the question has a particular problem but they don’t know exactly where the problem originates. So they plop down a bunch of code and say “It is somewhere in there… I think”. Usually a debugging session is really time consuming in this type of situation. They have so much code that if there we to start at the beginning and step through it once, it would take 20 minutes just to hit the code they are looking for… that is if they know what they are looking for. I will save that for another topic.
To help those new and old to the industry find errors quicker, and help speed up their debugging, I created a small list of quick and easy refactoring tricks using C# (for demonstration purposes). Keep in mind that these tips can fit just about every programming language out there. The goal of these tips are to take that wild pile of spaghetti code and cut it down to a more manageable size. When we have less lines to deal with, the problem’s complexity tends to fall off dramatically. Refactoring is all about recognizing a code pattern and trying to simplify it down into a generic template.
This tip is for all those programmers out there who find themselves copying and pasting the same line over and over again with perhaps one small thing changed on each line. This is often a problem for those new to programming. They have a line of code that adds a string to a combobox control and heck, if that works, just copy the statement again and add another string. Below is an example…
// String of names
String[] names = { "John", "Mary", "Jim", "Jamie" };
// So the coder adds each name to the combobox one at a time.
// They may do this for hundreds of items, copying and pasting along.
comboBox1.Items.Add(names[0]);
comboBox1.Items.Add(names[1]);
comboBox1.Items.Add(names[2]);
comboBox1.Items.Add(names[3]);
This type of error is very very common and it is one of the easiest to spot and fix. If you are looking at your code and you see this wild list of statements that are virtually identical, except for one small change, 9 times out of 10 you have a perfect candidate for a refactoring into a loop. Below is one way you might be able to fix this…
String[] names = { "John", "Mary", "Jim", "Jamie" };
// Loop through each item in the array and add it.
foreach (String name in names)
{
comboBox1.Items.Add(name);
}
I have been able to literally cut out hundreds and thousands of lines with just one refactor like this. They have handed me something that is like 300 lines and I cut it down to 10 using a simple loop. I know they feel a bit silly when I do this, but instead of fishing around 300 lines we can see all 10 right on the screen. No more vertical scrolling or anything. Very clean and very easy to understand.
This is a bit harder to spot. Usually you will catch this type of refactor when you are actually writing the code. You get the sense of “Haven’t I done this before?” and that is when you should stop, look at the last several functions you wrote and see if somewhere else in the code you did the exact set of steps. The example code for this one is a bit simplistic and contrived, but it is meant only to serve the point. If we take the steps and put them into their own function we can then go back to the functions that are doing the steps and call our function instead. This can cut out 50% or more lines. If two functions have 10 duplicate steps, creating one function that does the 10 steps means that those two functions only have to call the function. This is saving on the duplication.
Perhaps we have two functions. One is calculating the distance our character has traveled on a game map and the second is calculating how much time before a fireball is about to hit our character in a fight. Both do different things but both need to calculate the distance between two coordinates in our 2D game. One of these functions may put in the distance formula for where we started and where we are now to calculate the distance while the other is constantly plugging in points between the character and the fireball that is hurling towards him.
private double distanceTraveled(Point origin, Point currentPosition) {
double xPointTraveled = Math.Pow((currentPosition.x - origin.x), 2.0);
double yPointTraveled = Math.Pow((currentPosition.y - origin.y), 2.0);
return Math.Sqrt(xPointTraveled + yPointTraveled);
}
...
private double distanceBetweenObjects(Point characterLocation, Point projectile) {
double xDistance = Math.Pow((projectile.x - characterLocation.x), 2.0);
double yDistance = Math.Pow((projectile.y - characterLocation.y), 2.0);
return Math.Sqrt(xDistance + yDistance);
}
These two functions can be “distilled” down into having one function that takes two sets of coordinates and then runs the distance formula. These two functions then can call it. This removes a lot of lines of code and duplication. Now when we want to tweak the distance formula we change one function instead of these two.
// this function called from two places.
private double distance(int x1, int y1, int x2, int y2)
{
double xPoints = Math.Pow((x2 - x1), 2.0);
double yPoints = Math.Pow((y2 - y1), 2.0);
return Math.Sqrt(xPoints + yPoints);
}
private double distanceTraveled(Point origin, Point currentPosition) {
return distance(origin.x, origin.y, currentPosition.x, currentPosition.y);
}
private double distanceBetweenObjects(Point characterLocation, Point projectile) {
return distance(characterLocation.x, characterLocation.y, projectile.x, projectile.y);
}
This code is now cut down drastically because we “extracted” the body of the function out of the two functions, put them into the body of a third function and then call that function from the two original places. This is one of my favorite refactors because it leads to a lot of elegance and flexibility later. If we were to add yet another function that needed to calculate distance as a step, we could simply call the distance formula.
One of the old coding mantras is “a function should do one thing and do it well”. This pattern is relatively easy to spot because we start getting tired of the function and looking for it to finally be complete. We have to keep trying to go through too many lines and too many variables and keep them in our head which is proving difficult. This is where you stop and ask yourself “How can I cut this down?”
Related to trick number 2 above, we look at the pieces of function and try to find out where to break it. Does this function try to find the distance traveled for our character AND set the marker on the map? Sounds like two different things to me. Breaking out the distance part and putting it into its own function simplifies the original function. It no longer has to worry about calculating the distance, it just delegates that responsibility to another function. It just uses the returned answer. Do it enough and you will start to develop controller functions or functions made up of nothing but other function calls. These types of functions are great to have in larger projects.
// It is doing two things, calculating distance and placing a marker.
// It probably should just update our marker and leave distance calculating to someone else.
private void updateMapMarker(OurMap map) {
// Calculating distance
double xPointTraveled = Math.Pow((currentPosition.x - origin.x), 2.0);
double yPointTraveled = Math.Pow((currentPosition.y - origin.y), 2.0);
double distanceTraveled = Math.Sqrt(xPointTraveled + yPointTraveled);
// Placing marker on map
map.origin = lastSaveLocation;
map.direction = Direction.SOUTH;
map.distance = distanceTraveled;
map.CreateMarker();
}
This function is doing two things to update the map. It is calculating the distance traveled in addition to actually manipulating our map. Since we spot it is doing two things, we can extract out the distance calculation and just use it in the updating of a map marker.
// Now it is just updating the map marker
private void updateMapMarker(OurMap map) {
// Placing marker on map
map.origin = lastSaveLocation;
map.direction = Direction.SOUTH;
// Delegate that distance calculation to another function.
// Like the one we created above.
map.distance = distance(origin, currentPosition);
map.CreateMarker();
}
So let’s say that we are creating a RPG. We notice that we are writing the same type of functionality over and over again in various unit classes like properties or getters / setters. The name of a knight works the same way as it does for a mage. This type of repetition is often telling you that you should probably take out those properties and functions and move them to a base class which then the two classes can inherit from.
// Two specific types of units, Knights and Mage
// Two different classes but both have name functionality.
public class Knight {
private String name;
public String Name {
get { return name; }
set { name = value; }
}
public void DoNobleStuff() {
// Do Noble things here
}
}
public class Mage {
private String name;
public String Name {
get { return name; }
set { name = value; }
}
public void CastSpell() {
// Do wickedly evil spell
}
}
Here in the example we have two classes that have their own functionality (DoNobleStuff and CastSpell). However, they share the same things like a name and the mechanics for getting / setting it. We can pull that up into its base class with this simple refactor trick.
// Two specific types of units, Knights and Mage
// Two different classes but both have name functionality.
public class Knight : Unit {
public void DoNobleStuff() {
// Do Noble things here
}
}
public class Mage : Unit {
public void CastSpell() {
// Do wickedly evil spell
}
}
// Base class to inherit name functionality from
public abstract class Unit {
protected String name;
public String Name {
get { return name; }
set { name = value; }
}
}
Often times I see newbies trying to string together a series of conditional statements into really long chains. They don’t often realize they are repeating themselves. When they say something like the following, they are really saying the same thing in two different ways…
if (number >= 1 && number <= 100 && number > 0 && number != -2) {
// Do stuff
}
If a number is greater than or equal to 1 and less than or equal to 100 we already know it is going to be greater than zero and not be set to -2. The last two conditions are useless. So the trick here is that we quickly identify long if statement conditionals, boil them down to what they are testing and get rid of the useless cases. Then if we are left with a long conditional still then perhaps we can wrap up some conditionals into a separate function with a better name.
// Convert something like the following…
if ((piece.location >= 1) && (piece.location <= 64) && ((piece.location % 2) == 0)) {
// Move legit
}
// Into something more readable...
if (onChessBoard(piece) && isOnWhite(piece)) {
// Move Legit
}
public bool onChessBoard(Piece p) {
if ((p.location >= 1) && (p.location <= 64)) { return true; }
return false;
}
public bool isOnWhite(Piece p) {
if ((p.location % 2) == 0) { return true; }
return false;
}
[/csharp]
In this situation we added more code than the original if statement, but we did two things really good here. We broke our complex if into a new simplified conditional which has increased readability. We now see that we are dealing with a chess board and checking if the piece is on a white square. The second thing we did is made two really simple functions that are short and sweet and easy to maintain. If in the future we find a bug related to a piece being on a white square or not, we know we can come to the isOnWhite() function to look for that bug.
This particular refactor is pretty easy to spot. If you are doing really long if statements you probably want to see if you are repeating a test you have already done earlier or see if you can break it apart into functions that have descriptive labels. This will certainly help make things more readable as well as maintainable in the future.
Conclusion
So that is the 5 quick and easy refactoring tips to help you cut down that unwieldy spaghetti code down to size. Each refactor is meant to make things simplified, readable and a bit more maintainable… the three great principles behind software design. I hope you found the information useful and something you can look for in other people’s code as well as your own. Thanks for reading!
