Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lint repeated if/else if/ else after a certain threshold? #1868

Closed
IndrajeetPatil opened this issue Dec 21, 2022 · 4 comments
Closed

Lint repeated if/else if/ else after a certain threshold? #1868

IndrajeetPatil opened this issue Dec 21, 2022 · 4 comments

Comments

@IndrajeetPatil
Copy link
Collaborator

After a certain point, a block of conditional statements can become quite difficult to read, maintain, and extend further.

To see what I mean, consider this example from {insight}:

if (transform_fun == "identity") {
  out <- list(transformation = function(x) x, inverse = function(x) x)
} else if (transform_fun == "log") {
  out <- list(transformation = log, inverse = exp)
} else if (transform_fun %in% c("log1p", "log(x+1)")) {
  out <- list(transformation = log1p, inverse = expm1)
} else if (transform_fun == "log10") {
  out <- list(transformation = log10, inverse = function(x) NA)
} else if (transform_fun == "log2") {
  out <- list(transformation = log2, inverse = function(x) NA)
} else if (transform_fun == "exp") {
  out <- list(transformation = exp, inverse = log)
} else if (transform_fun == "sqrt") {
  out <- list(transformation = sqrt, inverse = function(x) x^2)
} else if (transform_fun == "power") {
  out <- list(transformation = function(x) x^2, inverse = sqrt)
} else if (transform_fun == "expm1") {
  out <- list(transformation = expm1, inverse = log1p)
} else if (transform_fun == "log-log") {
  out <- list(
    transformation = function(x) log(log(x)),
    inverse = function(x) exp(exp(x))
  )
}

The alternative here is to create a look-up table (using a data frame, named vector/list, etc.), which is much easier to read, and importantly, extend - we just need to add another row for every new transformation:

df <- tibble::tribble(
  ~transform_fun, ~out,
  "identity",     list(transformation = function(x) x, inverse = function(x) x),
  "log",          list(transformation = log, inverse = exp),
  "log1p",        list(transformation = log1p, inverse = expm1),
  "log(x+1)",     list(transformation = log1p, inverse = expm1),
  "log10",        list(transformation = log10, inverse = function(x) NA),
  "log2",         list(transformation = log2, inverse = function(x) NA),
  "exp",          list(transformation = exp, inverse = log),
  "sqrt",         list(transformation = sqrt, inverse = function(x) x^2),
  "power",        list(transformation = function(x) x^2, inverse = sqrt),
  "expm1",        list(transformation = expm1, inverse = log1p),
  "log-log",      list(transformation = function(x) log(log(x)), inverse = function(x) exp(exp(x)))
)

That said, I am not sure where to draw the line, i.e., how many repeats of if + else if is too many. Maybe this can be configurable?

Also, note that this is a different linter from any of the existing or proposed ones:

That said, IINM, it does have some overlap with Google's IfSwitchLinter().

@MichaelChirico
Copy link
Collaborator

yes, this is basically the IfSwitchLinter. We have not deployed that in the end internally -- it needs some fine-tuning.

I think the key is the cyclomatic complexity of the internal expressions. when each branch is complex, branching actually helps readability we found.

@MichaelChirico
Copy link
Collaborator

Related thing I've been meaning to write some linting for eventually (not sure if it can be subsumed here too):

if (x == "a") {
  # code that doesn't overwrite x
}
if (x == "b") {
  # code that doesn't overwrite x
}

can be collapsed to an if/else. the part where we also have to test if the condition variable is changing makes it tough.

@MichaelChirico
Copy link
Collaborator

See #2304 for the initial implementation of a related linter.

@IndrajeetPatil
Copy link
Collaborator Author

Closed by #2304

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants