Add magic number linter?

[IndrajeetPatil]

It will be difficult to provide general advice in the warning message for this linter, but I think it might still be worth flagging this as a lint.

WDYT?

Example-1

# bad
if (length(x) > 0L) {
  print("I am not empty.")
}

# good
is_empty <- length(x) == 0L

if (!is_empty) {
  print("I am not empty.")
}

Example-2

# bad
if (age > 21L) {
  serve_drinks <- TRUE
}

# good
min_drinking_age_usa <- 21L
if (age > min_drinking_age_usa) {
  serve_drinks <- TRUE
}

Example-3

# bad
i <- 2L
while (i < 8L) {
  print(i)
  i <- i + 2L
}

# good
i <- 2
max_needed_even_number <- 8L
while (i < max_needed_even_number) {
  print(i)
  i <- i + 2
}

etc.

[MichaelChirico]

Personally I don't see much value to usage like this -- The idea is never to use a literal in an if/while condition? Is this covered in some style guide?

[IndrajeetPatil]

Sorry, forgot to reply.

Is this covered in some style guide?

I've seen this recommendation in a few places, but here is the best explanation on this issue from McConnell's Code Complete (2nd Ed.):

[AshesITR]

While the premise seems okay, I generally tend to avoid defining unnecessary symbols unless they are reused elsewhere in the code.

To me

a <- bitwAnd(x, 65535L)

is easier to read than

last_16_bits <- 65535L
a <- bitwAnd(x, last_16_bits)

[IndrajeetPatil]

But will a <- bitwAnd(x, 65535L) be as readable for another team member? Or even for you, if you are revising this piece of code after 5 years?

I would bet that 65535L would feel mystical then.

[AshesITR]

Nope, sorry. Binary constants are baked into my brain ^^ Otherwise, you could use 2^16 - 1 or add a comment to say why it's that number.

[IndrajeetPatil]

Sure, but then it's not useful for you.

I am imagining the most general case where such literals are scattered — with some literals used repeatedly — in a large codebase, where not each team member is familiar with all parts of the codebase and can still be expected to modify them, which is hard to do if you don't know what these numbers stand for.

In such cases, there are two options to systematically deal with magic numbers:

• make sure all such instances have comments explaining what these numbers represent

• create variables, numeric constants, etc. to demystify the raw literals and boost readability

In both cases, a linter can be a nifty tool. Once this is dealt with, the linter can be turned off.

[MichaelChirico]

while magic numbers are indeed avoidable, I don't think there's a good definition that can be enforced statically.

e.g. it would drive me insane if I saw a codebase like

fraction_to_pct = 100

treat_pct = fraction_to_pct * mean(df$is_treated)

and while I agree 65535L isn't good, 2^16-1 is perfectly fine. would the rule allow the latter?

maybe there's a rule that we could come up with but it looks tough to avoid seas of false positives to me.

[IndrajeetPatil]

But I am arguing that this linter be applied only to conditional expressions for this very reason. Thus the motivating examples in the top-level comment.

[AshesITR]

What about

if (length(dim(x)) > 2L) {
  stop("Only matrices and vectors allowed!")
}

[IndrajeetPatil]

There's more than one way to skin this cat:

• Create a new variable for magic number.

max_allowed_dim <- 2L # or matrix_dim <- 2L or ...
if (length(dim(x)) > max_allowed_dim) {
  stop("Only matrices and vectors allowed!")
}

• Keep the magic number but create a meaningful boolean variable, which won't lint since it's not a conditional expression. I personally prefer inlining guard clauses like these.

is_multidim_array <- length(dim(x)) > 2L
if (is_multidim_array) stop("Only matrices and vectors allowed!")

• If this is a recurring pattern, skip linting this literal altogether.

# config file
magic_number_linter(except = c(0L, 1L, 2L))

• If you don't want only this one instance of a literal to be linted.

if (length(dim(x)) > 2L) { # nolint: magic_number_linter.
  stop("Only matrices and vectors allowed!")
}

• Or just turn off this entire linter since you don't find it particularly useful. This won't be a default linter, so not many people will use it anyway.

# config file
magic_number_linter = NULL,