Clean Code Principles
It’s so hard and useless to write clean code from scratch. First, make your code work. Then cover it with tests. Your team gets time to test your code, and you have a space to make it clean
Clean code is a result of the refactoring
Functions
Principle 1. A name of a function should tell what it does without looking at the implementation
// It is a real function name from the project I participated in
func checkNoMovementDuringAction(
element: Element,
action: () -> Void
)
// What is the movement?
// And what happens if that movement occurs? I'd suggest a bool
// Why do we need an element here? Is it about the movement?// Let's rename it
func doActionAndThrowErrorIfElementPositionIsChanged(
action: () -> Void,
element: Element
)
// Well, now the name answers all questions above,
// but it's quite long, right?
// And it tells implementations details like error throwing,
// which is not good// Let's change its name again
func doActionAndEnsureElementPositionNotChanged(
action: () -> Void,
element: Element
)
// I think this variant is appropriate
// It's a kind of compromise between expressiveness and length
// 'ensure' describes things better than 'check' in this case
// And the name is abstract enough
Principle 2. Any function should be responsible for just one thing. If a function has two responsibilities, its name should reflect both. If it has three or more responsibilities — it’s a bad function
It’s also worth to mention that name of any function should tell about all side effects inside
Principle 3. ‘Action’ is a function that changes a state of a system and never returns a value. ‘Function’ is a function that returns a value and never changes a state of a system. A function shouldn’t be ‘Action’ and ‘Function’ at the same time
let result = accountRepository.update(account)
// What is the result? .success? updatedAccount? Both?
// And what if I don't need anything but just updating?// I tried to refactor
accountRepository.update(account)
let updatedAccount = accountRepository.getBy(id: account.id)...// However, the worst thing is when you expect the function
// just to return a value, but inside it updates the database
let accounts = accountService.createAccountsFrom(accountDTOs)
// createAccountsFrom uses accountRepository to save accounts// I would suggest the following
let accounts = accountService.createAccountsFrom(accountDTOs)
accountService.saveAccounts(accounts)// Or even rename 'create' to 'build' to avoid possible confusions
let accounts = accountService.buildAccountsFrom(accountDTOs)
accountsService.saveAccounts(accounts)// And one more piece of refactoring here ;)
let accounts = accountFactory.buildAccountsFrom(accountDTOs)
accountService.saveAccounts(accounts)
// ... perfect
Principle 4. There are no function parameters — perfect! Just one parameter — fine! Two parameters — well, it’s still okay. Three and more parameters — if your domain doesn’t tell that is right, it’s wrong
When it has a small number of parameters:
- Tests become easier
- It doesn’t take many pieces of knowledge to use
- It gets simpler to understand how it works
Principle 5. All rows in a particular function should belong to the same level of abstraction
It is one of my favorite principles :) Just look at the example below
// Bad
func start() {
initializeResources()
let topics = getTopicsFrom(config)
for topic in topics {
consumer.subscribe(topic: topic, fromBeginning: true)
}
runKafkaConsumer()
let logBody = [
"tenantAlias": getTenantAlias(),
"consumerGroup": getConsumerGroup(),
"kafkaHost": config.kafkaHost,
"cluster": config.cluster,
"deploymentType": config.deploymentType,
]
logger.info(logBody)
}// Good
func start() {
initializeResources()
subscribeToTopicsFromConfig()
runKafkaConsumer()
logKafkaConsumerStarted()
}
Principle 6. All rows in a particular function should follow the same convention
// Bad
func loadView() {
createSegmentedControlView()
self.tableView = createTableView()
createRootView(self.segmentedControlView, self.tableView)
}// Good
func loadView() {
self.segmentedControlView = createSegmentedControlView()
self.tableView = createTableView()
self.view = createRootView(
segmentedControlView: self.segmentedControlView,
tableView: self.tableView
)
}// or
func loadView() {
createSegmentedControlView()
createTableView()
createRootView()
}
// However, this variant is worse
// because the order of function calls here is important
// (temporal binding)
Principle 7. Temporal binding is a situation when the order of rows in a function is crucial. Functions should avoid having temporal bindings inside
In practice, it’s impossible to follow this principle in 100% of cases. Even some examples in this article don’t follow it. But it’s nice to reduce the number of temporal bindings as much as possible
// Bad
function setupSegmentedControlView() {
createSegmentedControlView()
setSegmentedControlConstraints()
}
// If setting constraints of the control stays first,
// it will produce a runtime error// Good
func setupSegmentedControlView() {
self.segmentedControlView = createSegmentedControlView()
setConstraintsOf(segmentedControl: self.segmentedControlView)
}
// It's impossible to call setting constraints
// before the control is created
Principle 8. When it’s not obvious what the function’s call is about, it’s nice to store the call’s result in a variable with an expressive name
// Bad
dictionary[regexpMatch.group(0)] = regexpMatch.group(1)// Good
let id = regexpMatch.group(0)
let name = regexpMatch.group(1)
dictionary[id] = name
Principle 9. If several conditional operators belong to the same logical condition, they should be compounded into a single function, property, or variable with an expressive name
// Bad
if timer.isExpired && !timer.isRecurrent { ... }// Good
class Timer {
var shouldBeDeleted: Bool {
return isExpired && !isRecurrent
}
}
...
if timer.shouldBeDeleted { ... }// or
func shouldTimerBeDeleted(_ timer: Timer) -> Bool {
return timer.isExpired && !timer.isRecurrent
}
...
if shouldTimerBeDeleted(timer) { ... }
p.s. Every name should be expressive
Principle 10. Error handling should take a separate layer of abstraction
It shouldn’t be messed up with other logic
// This example uses JavaScript (btw, other examples use Swift)
// ... Swift is the best! ;)// Bad
async function performRequest(...) {
try {
const endpoint = createEndpoint(...)
const body = createRequestBody(...)
const request = createRequest(endpoint, body)
await request.perform()
} catch {
console.log("Oops. Failed request")
}
}// Good
async function performRequest(...) {
const endpoint = createEndpoint(...)
const body = createRequestBody(...)
const request = createRequest(endpoint, body)
await request.execute()
}
async function tryToPerformRequest(...) {
try {
performRequest(...)
} catch {
console.log("Oops. Failed request")
}
}
Principle 11. Object function should have a higher priority over a static one
The main reason is that a particular function can be polymorphic in the future. But, it doesn’t say that it shouldn’t use a static function at all!
Principle 12. Function length should be equal to or less than 20 rows. The depths of nesting if/else should be equal to or less than 2
Classes, Structures, and Other Custom Types
Principle 1. A class name should stick to the standard naming
E.g., if a class conforms to the BUILDER pattern, its name should contain the ‘Builder’ word — class AccountBuilder
Principle 2. A protocol name shouldn’t contain the ‘Protocol’ word — ‘AccountSubscriptionProtocol’ It is better to define a protocol ‘AccountSubscription’ but if it has just one class that conforms it, call the class ‘AccountSubscriptionDefault’ which means ‘default’ implementation
Principle 3. You should avoid using abstract words like Processor, Data, Info in class/struct names
E.g., what is the difference between Account and AccountInfo? What is the info? There could be Account and AccountLegalDetails
Principle 4. You should avoid transitive dependencies
Transitive dependencies are difficult to refactor. And they break encapsulation
class MyCuteBrowser {
let browser: WebBrowser
func open(url: URL?) { ... }
}let myCuteBrowser = MyCuteBrowser()
myCuteBrowser.open(url: URL("http://google.com"))// Bad
myCuteBrowser.browser.reloadCurrentPage() // Transitive dependency// Good
class MyCuteBrowser {
private browser: WebBrowser
...
func reloadCurrentPage() {
browser.reloadCurrentPage()
}
}
...
myCuteBrowser.reloadCurrentPage()
Principle 5. Every class should have the same vertical separation
// There can be used the principle ‘put the code next to its usage’
class ViewController: UIViewController {
override func viewDidLoad() {
super.viewDidLoad()
setupTableView()
setupBottomButton()
}
private setupTableView() { ... }
private setupBottomButton() { ... } override func updateViewConstraints() {
super.updateViewConstraints()
...
}
}
// internal private private, internal...// Or there can be a single convention about
// where functions and properties should be placed
class ViewController: UIViewController {
// MARK: - Lifecycle
override func viewDidLoad() {
super.viewDidLoad()
setupTableView()
setupBottomButton()
}
override func updateViewConstraints() {
super.updateViewConstraints()
...
} // MARK: - Private methods
private setupTableView() { ... }
private setupBottomButton() { ... }
}
General
Principle 1. The same word should have just one meaning. And only this word should be used everywhere when talking about the same thing. There should be no other synonyms
E.g., if ‘add’ means appending an element to an array, or a character to a string, don’t use it to have a sum of numbers. Use ‘plus’ for it
E.g., if ‘create’ means just creating an object and return it, don’t use it when you want to create something in the database. Use ‘save’ instead. And don’t use synonyms like ‘build’ or ‘make’ Otherwise, rename this word everywhere (DDD, ubiquitous language)
Principle 2. You should put pieces of code in places where you expect to find them
Suppose you open a class or file to find some function but don’t see it. And then you recall this function is placed in another file. It is nice to think about moving the function to the first file you’ve visited
Of course, if it doesn’t contradict architectural principles!
Principle 3. You shouldn’t use external libraries directly in your code. You should use wrappers on them
Following this principle allows:
- Having an ability to quickly replace one library with another without changing the rest code (rare case, but still)
- Picking an interface better for your needs (you can choose another name of a method or even combine several methods to a single one)
- Writing tests for the wrappers that will tell your teammates how they are used
Principle 4. Client code shouldn’t be responsible for creating objects, but only for their usages. It should contain a layer consists of factories to create all objects. The FACTORY pattern allows creating objects, entities, satisfying all invariants (hi there, Eric Evans). And the DEPENDENCY INJECTION pattern tells how the objects should be passed around (SOLID, Dependency Inversion Principle)
// Bad
class MyCuteBrowser {
private let browser = WebBrowser()
...
}// Good
class MyCuteBrowser {
private let browser: WebBrowser
init(browser: WebBrowser) {
self.browser = browser
}
}class MyCuteObjectFactory {
...
func createMyCuteBrowser() {
return MyCuteBrowser(browser: WebBrowser())
}
}
Principle 5. All constants should be defined on the highest level or even in separate config classes/structures rather than be magical values inside the code
Principle 6. Having code duplication is bad. Duplicated rows should be moved into a separate function. And repeated switch/case — into some ABSTRACT FACTORY
// Bad
func tableView(..., willDisplay cell: UITableViewCell, ...) {
let entity = entities[indexPath.row]
switch entity {
case is Movie: ...
case is PopularPerson: ...
default: ...
}
}
func tableView(..., cellForRowAt indexPath: IndexPath) -> ... {
let entity = entities[indexPath.row]
switch entity {
case is Movie: ...
case is PopularPerson: ...
default: ...
}
}// Good
func tableView(..., willDisplay cell: UITableViewCell, ...) {
let entity = entities[indexPath.row]
getEntityUtilsFactory(by: entity)
.setMultiSearchTableViewCellImageProperties(
cell: cell,
by: indexPath
)}
func tableView(..., cellForRowAt indexPath: IndexPath) -> ... {
let entity = entities[indexPath.row]
return getEntityUtilsFactory(by: entity)
.getMultiSearchTableViewCell(
from: tableView,
by: indexPath
)
}
func getEntityUtilsFactory(
by entity: MultiSearchEntity
) -> EntityUtilsAbstractFactory {
switch entity {
case is Movie: return movieUtilsFactory
case is PopularPerson: return personUtilsFactory
default: fatalError("Entity type is unexpected.")
}
}
class EntityUtilsAbstractFactory {
func setMultiSearchTableViewCellImageProperties(
cell: UITableViewCell,
by indexPath: IndexPath
) { }
func getMultiSearchTableViewCell(
from tableView: UITableView,
by indexPath: IndexPath
) -> UITableViewCell {
fatalError("getTableViewCell(..) must be overridden.")
}
}
Principle 7. You shouldn’t make a decision on how your code will be used
Yet another principle I especially like :) It is related to SOLID, Open-Closed Principle
// Let's imagine you have a type Person
struct Person {
let name: String
let age: Int
}// And you decided to create a greeting generator for people
class PersonGreetingGenerator {
// And you said: "Oh, it's always enough to have just a name
// to create a greeting"
// You decided that a person name is enough to use your code
func createGreetingFor(personName: String) -> String {
return "Hello, \(personName)!"
}
}// And what's wrong?// Okay. Let's imagine that we have the following function
func greet(
person: Person,
via generator: PersonGreetingGenerator
) {
let greeting = generator.createGreetingFor(
personName: person.personName
)
print(greeting)
}// And we want to generate a special greeting for old people
class AgeSpecificPersonGreetingGenerator: PersonGreetingGenerator {
// Well, now it's not enough to have just a name
... // Should we create one more function like this?
func createGreetingFor(
personName: String,
personAge: Int
) -> String {
...
}
}// But how can we use it with different generators?
greet(person: person, via: defaultGenerator)
greet(person: oldPerson, via: ageSpecificGenerator)
// Should we create one more function? Or add switch/case there?// No// We shouldn't decide what person fields should be used
// We just say: "A person greeting generator works with a person"
class PersonGreetingGenerator {
func createGreetingFor(person: Person) -> String {
return "Hello, \(person.name)!"
}
}
class AgeSpecificPersonGreetingGenerator: PersonGreetingGenerator {
override func createGreetingFor(person: Person) -> String {
...
}
}
func greet(
person: Person,
via generator: PersonGreetingGenerator
) {
let greeting = generator.createGreetingFor(person: person)
print(greeting)
}greet(person: person, via: defaultGenerator)
greet(person: oldPerson, via: ageSpecificGenerator)
Principle 8. You shouldn’t have pieces of code that are never used
It’s quite a common situation when there are commented rows ‘not to lose changes’ Or class methods included in runtime but never called. Don’t be worry about losing some knowledge. Git remembers everything
Principle 9. All pieces of code should be consistent — follow the same conventions. However, if the conventions contradict code best practices, they (conventions) should be reconsidered
P.S.
First of all, I would like to say thanks to Robert Martin and Eric Evans. Almost everything in this article I took from ‘Clean Code’ and ‘Domain-Driven Design’ I’m a big fan of those guys!
There are principles I didn’t include (TDD, parallel programming, …). I didn’t do it for several reasons:
- I didn’t manage to put some of them into practice
- For now, I’m quite stupid to understand some others
- Some of them I simply might miss while reading
- And others are obvious (follow SOLID, keep modularity, comments are bad, …)
I highly recommend everyone to read these books. You will find there a right way to design systems and write extensible and readable code. And it can help you to fall in love with the development process, and not consider the development is just a work
Another P.S.
I think it’s wrong to follow each principle in 100% of cases
E.g., Apple doesn’t do this when it returns the removed element from an array while using ‘remove(at:)’ — it behaves like a ‘Function’ and ‘Action’ at the same time. And it is quite comfortable to use
However, when you decide not to follow a particular principle, try to pick a strong defense argument. Because in most cases, these principles work
And The Last P.S.
Keep your code straightforward. Don’t try to use elaborate solutions with generic types, tons of abstractions, etc. People call it ‘overengineering’
Sometimes I check myself by showing my code to people who are quite far from development. And if I’m writing on Swift, a Java developer should mostly understand it, at least its Domain part
In the very beginning, I saw complicated code written by ‘senior developers’ I worked with. It was QUITE challenging to understand it, and when I came at them and asked: “Could you, please, clarify what is going here?” They said: “What exactly you didn’t understand?”
Funny…
I felt so stupid because I understood almost nothing and didn’t manage to identify particular things
Well, looking at their code today and keeping in mind the principles and how it could be, I wouldn’t say: “Could you, please, clarify..” anymore. I would say: “Could you, please, refactor this piece of shit? My time is valuable enough not to spend it digging in over 100-row functions, to check that you correctly build a request object and send it to the server”
References
- Clean Code: A Handbook of Agile Software Craftsmanship, by Robert C. Martin
- Domain-Driven Design: Tackling Complexity in the Heart of Software, by Eric Evans
