clean code

use meaningful variable names

int d; // bad
// better alternatives:
int elapsedTimeInDays;
int daysSinceCreation;
int daysSinceModification;
int fileAgeInDays;

Dont' call lists list.

Classes and objects should have noun or noun phrase names like Customer, WikiPage, Account and AddressParser. Avoid words like Data or Info in the name of a class. A class name should not be a verb.

Methods too should have naming conventions:

Accessor methods should use the get prefix: getPrice.
Mutator methods should use the set prefix: setPrice.
Check methods should use the is prefix: isExpired.

Functions

Functions should be small.

Functions should be very small. This means, factor out as much as you can to other functions. When writing an if statement inside a function consider moving the block to a separate, nicely named function.

Use the Open/closed principle to allow for shorter and more extendable code. See this article.

When writing methods that contain many if else or switch statements consider instantiating an object using a factory and let the polymorphism handle the differences (cfr. Clean Code page 38-39).

Don't pass booleans as arguments. They hint that the function does indeed more than one thing (one thing if the flag is true, another if it's false). For example, a function renderTree(true) that renders a an expanded tree if the flag is truthy and compressed if falsy could be factored out to 2 more telling functions renderExpandedTree() (no args needed) and renderCompressedTree().

Limit the number of arguments to max 3 (the less the better). When a function takes more than 1 argument think if they are really connected. In the case of let geoPlace = new Coords(200, 244); the coords naturally come in pairs and are evidently related. But in the case of let geoMarker = Coords.placeMaker('yellow', allowExpandedDetails) the two arguments are not connected at all.

Look for expressive naming, leveraging a verb/noun paradigm. For example getField(fieldName) uses get(verb) + Field (name), along with an argument that sort of completes the sentence. Now it's clear that this method is a getter, that it targets a field and that the name of the field can be passed as argument. This is clear without even reading the implementation.

Beware of side-effects. Take this function:

def checkViewFilePermissions(user):
	perms = False
	if user['role'] is 'admin':
		perms = True
		file.viewable = True # <= this line is modifying an unrelated entity and shoud not be here
	return perms

It is touching an entity that is unrelated and it's not clear from the function name that this will happen. checkViewFilePermissionsAndMarkAsViewable could convey this idea, but this shows that this function is already doing too much.

The single purpose of functions means that they either:

change something
provide an answer to a question

Not both at the same time. Read above the difference in naming mutators and check methods.

This leads to the idea of raising exceptions rather than returning error codes. Sometimes we write a function that performs an operations ("change something"), but returns an error code in case of failed execution ("provide an answer to a question"). This encourages use of nested if statements.

At the same time try to factor out try/catch blocks. Rather than:

removeOrderFromTable: orderId => {
	try {
		let order = lookUpOrder(orderId);
		order.delete();
		modal.flash('success');
	} catch(e) {
		logger.throw('order not found');
	}
}

Use something along these lines:

deleteOrder: orderId => {
		let order = lookUpOrder(orderId);
		order.delete();
		modal.flash('success');
},
removeOrderFromTable: orderId => {
	try {
		this.deleteOrder(orderId);
	} catch(e) {
		logger.throw('order not found');
	}
}

Why is this a good practice? Because it enforces the single purpose of a function. In the first case the function is doing the mutation and the error handling. In the second case the error handling is moveed to its own function that does just that. A hint that a function is successfully handling errors and only that is to check 3 things:

if it start with the try as the first operation
if it does not contain logic after the catch or finally
if the try block is actually calling another function that actually implements the mutation logic.

Also, when using TDD start by writing tests that expect an exception to be thrown as the first thing. Then write the implementation of the try...catch as stated above. From there it's easy to carry on by writing tests and code without caring about errors as the subsequent functions (in this case deleteOrder()) simply do not care about errors.

Formatting

Use spaces between code blocks wisely.

const fs = require('fs');
const promise = require('bluebird');
class Song {
	constructor(details) {
		this.name = details.name;
		this.duration = details.duration;
		switch(details.genre) {
			case 'country':
			case 'gospel':
				this.sucks = true;
				break;
			case 'rock':
				this.rocks = true;
				break;
			default:
				this.unknown = true;
		}
	}
}

Could be made more clear by diving the relevant blocks in order to highlight them and make them easier to scan:

const fs = require('fs');
const promise = require('bluebird');

class Song {
	constructor(details) {
		this.name = details.name;
		this.duration = details.duration;
		
		switch(details.genre) {
			case 'country':
			case 'gospel':
				this.sucks = true;
				break;
			case 'rock':
				this.rocks = true;
				break;
			default:
				this.unknown = true;
		}
	}
}

nobitagit/clean_code.md

clean code

use meaningful variable names

Functions

Formatting