Graduate Program KB

Smells and Heuristics

Table of Contents

Comments

C1: Inappropriate Information

  • Comments should be reserved for technical notes about the code and design.
  • Anything else is considered inappropriate (change history for example.)

C2: Obsolete Comment

  • Old, irrelevant, or incorrect comments.
  • It's best to update or get rid of obsolete comments asap.
  • They tend to migrate away from the code they describe as the code evolves.

C3: Redundant Comment

  • A comment is redundant if it describes something that adequately describes itself.

C4: Poorly Written Comment

  • A comment worth writing should be written with care and it should be brief and concise.

C5: Commented-Out Code

  • Whenever you see commented out code delete it!
  • There is no need for it, it just rots.
  • If someone really needs that code, they can recover it from source control.

Environment

E1: Build Requires More Than One Step

  • Building a project should be a trivial single command.
  • It should bot be a large plethora of commands and linking files together.

E2: Tests Require More Than One Step

  • Similar to [E1] we want to be able to run all tests easily with either a click of a button or running a command.
  • This makes having all the code under test easy to do.

Functions

F1: Too Many Arguments

  • A function should not have many arguments, having none is best.
  • Any more than 3 is a big smell.

F2: Output Arguments

  • If your function must change the state of something, have it change the state of the object it's called on.

F3: Flag Arguments

  • Boolean arguments hint that the function is doing more than one thing and should be eliminated.

F4: Dead Functions

  • Methods that aren't called should be deleted.
  • If you want it back later, get it from source control.

General

G1: Multiple Languages in One Source File

  • This is sloppy and confusing.
  • It is ideal for a source file to contain one language.
  • Realistically we will have more than one (say the programming language and english), but we should take pains to minimise both the number and extent of extra languages in source files.

G2: Obvious Behaviour is Unimplemented

  • If we write functions that ignore obvious intuitive features, users will lose trust in your code.
  • For example: You write a function that takes a day of the week and converts it to an enum, you would expect:
    • Casing to not matter.
    • Common abbreviations to be translated.

G3: Incorrect Behaviour at the Boundaries

  • Don't rely on intuition!
  • Find every boundary case (edge case) and write a test for it.

G4: Overridden Safeties

  • Overriding safeties may sometimes be necessary but it should always be done with caution.
  • Don't ignore warnings or errors or tests saying you will fix them later, this is a bad habit to get into.

G5: Duplication

  • DRY principle, Don't Repeat Yourself.
  • Every time you see repeated code, it represents missed opportunity for abstraction.
  • Raising the abstraction level increases the vocabulary of the language of your design, other programmers can use these abstract features.

G6: Code at Wrong Level of Abstraction

  • The base class should only consist of the higher levels of abstraction.
  • We should separate concepts at different levels and place them in different containers.
  • Don't want lower and higher levels mixed together.
  • Very difficult to do and you can't hide from it.

G7: Base Classes Depending on Their Derivatives

  • When we see base classes mentioning the names of their derivatives, we suspect an issue.
  • Base classes should know nothing about their derivatives.
  • We want to deploy derivatives and basses in different files and make sure that the base files know nothing about the contents of the derivative files.
  • This allows us to deploy our systems in discrete and independent components.
  • When these components are modified, they can be redeployed without having to redeploy the base components.
  • This means the impact of the change is greatly lessened.

G8: Too Much Information

  • Good modules have small interfaces that allow you to do a lot with a little.
  • Bad modules have large interfaces that force you to use many different gestures to get a simple task done.
  • A well-defined interface is achieved through loose coupling.
  • Hide your data, utility functions, constants, and your temporaries.

G9: Dead Code

  • Code that isn't executed.
  • When found delete it.
  • Common places are in conditions that never happen.

G10: Vertical Separation

  • Variables and functions should be defined close to where they are used.
  • Local variables should be declared just above their first usage.
  • Private functions should be declared just below their first usage.
  • We are essentially trying to minimise vertical distance to make it easier to read and understand.

G11: Inconsistency

  • If you do something a certain way... all good. But be consistent.
  • Principle of least surprise.

G12: Clutter

  • Get rid of stuff that isn't needed:
    • Default constructor with no implementation.
    • Unused variables.
    • Functions that are never called.
    • Comments that add no useful information.
  • Keep our source files clean.

G13: Artificial Coupling

  • Things that don't depend upon each other should not be artificially coupled.
  • Artificial coupling is a coupling between two modules that serves no direct purpose.
  • An example is constraining an enum to a specific class, now whenever something else needs the enum, it needs that class.

G14: Feature Envy

  • The methods of a class should be interested in the variables and functions of the class they belong to, and no the variables and functions of other classes.
  • We don't want methods in our class to envy methods of another.
  • This is no good because it exposes the internals of one class to another.

G15: Selector Arguments

  • It is better to have many functions than to pass some code into a function to select the behaviour.

G16: Obscured Intent

  • We want expressive code.
  • It is worth taking the time to make the intent of our code visible to our readers.

G17: Misplaced Responsibility

  • Code should be placed where a reader would naturally expect it to be.
  • If you try to be too clever with where you place your code it can make it hard to read.

G18: Inappropriate Static

  • In general you should prefer non-static methods to static methods.
  • If in doubt use non-static methods.
  • Use static methods if you want it to behave polymorphically.

G19: Use Explanatory Variables

  • Break calculations up into intermediate values that are held in variables with meaningful names.

