Smells and Heuristics

Comments

Innapropiate Information

It is inappropriate for a comment to hold information better held in a different kind of system such as your source code control system, your issue tracking system, or any other record-keeping system. Comments should be reserved for technical notes about the code and design.

Obsolete Comment

A comment that has gotten old, irrelevant, and incorrect is obsolete. If you find an obsolete comment, it is best to update it or get rid of it as quickly as possible.

Redundant Comment

A comment is redundant if it describes something that adequately describes itself. Comments should say things that the code cannot say for itself.

Poorly Written Comment

A comment worth writing is worth writing well. Be brief.

Commented-Out Code

When you see commented-out code, delete it! Don’t worry, the source code control system still remembers it.

Environment

Build Requires More Than One Step

You should not need a sequence of arcane commands or context dependent scripts in order to build the individual elements. You should be able to check out the system with one simple command and then issue one other simple command to build it.

Tests Require More Than One Step

You should be able to run all the unit tests with just one command.

Functions

Too Many Arguments

Functions should have a small number of arguments.

Output Arguments

Output arguments are counterintuitive. Readers expect arguments to be inputs, not outputs.

Flag Arguments

Boolean arguments loudly declare that the function does more than one thing. They are confusing and should be eliminated.

Dead Function

Methods that are never called should be discarded, your source code control system still remember it.

General

Multiple Languages in One Source File

The ideal is for a source file to contain one, and only one, language. Realistically, we will probably have to use more than one. But we should take pains to minimize both the number and extent of extra languages in our source files.

Obvious Behaviour Is Unimplemented

Following “The Principle of Least Surprise”, any function or class should implement the behaviors that another programmer could reasonably expect.

Incorrect Behaviour at the Boundaries

It seems obvious to say that code should behave correctly. The problem is that we seldom realize just how complicated correct behavior is. Don’t rely on your intuition. Look for every boundary condition and write a test for it.

Overriden Safeties

It is risky to override safeties. Turning off failing tests and telling yourself you’ll get them to pass later is bad.

Duplication

Every time you see duplication in the code, it represents a missed opportunity for abstraction. Abide by the DRY principle (Don’t Repeat Yourself).

Code at Wrong Level of Abstraction

It is important to create abstractions that separate higher level general concepts from lower level detailed concepts. We need to make sure that the separation is complete. We want all the lower level concepts to be in the derivatives and all the higher level concepts to be in the base class.

Good software design requires that we separate concepts at different levels and place them in different containers. We don’t want lower and higher level concepts mixed together.

Base Classes Depending on Their Derivatives

The most common reason for partitioning concepts into base and derivative classes is so that the higher level base class concepts can be independent of the lower level derivative class concepts. Therefore, when we see base classes mentioning the names of their derivatives, we suspect a problem. In general, base classes should know nothing about their derivatives.

Too Much Information

Well-defined modules have very small interfaces that allow you to do a lot with a little.

Good software developers learn to limit what they expose at the interfaces of their classes and modules. The fewer methods a class has, the better. The fewer variables a function knows about, the better. The fewer instance variables a class has, the better.

Dead Code

Dead code is code that isn’t executed. Dead code is not completely updated when designs change. It still compiles, but it does not follow newer conventions or rules. Delete it.

Vertical Separation

Variables and function should be defined close to where they are used. Local variables should be declared just above their first usage and should have a small vertical scope. Private functions should be defined just below their first usage.

Inconsistency

If you do something a certain way, do all similar things in the same way.

Artificial Coupling

Things that don’t depend upon each other should not be artificially coupled. In general an artificial coupling is a coupling between two modules that serves no direct purpose. It is a result of putting a variable, constant, or function in a temporarily convenient, though inappropriate, location. This is lazy and careless.

Feature Envy

When a method uses accessors and mutators of some other object to manipulate the data within that object, then it envies the scope of the class of that other object. It wishes that it were inside that other class so that it could have direct access to the variables it is manipulating.

Selector Arguments

Not only is the purpose of a selector argument difficult to remember, each selector argument combines many functions into one. Selector arguments are just a lazy way to avoid splitting a large function into several smaller functions.

Of course, selectors need not be boolean. They can be enums, integers, or any other type of argument that is used to select the behavior of the function. In general it is better to have many functions than to pass some code into a function to select the behavior.

Obscured Intent

We want code to be as expressive as possible. Run-on expressions, Hungarian notation, and magic numbers all obscure the author’s intent. It is worth taking the time to make the intent of our code visible to our readers.

Misplaced Responsibility

One of the most important decisions a software developer can make is where to put code. The principle of least surprise comes into play here. Code should be placed where a reader would naturally expect it to be.

Use Explanatory Variables

One of the more powerful ways to make a program readable is to break the calculations up into intermediate values that are held in variables with meaningful names.

Function Names Should What They Do

If you have to look at the implementation (or documentation) of the function to know what it does, then you should work to find a better name or rearrange the functionality so that it can be placed in functions with better names.

Understand the Algorithm

Lots of very funny code is written because people don’t take the time to understand the algorithm. Before you consider yourself to be done with a function, make sure you understand how it works. It is not good enough that it passes all the tests. You must know that the solution is correct. Often the best way to gain this knowledge and understanding is to refactor the function into something that is so clean and expressive that it is obvious how it works.

Make Logical Dependencies Physical

If one module depends upon another, that dependency should be physical, not just logical. The dependent module should not make assumptions (in other words, logical dependencies) about the module it depends upon. Rather it should explicitly ask that module for all the information it depends upon.

Prefer Polymorphism to If/Else or Switch/Case

Most people use switch statements because it’s the obvious brute force solution, not because it’s the right solution for the situation. So this heuristic is here to remind us to consider polymorphism before using a switch. Also the cases where functions are more volatile than types are relatively rare. So every switch statement should be suspect.

Follow Standard Conventions

Every team should follow a coding standard based on common industry norms. This coding standard should specify things like where to declare instance variables; how to name classes, methods, and variables; where to put braces; and so on.

Replace Magic Numbers with Named Constants

In general it is a bad idea to have raw numbers in your code. You should hide them behind well-named constants. The term “Magic Number” does not apply only to numbers. It applies to any token that has a value that is not self-describing.

Be Precise

When you make a decision in your code, make sure you make it precisely. Know why you have made it and how you will deal with any exceptions. Ambiguities and imprecision in code are either a result of disagreements or laziness. In either case they should be eliminated.

Structure over Convention

Enforce design decisions with structure over convention.

Encapsulate Conditionals

Extract functions that explain the intent of the conditional.

Avoid Negative Conditionals

Negatives are just a bit harder to understand than positives. So, when possible, conditionals should be expressed as positives.

Functions Should Do One Thing

Functions that do more than one thing should be converted into many smaller functions, each of which does one thing.

Hidden Temporal Couplings

Temporal couplings are often necessary, but you should not hide the coupling. Structure the arguments of your functions such that the order in which they should be called is obvious.

Don’t Be Arbitrary

Have a reason for the way you structure your code, and make sure that reason is communicated by the structure of the code.

Encapsulate Boundary Conditions

Boundary conditions are hard to keep track of. Put the processing for them in one place. Don’t let them leak all over the code.

Functions Should Descend Only One Level of Abstraction

The statements within a function should all be written at the same level of abstraction, which should be one level below the operation described by the name of the function.

Keep Configurable Data at High Levels

If you have a constant such as a default or configuration value that is known and expected at a high level of abstraction, do not bury it in a low-level function.

Avoid Transitive Navigation

In general we don’t want a single module to know much about its collaborators. More specifically, if A collaborates with B, and B collaborates with C, we don’t want modules that use A to know about C. (For example, we don’t want a.getB().getC().doSomething();). Rather we want our immediate collaborators to offer all the services we need.

This is sometimes called the Law of Demeter. The Pragmatic Programmers call it “Writing Shy Code.”

Names

Choose Descriptive Names

Names in software are 90 percent of what make software readable. You need to take the time to choose them wisely and keep them relevant. Names are too important to treat carelessly. The power of carefully chosen names is that they overload the structure of the code with description.

Choose Names at the Appropriate Level of Abstraction

Don’t pick names that communicate implementation; choose names the reflect the level of abstraction of the class or function you are working in.

Use Standard Nomenclature Where Possible

Names are easier to understand if they are based on existing convention or usage.

Unambiguous Names

Choose names that make the workings of a function or variable unambiguous.

Use Long Names for Long Scopes

The length of a name should be related to the length of the scope. You can use very short variable names for tiny scopes, but for big scopes you should use longer names. Variables and functions with short names lose their meaning over long distances. So the longer the scope of the name, the longer and more precise the name should be.

Avoid Encodings

Names should not be encoded with type or scope information. Prefixes such as m_ or f are useless in today’s environments.

Names Should Describe Side-Effects

Names should describe everything that a function, variable, or class is or does. Don’t hide side effects with a name.

Tests

Insufficient Tests

A test suite should test everything that could possibly break. The tests are insufficient so long as there are conditions that have not been explored by the tests or calculations that have not been validated.

Use A Coverage Tool

Coverage tools reports gaps in your testing strategy. They make it easy to find modules, classes, and functions that are insufficiently tested.

Don’t Skip Trivial Tests

They are easy to write and their documentary value is higher than the cost to produce them.

Test Boundary Conditions

Take special care to test boundary conditions. We often get the middle of an algorithm right but misjudge the boundaries.

Exhaustively Test Near Bugs

Bugs tend to congregate. When you find a bug in a function, it is wise to do an exhaustive test of that function.

Patterns of Failure Are Revealing

Sometimes you can diagnose a problem by finding patterns in the way the test cases fail. This is another argument for making the test cases as complete as possible. Complete test cases, ordered in a reasonable way, expose patterns.

Test Covertage Patterns Can Be Revealing

Looking at the code that is or is not executed by the passing tests gives clues to why the failing tests fail.

Tests Should Be Fast

A slow test is a test that won’t get run.