Clean Coding Practices
Language Agnostic
As the software engineering industry grew and evolved, some common best practices also grew. These practices became known as “Clean Coding Practices”. They are not specific to any particular language, as they bear true across all programming languages. Clean coding practices will make you a better engineer, make the codebase more stable, and make your company more money. Observing these practices will allow you to spend your time building cool new features, and not maintaining fragile code.
“Any fool can write code that a computer can understand. Good programmers write code that humans can understand.”
— Martin Fowler
Naming
“There are only two hard things in Computer Science: cache invalidation and naming things.”
— Phil Karlton
Naming things is quite difficult. A good name should reveal the intention of the object. It should answer questions like what it does, how to use it, and why it’s here.
Bad:
var date1; var date2; private makeExDate() { … }
Good:
var tokenExpirationDate; var tokenRefreshDate; private updateTokenDate() { … }
Be consistent in your naming conventions. Define standards for each language in your tech stack with naming conventions. Always follow these standards to produce code that undisguisable from the person before or after you. This will provide consistency across the application and make it easier to support in the future.
Be consistent in your terminology. Do not use the thesaurus to come up with new words because you are tired of typing `retrieve`. Changing the terminology will only confuse those who come after you, and make searching the codebase more difficult. It is simple, your future self will thank you.
When picking a name avoid disinformation or otherwise confusing names. Names should not leave deceptive hints. For example, an object named `UserList`, when it is not a list but rather an array, is disinformation. Our technical knowledge of a type of `list` makes anything other than a `list` data structure disinformation.
It is important to make meaningful distinctions when picking a name. An example of poor naming is `a1` and `a2` for parameter names instead of creating meaningful names like `user` and `account`.
Use pronounceable names. Write the name as we would speak it.
As humans, our brains have evolved to deal with the spoken language and therefore pronounceable names are just aesthetically nice and easier to talk about with colleagues.
— Clean Code by Uncle Bob
Bad:
var tltAmt;Good:
var totalAmount;Create names that can be easily searchable. Having names that follow a pattern gives fellow engineers some intuition about where and how to find code. It will also prevent find and replace errors.
If all your NgRx stores call the reducer `reducer` then it can get complicated picking the one you need. However, if you name it `userReducer` then we can easily pick the correct class from intellisense. Following the pattern of [store-name]Reducer makes it easier to:
- name the class
- find the class when searching
- makes intellisense easier to use, because you don’t have to comb through 8 “reducer” options.
There are language-specific expectations to this rule. For example, in JavaScript, when writing a small callback function, a throw-away variable is expectable. This is because the name lives and dies within 1–3 lines.
// This is OK because the variable u is easily understandable as a // user in the collection of users. const thisUser = users.find(u => u.userId === selectedUser.userId);
When designing an application terminology should be determined and documented before development begins. This should be done by a combination of the stakeholders, architecture, and engineering. It is important that everyone is speaking the same language from the beginning so data structures can be created properly. We don’t want to design the database table called `Users` and then 6 months into the project the stakeholders start calling users `profiles`. Now, all UI code refers to users as profiles, but the API and database refer to them as users. Changing the language mid-project will cause the person maintaining the code to have “domain” knowledge that profile == user in the backend. This is called mental mapping and it should be avoided. Keep the same name for an object from the UI to the Database to avoid confusion down the line.
Class names should be a noun. Method and function names should be a verb. No exceptions.
Don’t be cute, funny, or punny. Do not name a function `SeekAndDestroy`. Make a clear concise name like `DeleteRecords`.
Use solution domain names. We can assume that engineers have computer science knowledge. Thus, we can use computer science language, algorithm names, pattern names, math terms, etc.
Use problem domain names. Code that has more to do with problem domain constructs should have names drawn from the problem domain. Worst case scenario, the engineer will have to search the problem domain for an answer.
Sometimes, a word can have multiple meanings. Adding meaningful context can help provide clarity. Imagine a variable out of context. If you see state next to zip code, you have a different idea than if you see state by itself.
Do not add superfluous context. Adding prefixes or using redundant conventions are both nasty and excessive. Shorter names are better if they are clear.
Single Responsibility
Encapsulate your code. Group all functions and methods about a domain in the same class or module (depending on the language).
KISS (Keep it simple stupid). Keep functions as short as possible. I have seen methods that are 3 lines long and ones that are over 100 lines long. Obviously, the 3-line method is easier to read. Giving guidance of “methods and functions should never be more than 15 lines” is unreasonable. Instead, we should strive to write methods and functions that have a single responsibility. A method that validates the user credentials, then creates a token, and sets a cookie on the browser should delegate most of the work to other functions.
Bad:
function loginUser(user) {
// All user validation logic
…
var isValidUser = (from u in ctx.users
where u.userId == user.userId
select u).Any();
if(isValidUser) {
// A lot of logic to create a token
… var token = new JwtSecurityToken( …);
…
// A lot of logic to create a cookie
var cookie = new Cookie(); return token;
} return;
}
Good:
public User loginUser(User user) {
var token;
if(validateUser(user)) {
token = createToken(user);
var cookie = createCookie(token);
browser.cookies.set(cookie);
} // 1 return statement limits the ways this function can exit
// this reduces cyclical complexity & makes the function easier
// to read
return token;
}private Boolean validateUser(User user) {
// All user validation logic
var thisUser = from u in ctx.users
where u.userId == user.userId
select u;
return thisUser.Any();
}private Token createToken(User user) {
// A lot of logic to create a token
… var token = new JwtSecurityToken( …);
… return token;
}private Cookie createCookie(Token token) {
// A lot of logic to create a cookie
var cookie = new Cookie();
… return cookie;
}
The statements in our function should be within one level of abstraction. As with the code above, we don’t want to send the reader of the code into a never-ending rabbit hole of:
“This method calls this function which calls another function which calls another function, which…. I am so lost. I don’t even remember where I started or what problem I was trying to solve.”
We could have easily asked the token function to create a cookie since the cookie method needs the token anyway. However, that is not the responsibility of the token function. It cannot assume every time a token is created, that a cookie should also be created.
The goal of any function or method doing “one thing” is to have no consequences. It does not mutate anything unexpectedly. There are no tricks. It takes in parameters and emits a new value or object. It never mutates its parameters. Mutating parameters could have unintended consequences and cause race conditions since the calling function cannot see the internals. For instance:
Bad:
public Token loginUser(User user) {
var token;
if(validateUser(user)) {
token = createToken(user);
} return token;
}function validateUser(user) {
// All user validation logic
var user = from u in ctx.users
where u.userId == user.userId select u;
return user.Any();
}function createToken(user) {
// A lot of logic to create a token
… var token = new JwtSecurityToken( …);
…
createCookie(token); return token;
}function createCookie(token) {
// A lot of logic to create a cookie
var cookie = new Cookie();
…
browser.cookies.set(cookie);
}
In the above code example, the token function called the cookie function, which created a cookie and set it on the document. We didn’t want to use cookies. Having a cookie set could mess up the UI reading invalid things. As an engineer debugging, the application it would take me longer reading the different levels of abstraction (and maybe some debugging time walking the tree) to find out the token function is calling the cookie function. I don’t want that. Now, I have to write a new function that creates the token but does not set the cookie. What a mess! What do I even call the function? `createToken` was already taken and renaming that function will cause a lot of refactoring work. If I remove the cookie call, I could break other things that are depending on it to set the cookie. Also, why did the `validateUser` function change the user parameter? What if there were things on that user I needed that are not on the database version of the object? It is not apparent from reading the `loginUser` function that the user object has been mutated.
Comments
It is possible to make the point that programmers should be disciplined enough to keep the comments in a high state of repair, relevance, and accuracy. I agree, they should. But I would rather that energy go toward making the code so clear and expressive that it does not need the comments in the first place.
— Clean Code
If you went to college for any kind of computer science degree, you may have been told to use comments to document your code. Documenting functions and methods with documentation tools like JSDoc lets the consumer of that function know the:
- parameters and their purpose
- the expected return value and its type
- any other relevant comments describing the purpose of the method and how to use it. These should be short and to the point. We do not need a history lesson here.
However, we should always strive to write self-documenting code. Comments can quickly become outdated. Outdated comments can be confusing to others that come along after you. The ideal state is for code to be self-evident. Meaning:
- Its parameters have clear meaningful names.
- A return type has been specified so the caller knows what to expect.
- The name of the function or method is clear, meaningful, and searchable.
- It only performs 1 task and never mutates its parameters.
Since these are already clean coding practices, simply following these habits will create self-documenting code.
Do not comment on bad code. Adding comments to explain code means it’s time to refactor and create new methods.
Bad:
// check if they entered more than 40 hours
if(mondayMinutes + tuesdayMinutes + wednesdayMinutes + thursdayMinutes + fridayMinutes / 60 > 40 || mondayMinutes + tuesdayMinutes + wednesdayMinutes + thursdayMinutes + fridayMinutes * 60 < 0)Good:
if(getTotalHours() > 40 || getTotalHours() < 0)Better:
if(validateWorkWeekHours())Do not leave in comment out code. The logic here is usually:
“We might need this block in the future.”
Or
“We are leaving this for historical purposes.”
Use source control so you can see the state of a file at a certain point in time. There is no reason to clutter your code base with old code. This will bloat the size of the application slowing performance. More importantly, it makes the code harder to read which will slow your velocity.
Error Handling
For code to truly be clean, the error handling must not camouflage the logic. How can we write clean, clear code and still handle errors?
Write your try-catch-finally statement first. This will keep all happy path logic together away from the error handling, and error handling logic will all be done in 1 place.
Give context with exceptions. Thrown exceptions give you the ability to establish the source and location of an error so that you can define the intent of the failed operation.
Define exception classes in terms of a caller’s needs. While we can categorize exceptions in a myriad of ways, like source or type, the critical distinction is in how the error is caught. This will provide the most context for debugging the error.
Do not return null. If you never return null, you will never have to check for null. Simple. (Caveat: JavaScript will always need to do null checking, because JavaScript…)
Testing
Tests are important and are not exempt from clean coding practices. Follow the same naming standards and other clean coding practices in your tests that you follow for your code. This will keep tests running fast and make them easy to maintain. Following the FIRST principle when it comes to tests will keep them clean:
- Fast: Tests should run quickly, otherwise people are less likely to run them and the code will begin to rot.
- Independent: Tests do not depend on each other.
- Repeatable: They can run in any environment.
- Self-Validating: They either pass or fail. You do not need to interpret the results. This requires 1 assertion per test.
- Timely: The tests ought to be written just before the production code. Writing it before is key. If you write it after, the production code may be hard to test.
Design Rules
The most important single aspect of software development is to be clear about what you are trying to build.
— Bjarne Strostrup
Proper design before you begin a project is key. Having a common understanding with the stakeholders about what you are building will prevent you from building the wrong thing. Having a mutual understanding with Architecture will:
- keep the teams moving
- ease the back and forth between engineering and architecture
- prevent refactoring and frustration at PR time
If you are in the middle of the sprint and having team discussions about how to code something, you are too late in the process. This will slow a team’s velocity by taking up precious sprint development time with hour-long meetings and conversations you should have had during design time.
Without requirements or design, programming is the art of adding bugs to an empty text file.
— Louis Srygley
Creating a proper design is one of the most important steps in the process. Engineers creating a design should consult with Architecture and get approval before implementing the design. Architects should be given proper time to design the overall system. Engineers can help design smaller features in the project.
Architects pick the design patterns that the team will use. Engineers learn the patterns, if necessary, and develop code that adheres to the design patterns chosen. Remember, architects are planning the forest, engineers are planting the trees.
Some other things to keep in mind:
- Follow the Law of Demeter: A class should know only its direct dependencies.
- Use dependency injection (DIP) to avoid tight coupling.
- If you use third-party libraries, wrap them, so if they change, only your wrapper implementation needs to change.
Other
Organizing the code
Declare variables as close to their usage as possible. Instance variables should be declared at the top of the class. All variables, methods, and functions should be listed in order of accessibility with public members at the top and private members at the bottom. Constants should be declared at the top of the class or in their own file.
DRY: Don’t Repeat Yourself. Before creating a new method/function search the codebase and make sure it does not already exist elsewhere.
Rule of 3: Don’t create a “shared” version of a piece of code until it has been used 2 places before. The rule of 3 means on the 3rd time writing the code, it is clear that the code is shared and can be refactored into a shared location. If you know before you write it that the code will be shared, you can place it into a shared location the 1st time.
Don’t hardcode. Define a constant or use variables instead of hardcoding the values. Using the variable will not only make it readable but will also make it easy to change if it is being used at multiple places.
Bad:
public getUsers() {
try { ... }
catch {
this.notifyUserOfError("There was an error. Please try again.")
}
}
public getOrders() {
try { ... }
catch {
this.notifyUserOfError("There was an error. Please try again.")
}
}public getProducts() {
try { ... }
catch {
this.notifyUserOfError("There was an error. Please try again.")
}
}private notifyUserOfError(message) {
this.messageService(message);
}
Consider the code above. We have repeated the same error message in 3 methods. If the stakeholder comes back and tells us to modify the message to read:
“An error was encountered. If you continue to receive this message, please reach out to support@myapp.com.”
Now we have to modify the message in 3 places. This is called a “magic string”, and it should be avoided. Let’s see how a constant can make this code easier to maintain.
export const genericErrorMessage = "There was an error. Please try again.";public getUsers() {
try { … }
catch {
this.notifyUserOfError(genericErrorMessage)
}
}public getOrders() {
try { … }
catch {
this.notifyUserOfError(genericErrorMessage)
}
}public getProducts() {
try { … }
catch {
this.notifyUserOfError(genericErrorMessage)
}
}private notifyUserOfError(message) {
this.messageService(message)
}
Now we have 1 place to modify the text of the message. We could have moved the magic string to the `notifyUserOfError` method, but then we would lose the ability to define a custom error message.
Always be a good boy scout. Leave the codebase cleaner than you found it. These practices were developed over time after decades of writing code. It is important to remember that our industry is not that old, comparatively. While lawyers and accountants have been doing their thing for centuries, computer programming has only been around as a career for a few decades. Best practices evolve over time, and we need to stay current. Practices that exist today, were still in their infancy 20 years ago. You will run into code that does not meet these standards because it was written before they existed.
We also grow as engineers throughout our careers. The code you wrote a year ago may not look like the code you wrote yesterday. You may look at that code and say to yourself “What was I thinking? What does `amtToRcvTl` even mean?”. For these and many other reasons, we should always be looking for ways to improve the readability, maintainability, and testability to make our codebase cleaner, more stable, and better than we found it.
Smells
Knowing how to write clean code is important in your road to becoming an amazing engineer. Equally important is being able to review some else’s code, spot code smells, and provide constructive feedback. Here is a list of some common code smells:
- Obsolete comments: Old comments.
- Redundant comments: Comments that describe something self-evident.
- Poorly-written comments: The worst writing over-explains details that nobody cares about.
- Commented-out code: This only rots over time. You have version control. Just remove it.
- Tests require more than one step: All unit tests should run with one command.
- Functions have too many arguments: Keep it down to zero, one, or two. More than three is questionable.
- Dead functions: Functions should be removed if they are never called.
- Duplication: Duplication in code is a missing opportunity for abstraction.
- Inconsistency: Do the same things the same way every time.
- Understand the algorithm: Make sure you understand exactly how your function works. Understandably, the process to get there came from little tweaks here and there, but truly get the full picture before you’re done.
- Replace magic numbers and strings with named constants: Magic numbers are those strange numbers that are not self-evident in purpose, but we need them to make specific calculations. Leaving the numbers as they are, without a label, can confuse future readers and obscure what the operation is doing.
