Code Smells

The following list to identify a code smell:

• Rigidity: the software is difficult to change. A small change causes a cascade of subsequent changes.

• Fragility: the software breaks in many places due to a single change.

• Immobility: you cannot reuse parts of the code in other projects because of the involved risks and high effort.

• Needless Complexity.

• Needless Repetition: the two code fragments look almost identical.

• Opacity: the code is hard to understand.

Comments

Always try to explain yourself in code.

Avoid:

• Inappropriate information

• Obsolete comment

• Redundant comment

• Poorly Written comment

• Commented-out code

Functions

A function should be small and do one thing.

Avoid:

• Long function: A function contains too many lines of code. Generally, any function longer than ten lines should make you start asking questions.

• Too many arguments: More than three or four parameters for a function.

• Output arguments: Readers expect arguments to be inputs, not outputs. If your function must change the state of something, have it change the state of the object it is called on.

• Flag arguments: Boolean arguments loudly declare that the function does more than one thing.

• Selector Arguments: They can be boolean, 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. Selector arguments are just a lazy way to avoid splitting a large function into several smaller functions.

• Dead function: Methods that are never called should be discarded.

• Feature Envy: A method that accesses the data of another object more than its own data.

Classes

A class should be small and do one thing.

Avoid:

• Large Class: a class that contains many fields/methods/lines of code.

• Alternative classes with different interfaces: two classes perform identical functions but have different method names. The programmer who created one of the classes probably didn’t know that a functionally equivalent class already existed.

• Refused Bequest: If a subclass uses only some of the methods and properties inherited from its parents, the hierarchy is off-kilter. The unneeded methods may simply go unused or be redefined and give off exceptions.

• Change Preventers:

  • Having to change many unrelated methods when you make changes to a class.

  • Making any modifications requires that you make many small changes to many different classes.

  • Parallel inheritance hierarchies: Whenever you create a subclass for a class, you find yourself needing to create a subclass for another class.

• Speculative generality: code is created “just in case” to support anticipated future features that never get implemented.

• Dead Code: A variable, parameter, field, method, or class is no longer used (usually because it’s obsolete).

• Violate the Law of Delimiter: see a series of calls resembling a->b()->c()->d(). A chain occurs when a client requests another object, that object requests yet another one, and so on. These chains mean that the client is dependent on navigation along the class structure. Any changes in these relationships require modifying the client.

• Violate Encapsulation: a class exposes the internal structure.

• Inherit Constants: a class extends from a constant class. Should use a static import instead.

• Inappropriate use of constants: Enums can have methods and fields. This makes them very powerful tools that allow much more expression and flexibility than constant.

General

• Multiple Languages in One Source File: a source file should contain one, and only one language.

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

• Incorrect Behavior at the Boundaries: Don’t rely on your intuition. Look for every boundary condition and write a test for it.

• Duplication: Following "Don’t Repeat Yourself", or "Once, and only once". The duplication can be addressed by using the TEMPLATE METHOD or STRATEGY pattern.

• Code at Wrong Level of Abstraction: It is important to create abstractions that separate higher-level general concepts from lower-level detailed concepts. Sometimes we do this by creating abstract classes to hold the higher level concepts and derivatives to hold the lower level concepts. When we do this, 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. We don’t want lower and higher-level concepts mixed together.

• Base Classes Depending on Their Derivatives:

  • In general, base classes should know nothing about their derivatives.

  • Sometimes the number of derivatives is strictly fixed, and the base class has code that selects between the derivatives. In that case, the derivatives and base class are strongly coupled and always deploy together in the same jar file. In the general case, we want to be able to deploy derivatives and bases in different jar files.

  • Deploying derivatives and bases in different jar files and making sure the base jar files know nothing about the contents of the derivative jar files allow us to deploy our systems in discrete and independent components. When such components are modified, they can be redeployed without having to redeploy the base components. This means that the impact of a change is greatly lessened, and maintaining systems in the field is made much simpler.

• Too Much Information: Hide your data. Hide your utility functions. Hide your constants and your temporaries. Don’t create classes with lots of methods or lots of instance variables. Don’t create lots of protected variables and functions for your subclasses. Concentrate on keeping interfaces very tight and very small. Help keep coupling low by limiting information.

• Dead Code: Dead code is code that isn’t executed.

• Vertical Separation: Variables and functions should be defined close to where they are used.

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

• Clutter: Variables that aren’t used, functions that are never called, comments that add no information, and so forth.

• Artificial Coupling: an artificial coupling is a coupling between two modules that serve no direct purpose. It is a result of putting a variable, constant, or function in a temporarily convenient, though inappropriate, location. Should figure out where functions, constants, and variables ought to be declared.

• Misplaced Responsibility: where to put code.

• Inappropriate Static: you should prefer nonstatic methods to static methods. When in doubt, make the function nonstatic. If you really want a function to be static, make sure that there is no chance that you’ll want it to behave polymorphically.

• Use Explanatory Variables: break the calculations up into intermediate values that are held in variables with meaningful names.

• Function Names Should Say What They Do

• 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.

• 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

• Follow Standard Conventions

• Replace Magic Numbers with Named Constants

• 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. Don’t be lazy about the precision of your decisions.

• Structure over Convention: Enforce design decisions with structure over convention. Naming conventions are good, but they are inferior to structures that force compliance. For example, switch/cases with nicely named enumerations are inferior to base classes with abstract methods.

• Encapsulate Conditionals: Boolean logic is hard enough to understand without having to see it in the context of an if or while statement. Extract functions that explain the intent of the conditional.

• Avoid Negative Conditionals

• Functions Should Do 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.

• 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. Expose it as an argument to that low-level function called from the high-level function.

• Avoid Transitive Navigation: we don’t want a single module to know much about its collaborators. This is sometimes called the Law of Demeter.

Names

• Choose Descriptive Names.

• Choose Names at the Appropriate Level of Abstraction: Don’t pick names that communicate implementation; choose names that 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 conventions or usage. For example, if you are using the DECORATOR pattern, you should use the word Decorator in the names of the decorating classes.

• Unambiguous Names: Choose names that make the workings of a function or variable unambiguous. Avoiding the name of the function does not say what the function does.

• Use Long Names for Long Scopes: Use Long Names for Long Scopes.

• Avoid Encodings

• 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. Don’t use a simple verb to describe a function that does more than just that simple action.

Tests

• Avoid 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: 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.

• An Ignored Test Is a Question about an Ambiguity: Sometimes we are uncertain about a behavioral detail because the requirements are unclear. We can express our question about the requirements as a test that is commented out, or as a test that is annotated with @Ignore.

• 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. You’ll probably find that the bug was not alone.

• 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, and expose patterns.

• Test Coverage 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