5 signals that can indicate its time to re-factor

Re-factoring code is a part of any programmers life or should be. Very few people write beautifully clean and well factored code from the start, and I am certainly not one of them. Most times my code starts out a sprawling tangle that takes an additional pass or two to look like something I would consider clean. I find that when programming and designing I start with a rough idea or sketch of what I want to accomplish. Through my process I refine and reshape it into something beautiful. Where programming and art differ is that when programming once it starts ‘working’ there is the temptation to stop tweaking it. If you do stop, then only the people looking at the code will know what a mess has been made. Whereas with art & design, if you try and pass off a rough unfinished piece as something complete and polished you will often be met with raised eyebrows. What follows is part of my personal hit list of things I look for when I clean my code. And while there are far more exhaustive and thorough lists of things to look for. I kept to five because it made for a better title than 7 or 8 signals that indicate its time to re-factor.

1. Methods with too many parameters

Methods that require lots of parameters require you to remember more and make for constant referencing to an API. Its also easier to make mistakes using these methods. A perfect example of this is Model::find() Compare the 1.1 method use vs. 1.2

6 parameters! While all necessary, remembering which goes where was difficult, and required memorization. Contrast that with the 1.2 usage

Only two parameters makes this method far easier to remember, and far easier to use. I say this method is easier to use as you will make less mistakes by getting the parameter order incorrect. Another thing I’ve grown to dislike is flag parameters. You know those little boolean parameters we tack onto method to make them do more than one thing. The problem with flag parameters is they open you up for methods that do more than one thing, and again they are harder to remember. How many times have you forgotten the little false on the end of Model::bindModel(). Now how often would you make the mistake between Model::bindModel() and Model::bindModelPermanently(). By dropping flag parameters and introducing an additional method you make a trade off though. You end up with more methods, but in exchange you get more readable code.

2. Methods that do too many things.

Often times methods that do many things are often the same as those that have lots of parameters. However, methods that do more than one thing are double trouble. First they are harder to test, and secondly they can be confusing to use. To figure out if a method is doing more than one thing I describe what the function does. If when describing my method, I use and, or or but, then there is a pretty good chance that what I have written should be two methods and not one. At that point, I normally try to split my method into more than one public method. I would also create a protected method if there is some common features shared between the two public methods.

This method both gets a Node object and updates the view count based on a flag parameter. The incrementing of the view count is not very clear from reading the name of the method. While this particular method is not overly difficult to test, it could be confusing to use, as the view count could accidentally be incremented.

There are now 2 separate methods that do one thing each. With this setup, it is easy to figure out what each method is going to do by simply reading the method name. We also benefit from simpler tests as we don’t have as many paths of execution to test. By making methods only do one thing, and giving them appropriate names our code is easier to read, easier to use, and easier to test.

3. Repetitive code blocks.

Repetitive code can happen when you are in a hurry, and just need a loop here, or that method there. Removing these duplicated blocks of code is really important as its easy to forget to update them all when you update one. Ideally when you find repeated code, you want to move it into its own method and make that method flexible enough to handle the variations required. For example:

Two identical loops. Now how easy would it be to forget to update one when you fix the other. We can more easily express this as

Not only did we use less code, we also reduced our ability to make mistakes. Removing duplicate code makes things easier to maintain and more homogenous. With fewer lines of code to maintain, and less places to make mistakes bugs are easier to fix.

4. Bad Names

As programmers we have to name many things, classes, methods, variables, and files all need names. Names that are descriptive and befitting of their purpose are the best. They help describe our code and give meaning to what we are trying to communicate. At its root programming is communication. You need to communicate ideas to both the machine and those who will read the code after you. Compare the following:

I personally find this code totally opaque, its very hard to discern what the author was trying to do with this code. In comparison the same code with good names makes the code far more accessible.

As a corollary, hungarian notation and single letter variables outside of loop variables should be avoided. Hungarian notation in dynamic languages just does not make sense. If you change the type $iFoo to null at some point. The value of the embedded type is lost as $iFoo is no longer accurate. What you are left with is a more confusing variable than if you had just given the variable a name that accurately describes its purpose in the first place. Lastly, knowing a type is useless if you don’t know what the variable is for. The only place I can see embedding a type into a variable name is when the language supports or requires it, like ruby.

5. Mixed levels of abstraction and muddled areas of concern

Mixed levels of abstraction can be difficult to spot at first. The idea behind the concept is that each object or layer should have one role, and should delegate to other objects the implementation details. By separating your concerns you encapsulate your logic better and create more reusable code. As an added bonus your code becomes easier to test, more focused, and shorter. For example in an MVC application the controller should be responsible for handling user requests, and providing the View layer with the necessary information. It should not be directly talking to the database in terms of SQL. Another example is having inline CSS and Javascript in your HTML. Doing so couples the two layers together creating inflexible and often brittle code.

Not only is this code terribly insecure, but it demonstrates a mixture of concerns and abstraction levels. The controller is sending SQL directly to the database. If your database structure ever changes you need to update objects that should not even be related to the database domain. A much better solution is to use a model method to encapsulate the read logic. So that each of our objects are only responsible for one role.

This approach keeps our controller and model concerns separate. We could improve our code further by using Model::read(), as that method better encapsulates and abstracts the database.

So there you go, those are some of the more common things I look for when re-factoring code. Obviously this is not an exhaustive and complete list. My goal in using these re-factoring methods is to create simpler code. Simpler code is easier to maintain, and easier to test. If code is easy to test, it is easy to fix when something goes wrong. I hope you can find these useful in your on going quest against stinky code.