This has to be the oldest trick in the book in the sense that only really early beginner make these mistakes.
Instead of
class User
attr_reader :username
attr_reader :email
def is_valid?
valid = !self.username.blank? && !self.password.blank?
valid == true
end
end
Do this
class User
attr_reader :username
attr_reader :email
def is_valid?
!self.username.blank? && !self.password.blank?
end
end
Why?
Boolean expressions eventually resolve to either true
or false
. Comparing them again to true
wastes computation and adds extra cognitive load without actually adding to the readability or explicitlness.
Why Not?
Sometimes you don't always have a binary true
or false
values. For example, you can have undefined
, null
or nil
values in other languages. In which case, comparing them to true
do have a use because you want to avoid the other two (or three) possibility of the value not existing, or the value being false
at the same time.
let result = render(template); // default options `{}`
result = render(template, { escape: true }); // explicit options
function render(content, options = {}) {
if (options['escape'] === true) {
content = htmlEscape(content)
}
// ...
}
It may, however, confuses the reader because the absence of value is implicit in this context. Calling out implicitness usually improves readability but reduce succintness and adds conginitive load. So depending on your situation, shorter may be better or explicit may be better.
One way to make this explicit is to split apart the two concerns in the code:
- Checking for absence of value
- Checking for the value itself
Like so:
function render(content, options = {}) {
if (typeof options['escape'] !== 'undefined' && options['escape']) {
content = htmlEscape(content)
}
// ...
}
This code calls out to the reader to be on the lookout for the absence of values in the options
hash. That not all options maybe passed into the function.
Alternatively, we can eliminate 1) entirely by supplying defaults:
function render(content, options = {}) {
if (!options.hasOwnProperty('escape')) {
options['escape'] = defaults['escape']
}
// now we are sure the 'escape` option will always be specified
// our check is now straightforward
if (options['escape']) {
content = htmlEscape(content)
}
// ...
}
An experienced coder will also notice that the pattern of "applying defaults" can be made more generic and applied uniformly throughout the codebase. This is exactly the usecase of lodash's _.defaults() method which we can use to make the code more succinct:
function render(content, options = {}) {
_.defaults(options, defaults); // eliminate any "absence of value" cases
if (options['escape']) {
content = htmlEscape(content)
}
// ...
}
We can avoid Lodash entirely and use null-coalescing operator ??
if your language supports them:
function render(content, options = {}) {
if (options['escape'] ?? false) {
content = htmlEscape(content)
}
// ...
}
Top comments (0)