This is a refactoring where an example task goes through rounds of iteration, improving the design with commentary.
Our goal is to create a service for sending email. We need a set of options that fit into two sets:
- Service options, that will remain constant for the service’s lifetime, like transport configuration.
- Per-email options, that will be used to send a single email.
We separate the two for ergonomics.
A feature we need is that while developing we don’t actually want to send email. We can print emails to the console instead.
Round 1: Meet requirements
The first iteration is something “good enough.”
class EmailService {
constructor (serviceOptions) {
this.options = serviceOptions;
}
sendEmail (mailOptions) {
if (process.env.NODE_ENV !== 'production') {
console.log(`Sent email to ${mailOptions.recipient.email}`)
return
}
// TODO: Send email
}
}
Round 2: Clarify intent
The glaring issue with the above is the process.env.NODE_ENV check. This alone needs some iterations.
First, we have made the assumption that !production === development which is not always the case. With NODE_ENV=test we will be printing to the console. We should not be restricting ourselves to a two-mode Node environment within our email service.
- if (process.env.NODE_ENV !== 'production') {
+ if (process.env.NODE_ENV === 'development') {
Now it is more clear, you must be in development mode to print emails. But that sucks because we happen to test locally and we use a 3rd party testing tool that sets NODE_ENV=test and we really do not want to send email while doing anything locally. This is a mess because the staging CI server does need to send emails and it is running the same test tool.
If you cannot reliably meet your requirements, ask for more information. Our pain is coming from associating NODE_ENV mode with sending emails. Environment variables are great because it is like a dictionary or map, you can keep adding new key-values without breaking any existing code. What we really want is a IS_EMAIL_ENABLED environment variable that we can set independent of whatever NODE_ENV is.
- if (process.env.NODE_ENV === 'development') {
+ if (process.env.IS_EMAIL_ENABLED !== 'true') {
Now local testing and CI testing can differ and we can fiddle with this option for any other reason that may come up. We now have:
class EmailService {
constructor (serviceOptions) {
this.options = serviceOptions;
}
sendEmail (mailOptions) {
if (process.env.IS_EMAIL_ENABLED !== 'true') {
console.log(`Sent email to ${mailOptions.recipient.email}`)
return
}
// TODO: Send email
}
}
Round 3: Test perspective
Testing is something we all wish we had more time to do, but testing is often ignored because:
-
It is hard. Getting a test setup often is a lot of boilerplate. Writing mocks, providing fake data, performing an exact number of steps to approach the subject of the test is painful.
-
It is brittle. If you write a bunch of tests they are not free. Tests need to be maintained like any other code and if you modify code that touches them, tests will need rewritten too.
It is disheartening. The good news is good implementations’ tests are less likely to suffer these problems. This is because good implementations rely on the minimum amount of input and dependencies to do their jobs. It is easier to set them up and a change to the system is less likely to effect the implementation or its tests.
Unfortunately, we are not done harping on process.env. Look how we would have to test the email service, to make sure it adheres to not being enabled:
const oldValue = process.env.IS_EMAIL_ENABLED
process.env.IS_EMAIL_ENABLED = 'false'
// test
process.env.IS_EMAIL_ENABLED = oldValue
This is boilerplate and it is nasty for 3 reasons:
-
We had to write all that code just for the test to work. Yuck.
-
We have to look inside the implementation of the
EmailServiceto know to write this boilerplate. That is certainly problematic is anything changes there. -
We cannot run this test in parallel, unless we force that test to be synchronous (and sending email definitely is not). Our tests would have a race condition, which is bad for sanity and morale.
Functions should be referentially transparent, at every chance we get. “Referentially transparent” is fancy talk for given an input, the output should always be the same. The process.env can be externally changed and more importantly is not provided as an input. Let us get rid of that concern:
class EmailService {
constructor (serviceOptions) {
this.options = serviceOptions;
}
sendEmail (mailOptions) {
if (!this.options.shouldSendEmail) { // NEW
console.log(`Sent email to ${mailOptions.recipient.email}`)
return
}
// TODO: Send email
}
}
// Upon use:
const service = new EmailService({
shouldSendEmail: process.env.IS_EMAIL_ENABLED === 'true'
})
No more boilerplate and we can create tons of EmailService and test them in parallel.
Round 4: Document thee
Thinking of who is going to read this code, we should probably document just what the heck serviceOptions and mailOptions are. If the programming language supports destructuring, it is good to take advantage of it. In JavaScript, this is a nice way to describe some of the things you accept as input without reaching for JSDoc or TypeScript or Trent.
class EmailService {
constructor ({ shouldSendEmail, apiKey }) {
this.options = { shouldSendEmail, apiKey };
}
sendEmail ({ recipient: { email }, subject, messageText }) {
if (!this.options.shouldSendEmail) {
console.log(`Sent email to ${email}`)
return
}
// TODO: Send email
}
}
This pass can really happen anywhere. I personally start out destructuring from the get-go and add and remove as I shape requirements.
Round 5: Bare essentials
Object-oriented programming is appealing because of its familarity, but starting with a class first is starting out with a more complicated approach before you know it is necessary. We can strip down to exactly what we need to provide by going with plain functions.
const createEmailService = ({ shouldSendEmail, apiKey }) => ({
sendEmail ({ recipient: { email }, subject, messageText }) {
if (!shouldSendEmail) {
console.log(`Sent email to ${email}`)
return
}
// TODO: Send email
}
})
This simplification means we don’t need new, we do not have to program defensively to prevent an outside actor from manipulating this.options, we do not have worry about this which means further function composition has less cognitive overhead, and we are forcing composition over inheritance. There is less that can go wrong and less that can be exploited without the design accounting for it.
Thank you for reading.
An exercise for the reader: To test the console.log we would need write some boilerplate to trap those calls and check their arguments, how can it be rewritten so we do not need the boilerplate?
development
