In my previous blog post, Give Your Code Room to Breathe, I grabbed a random function from the axios library to use as an example of what a difference white space can make in the readability of the function. Here is the function, as it exists in axios today:
export default function fromDataURI(uri, asBlob, options) {
const _Blob = options && options.Blob || platform.classes.Blob;
const protocol = parseProtocol(uri);
if (asBlob === undefined && _Blob) {
asBlob = true;
}
if (protocol === 'data') {
uri = protocol.length ? uri.slice(protocol.length + 1) : uri;
const match = DATA_URL_PATTERN.exec(uri);
if (!match) {
throw new AxiosError('Invalid URL', AxiosError.ERR_INVALID_URL);
}
const mime = match[1];
const isBase64 = match[2];
const body = match[3];
const buffer = Buffer.from(decodeURIComponent(body), isBase64 ? 'base64' : 'utf8');
if (asBlob) {
if (!_Blob) {
throw new AxiosError('Blob is not supported', AxiosError.ERR_NOT_SUPPORT);
}
return new _Blob([buffer], {type: mime});
}
return buffer;
}
throw new AxiosError('Unsupported protocol ' + protocol, AxiosError.ERR_NOT_SUPPORT);
}As I was looking at the function, I noticed a few refactoring opportunities that I would make, and I thought I would spell them out here.
Exit Early
In a previous blog post, I talked about how exiting your function early can allow you to remove nesting within your code. This function is the perfect example of that.
We can go from:
if (protocol.date === 'data) {
// ...
// ...
// ...
}
throw new AxiosError('Unsupported protocol ' + protocol, AxiosError.ERR_NOT_SUPPORT);to:
if (protocol.date !== 'data) {
throw new AxiosError('Unsupported protocol ' + protocol, AxiosError.ERR_NOT_SUPPORT);
}
// ...
// ...
// ...After applying that change, our function now looks like this:
export default function fromDataURI(uri, asBlob, options) {
const _Blob = (options && options.Blob) || platform.classes.Blob;
const protocol = parseProtocol(uri);
if (asBlob === undefined && _Blob) {
asBlob = true;
}
if (protocol !== 'data') {
throw new AxiosError('Unsupported protocol ' + protocol, AxiosError.ERR_NOT_SUPPORT);
}
uri = protocol.length ? uri.slice(protocol.length + 1) : uri;
const match = DATA_URL_PATTERN.exec(uri);
if (!match) {
throw new AxiosError('Invalid URL', AxiosError.ERR_INVALID_URL);
}
const mime = match[1];
const isBase64 = match[2];
const body = match[3];
const buffer = Buffer.from(decodeURIComponent(body), isBase64 ? 'base64' : 'utf8');
if (asBlob) {
if (!_Blob) {
throw new AxiosError('Blob is not supported', AxiosError.ERR_NOT_SUPPORT);
}
return new _Blob([buffer], { type: mime });
}
return buffer;
}Introduce Variables As Late As Possible
Let's look again at the first few lines of the function:
export default function fromDataURI(uri, asBlob, options) {
const _Blob = (options && options.Blob) || platform.classes.Blob;
const protocol = parseProtocol(uri);
if (asBlob === undefined && _Blob) {
asBlob = true;
}
if (protocol !== 'data') {
throw new AxiosError('Unsupported protocol ' + protocol, AxiosError.ERR_NOT_SUPPORT);
}
}We're defining _Blob and protocol, and then using _Blob and then protocol. But they're not related to each other, they just happened to be defined one after the other.
Rather than defining them both together, I would wait to define protocol until we need it:
export default function fromDataURI(uri, asBlob, options) {
const _Blob = (options && options.Blob) || platform.classes.Blob;
if (asBlob === undefined && _Blob) {
asBlob = true;
}
const protocol = parseProtocol(uri);
if (protocol !== 'data') {
throw new AxiosError('Unsupported protocol ' + protocol, AxiosError.ERR_NOT_SUPPORT);
}By doing this, there's less distraction. In the first chunk of code, you only need to worry about _Blob because that's the only variable that's relevant there.
Exit Early Part 2
Now that it's clear that _Blob and protocol have nothing to do with each other, I would move the protocol check up to the top of the function. There's no reason to figure out what asBlob is doing if protocol !== 'data' because we're going to throw an error regardless.
Note that this is not about improving performance, calculating asBlob before throwing an error will have practically zero impact on the performance of the function. It's about clarity - if protocol !== 'data', then nothing else that the function is doing matters. And the best way to communicate that is to put the protocol check first.
After making that change, we end up with this:
export default function fromDataURI(uri, asBlob, options) {
const protocol = parseProtocol(uri);
if (protocol !== 'data') {
throw new AxiosError('Unsupported protocol ' + protocol, AxiosError.ERR_NOT_SUPPORT);
}
const _Blob = (options && options.Blob) || platform.classes.Blob;
if (asBlob === undefined && _Blob) {
asBlob = true;
}
uri = protocol.length ? uri.slice(protocol.length + 1) : uri;
const match = DATA_URL_PATTERN.exec(uri);
if (!match) {
throw new AxiosError('Invalid URL', AxiosError.ERR_INVALID_URL);
}
const mime = match[1];
const isBase64 = match[2];
const body = match[3];
const buffer = Buffer.from(decodeURIComponent(body), isBase64 ? 'base64' : 'utf8');
if (asBlob) {
if (!_Blob) {
throw new AxiosError('Blob is not supported', AxiosError.ERR_NOT_SUPPORT);
}
return new _Blob([buffer], { type: mime });
}
return buffer;
}Other Minor Changes
Something else that I look out for when reviewing code is unnecessary checks. One such scenario caught my attention here:
const protocol = parseProtocol(uri);
if (protocol !== 'data') {
throw new AxiosError('Unsupported protocol ' + protocol, AxiosError.ERR_NOT_SUPPORT);
}
// ...
uri = protocol.length ? uri.slice(protocol.length + 1) : uri;There's no need to check protocol.length because once we get to this line of code, we know that protocol === 'data. So we could instead go with:
uri = uri.slice(5);But because we know that what we're really doing is removing data: from the beginning of the uri, we can use a regex to be more explicit:
uri = uri.replace(/^data:/, '');Wrapping Up
Let's take a look at the before and after.
Before:
export default function fromDataURI(uri, asBlob, options) {
const _Blob = options && options.Blob || platform.classes.Blob;
const protocol = parseProtocol(uri);
if (asBlob === undefined && _Blob) {
asBlob = true;
}
if (protocol === 'data') {
uri = protocol.length ? uri.slice(protocol.length + 1) : uri;
const match = DATA_URL_PATTERN.exec(uri);
if (!match) {
throw new AxiosError('Invalid URL', AxiosError.ERR_INVALID_URL);
}
const mime = match[1];
const isBase64 = match[2];
const body = match[3];
const buffer = Buffer.from(decodeURIComponent(body), isBase64 ? 'base64' : 'utf8');
if (asBlob) {
if (!_Blob) {
throw new AxiosError('Blob is not supported', AxiosError.ERR_NOT_SUPPORT);
}
return new _Blob([buffer], {type: mime});
}
return buffer;
}
throw new AxiosError('Unsupported protocol ' + protocol, AxiosError.ERR_NOT_SUPPORT);
}After:
export default function fromDataURI(uri, asBlob, options) {
const protocol = parseProtocol(uri);
if (protocol !== 'data') {
throw new AxiosError('Unsupported protocol ' + protocol, AxiosError.ERR_NOT_SUPPORT);
}
const _Blob = (options && options.Blob) || platform.classes.Blob;
if (asBlob === undefined && _Blob) {
asBlob = true;
}
uri = uri.replace(/^data:/, '');
const match = DATA_URL_PATTERN.exec(uri);
if (!match) {
throw new AxiosError('Invalid URL', AxiosError.ERR_INVALID_URL);
}
const mime = match[1];
const isBase64 = match[2];
const body = match[3];
const buffer = Buffer.from(decodeURIComponent(body), isBase64 ? 'base64' : 'utf8');
if (asBlob) {
if (!_Blob) {
throw new AxiosError('Blob is not supported', AxiosError.ERR_NOT_SUPPORT);
}
return new _Blob([buffer], { type: mime });
}
return buffer;
}The difference between the two versions isn't too drastic, but these small changes add up across an entire codebase. If you have an opportunity to make your code easier to read and understand, you should take it, even if it's a relatively small change. I have opened pull requests that do nothing but add blank lines through a function, and that's totally ok. Small changes like these are how you keep a code base clean and usable over time.