G20: Function Names Should Say What They Do

  • The name of the function should clearly tell the reader what the function does.

G21: Understand the Algorithm

  • Take the time to understand the algorithm, don't just poke around it until the tests work.
  • You need to know that the solution is correct.

G22: Make Logical Dependencies Physical

  • If a module depends on another, it should be physical not just logical.
  • The dependent modules should explicitly ask the module it depends on for all the information that it depends on.

G23: Prefer Polymorphism to If/Else or Switch/Case

  • The heuristic here is to consider polymorphism before using a switch.
  • Cases where functions are more volatile than types are relatively rare, so every switch statement should be suspect.
  • ONE SWITCH rule: There may be no more than one switch statement for a given type of selection. The cases in that switch statement must create polymorphic objects that take the place of other such switch statements in the rest of the system

G24: Follow Standard Conventions

  • Every team should follow a coding standard based on common industry norms.
  • The standard should specify:
    • Where to declare instance variables.
    • How to name classes, methods and variables.
    • Where to put braces.
    • etc.
  • Not needed as a document as previous code should serve as the standard.

G25: Replace Magic Numbers with Named Constants

  • Using raw numbers in your code is (most of the time) bad practice.
  • Hide it behind a variable with a meaningful name.

G26: Be Precise

  • Make precise decisions in your code.
  • Know why you made that decision and how you will deal with exceptions.
  • Ambiguities and imprecision in code are either a result of disagreements or laziness... they should be eliminated.

G27: Structure Over Convention

  • Enforce design decisions with structure over convention.
  • This is commonly done through classes with abstract methods.

G28: Encapsulate Conditionals

  • Extract conditionals (predicates) into functions that describe the intent of the condition.

G29: Avoid Negative Conditionals

  • Negatives are just a bit harder to understand the positives.
  • Due to this we rather not having to negate things where possible or have "not" in the name.

G30: Functions Should Do One Thing

  • Follow the Single Responsibility Principle.

G31: Hidden Temporal Couplings

  • Temporal couplings are often necessary, but you shouldn't hide the couplings.
  • Structure the arguments of your functions such that the order in which they should be called is obvious.
  • We can do this by exposing the coupling through arguments.

G32: Don't Be Arbitrary

  • Have a reason for the way you structure your code, and make sure that the reason is communicated by the structure of the code.
  • If a structure feels arbitrary, others will be inclined to change it.
  • Public classes that are not utilities of some other class should not be scoped inside another class.
  • The convention is to make them public at the top level of their package.

G33: Encapsulate Boundary Conditions

  • Put the processing of boundary conditions in one place.
  • Encapsulate boundary conditions in variables with useful names.

G34: Functions Should Descend Only One Level of Abstraction

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

G35: 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 attraction, don't bury at lower levels.
  • You can expose it as an argument passed to those functions if it is needed.

G36: Avoid Transitive Navigation

  • In general we don't want a single module to know much about its collaborators.
  • Example: If A -> B, B -> C. We don't want modules that use A to know about C.
  • The Law of Demeter.
  • It comes down to making sure modules only know about their immediate collaborators.

Java

J1: Avoid long Import Lists by Using Wildcards

  • Using a wildcard means that the package has to exist, doing it explicitly doesn't mean the package has to exist.
  • You're not actually importing the entire library like you might think. It only imports what you need.
  • Wildcards just avoid the need for multiple import statements.

J2: Don't Inherit Constants

  • Don't use inheritance as a way to cheat the scoping rules of the language.
  • Use static imports instead.

J3: Constants versus Enums

  • Use enums!
  • Enums can have methods and fields which makes them very powerful tools that allow much more expression and flexibility than other types.

Names

N1: Choose Descriptive Names

  • Be clear with your intent in the name.
  • Don't worry about it being long.
  • No need to rush, this is important.

N2: Choose Names at the Appropriate Level of Abstraction

  • Don't pick names that communicate implementation.
  • We need to be careful and select appropriate names depending on the level of abstraction we are at.

N3: Use Standard Nomenclature Where Possible

  • Names are easier to understand if they are based on existing convention or usage.
  • The more you can use standards that your work environment implement, the easier the code will be to understand.

N4: Unambiguous Names

  • Choose names that make the workings of a function or variable unambiguous.
  • This can be done with useful prefixes to specify what the function is doing, like 'handle' for example.

N5: Use Long names for Long Scopes

  • Length of the name should be related to its scope.
  • For example if your name is used only in a for loop i and j may be acceptable names.

N6: Avoid Encodings

  • Names should not be encoded with type or scope information.
  • Keep your names free of hungarian pollution.

N7: Names Should Describe Side-Effects

  • This comes back to naming the function carefully to describe what this function does.
  • Don't hide side-effects.

Tests

The names speak for themselves here:

  • T1: Insufficient Tests.
  • T2: Use a Coverage Tool!
  • T3: Don't Skip Trivial Tests.
  • T4: An Ignored Test Is a Question about an Ambiguity.
  • T5: Test Boundary Conditions.
  • T6: Exhaustively Test Near Bugs.
  • T7: Patterns of Failure are Revealing.
  • T8: Test Coverage Patterns are Revealing.
  • T9: Tests Should Be First.

Conclusion

  • This is not a complete list, such a thing probably doesn't exist.
  • Use this as a guide and a cheat sheet with your work.