"Programs should be written for people to read, and only incidentally for machines to execute." -- Structure and Interpretation of Computer Programs
"How would you define good code? [...] After lots of interviews we started wondering if we could come out with a definition of good code following a pseudo-scientific method. [...] The population is defined by all the software developers. The sample consists of 65 developers chosen by convenience. [...] The questionnaire consists in a single question: “What do you feel makes code good? How would you define good code?”. [...] Of those, the most common answer by far was that the code has to be Readable (78.46%), almost 8 of each 10 developers believe that good code should be easy to read and understand." -- "What is Good Code: A Scientific Definition"
- Follow the style guide
- Try not to go over 80 characters on any one line
- Use piping if your language has it
- Align your code
- DRY
- Use ample documentation
- Use variable names that explain themselves
- Explain the why, code the what
- Avoid intermediary assignment
- Use functions for code groups
- Consider functional programming
- Have someone else tell you what your code does
- Once you know where you want to go, refactor
If your language has a style guide, start by following it. That will get you more than 20% of the way there!
These principles are meant to go beyond the style guide.
There's a reason why newspapers arrange things in multiple columns -- it's much easier to read. We should also aim to do that with our code by keeping things under 80 characters per line.
Which is easier to read?
(reduce + (filter even? (take-while (partial > 4000000) ((fn fib [a b] (lazy-seq (cons a (fib b (+ a b))))) 0 1))))...or
(reduce +
(filter even?
(take-while (partial > 4000000)
((fn fib [a b] (lazy-seq (cons a (fib b (+ a b))))) 0 1))))x(a, z(y(b, c + d) + e), f). What is going on here?
Humans read left-to-right, but functions don't always go that way. But what if we could rewrite them so they could? Imagine we could use the pipe from Linux, where a | f(.) | g(.) means g(f(a)) and a | f(., x) means f(a, x).
x(a, z(y(b, c + d) + e), f) can then be rewritten as c + d | y(b, .) | z(. + e) | x(a, ., f) which is a bit clearer about the order of steps.
For another example, we can rewrite the Clojure code from earlier using the Clojure piping (aka threading) macro:
(let [fibs (fn f [a b] (lazy-seq (cons a (f b (+ a b)))))]
( ->> (fibs 0 1)
(take-while #(<= % 4000000))
(filter even?)
(reduce +)))...Now that Clojure code is a lot more clear about what is happening at each step.
apples = 2
pears = 3
oranges_and_bananas = 20...looks nicer than...
apples = 2
pears = 3
oranges_and_bananas = 20DRY stands for "Don't Repeat Yourself". Basically, if you find yourself rewriting the code over again, you should assign that code to a common variable or function so you don't rewrite it. This makes the code clearer and means that when you update the code you only have to update it in one place.
For example, if you see yourself doing:
z1 = x1 + 365 - f(y1)
z2 = x2 + 365 - f(y2)
z3 = x3 + 365 - f(y3)We might want to make x, y, and z into a list and use a map:
z = map(lambda x, y: x + 365 - f(y), x, y)DRY code is the opposite of WET code (write everything twice).
Use docstrings and parameter documentation to explain what is going on:
What does this mean?
def fv(rate, nper, pmt, pv, when='end'):
when = _convert_when(when)
(rate, nper, pmt, pv, when) = map(np.asarray, [rate, nper, pmt, pv, when])
temp = (1+rate)**nper
miter = np.broadcast(rate, nper, pmt, pv, when)
zer = np.zeros(miter.shape)
fact = np.where(rate == zer, nper + zer,
(1 + rate*when)*(temp - 1)/rate + zer)
return -(pv*temp + pmt*fact)Is it more clear with some docs?
def fv(rate, nper, pmt, pv, when='end'):
"""
Compute the future value.
Given:
* a present value, `pv`
* an interest `rate` compounded once per period, of which
there are
* `nper` total
* a (fixed) payment, `pmt`, paid either
* at the beginning (`when` = {'begin', 1}) or the end
(`when` = {'end', 0}) of each period
Return:
the value at the end of the `nper` periods
Parameters
----------
rate : scalar or array_like of shape(M, )
Rate of interest as decimal (not per cent) per period
nper : scalar or array_like of shape(M, )
Number of compounding periods
pmt : scalar or array_like of shape(M, )
Payment
pv : scalar or array_like of shape(M, )
Present value
when : {{'begin', 1}, {'end', 0}}, {string, int}, optional
When payments are due ('begin' (1) or 'end' (0)).
Defaults to {'end', 0}.
Returns
-------
out : ndarray
Future values. If all input is scalar, returns a scalar float. If
any input is array_like, returns future values for each input element.
If multiple inputs are array_like, they all must have the same shape.
Examples
--------
What is the future value after 10 years of saving $100 now, with
an additional monthly savings of $100. Assume the interest rate is
5% (annually) compounded monthly?
>>> np.fv(0.05/12, 10*12, -100, -100)
15692.928894335748
By convention, the negative sign represents cash flow out (i.e. money not
available today). Thus, saving $100 a month at 5% annual interest leads
to $15,692.93 available to spend in 10 years.
If any input is array_like, returns an array of equal shape. Let's
compare different interest rates from the example above.
>>> a = np.array((0.05, 0.06, 0.07))/12
>>> np.fv(a, 10*12, -100, -100)
array([ 15692.92889434, 16569.87435405, 17509.44688102])
"""
when = _convert_when(when)
(rate, nper, pmt, pv, when) = map(np.asarray, [rate, nper, pmt, pv, when])
temp = (1+rate)**nper
miter = np.broadcast(rate, nper, pmt, pv, when)
zer = np.zeros(miter.shape)
fact = np.where(rate == zer, nper + zer,
(1 + rate*when)*(temp - 1)/rate + zer)
return -(pv*temp + pmt*fact)Variable names should explain what they are. We all have tab complete in our text editors, so there's little excuse for abbreviating. Avoid naming variables x or mv or var or foo.
For example, we can improve
def fv(rate, nper, pmt, pv, when='end'):
when = _convert_when(when)
(rate, nper, pmt, pv, when) = map(np.asarray, [rate, nper, pmt, pv, when])
temp = (1+rate)**nper
miter = np.broadcast(rate, nper, pmt, pv, when)
zer = np.zeros(miter.shape)
fact = np.where(rate == zer, nper + zer,
(1 + rate*when)*(temp - 1)/rate + zer)
return -(pv*temp + pmt*fact)to...
def future_value(rate, num_compounding_periods, payment, present_value, when='end'):
when = _convert_when(when)
(rate, num_compounding_periods, payment, present_value, when) = map(np.asarray, [rate, num_compounding_periods, payment, present_value, when])
total_rate = (1+rate)**num_compounding_periods
miter = np.broadcast(rate, num_compounding_periods, payment, present_value, when)
zeros = np.zeros(miter.shape)
factors = np.where(rate == zeros, num_compounding_periods + zeros,
(1 + rate*when)*(total_rate - 1)/rate + zeros)
return -(present_value*total_rate + payment*factors)Comments are very useful for explaining what is going on.
But we don't need comments to tell what we already know:
# initalize a function to sum a list.
def sum_list(lst):
total = 0 # start the total at 0
for elem in lst: # iterate over each element in the list
total += elem # add that element to the total
return total # return the totalThat's just a waste of space and a violation of DRY.
Instead, we use comments to explain why things are happening:
def sum_list(lst):
total = 0
for elem in lst: # use for loop instead of `map` because it has faster performance
total += elem
return totalDon't assign things to temporary or intermediary variables if you can just make a functional chain:
(let [fibs (fn f [a b] (lazy-seq (cons a (f b (+ a b)))))]
( ->> (fibs 0 1)
(take-while #(<= % 4000000))
(filter even?)
(reduce +)))...is much better than
(def fibs (fn f [a b] (lazy-seq (cons a (f b (+ a b))))))
(def fibs_under_4M (take-while #(<= % 4000000) (fibs 0 1)))
(def even_fibs_under_4M (filter even? fibs_under_4M))
(def sum_fibs_under_4M (reduce + even_fibs_under_4M))...all those variable assignments are unnecessary when you can just thread.
If you find yourself doing a lot of:
def my_long_function(x):
## First, calculate the length of x and check for validity
len_x = len(x)
if len_x == 0:
raise ValueError('x was length 0!')
## Then check if x matches my string
return(x == "mystring")We can instead use functions instead of comments, to break things up and document with code instead of comments:
def my_long_function(x):
check_length_above_0(x)
return(check_matches_mystring(x))
def check_length_above_0(x):
if len(x) == 0:
raise ValueError('x was length 0!')
def check_matches_mystring(x):
x == "mystring"Following from above, you could consider adopting a style more in line with functional programming. "A practical introduction to functional programming" by Mary Rose Cook goes into detail on this.
The point is to go from:
squares = []
for i in range(4):
squares.append(i * i)
to...
map(lambda i: i * i, range(4))
Much better!
See also "What is functional programming?" by Kris Jenkins.
Lastly, if you're wondering if the code you've written is understandable, simply give it to someone else and see if they can tell you what is happening without you explaining it in advance! Add some comments and clean up the code if it doesn't happen.
To learn about refactoring, I recommend watching "All the Little Things" by Sandi Metz.
Here are some good exercises to verify understanding:
-
What is "flog"?
-
What is the "squint test"?
-
Why is it a bad idea to have a 43-line conditional statement?
-
Why are tests important for refactoring? Why do you not refactor until tests are passing?
-
What does "open / closed" mean? Why does that matter?
-
When is violating DRY okay?