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

Parameterize if_switch_linter() not to require switch() for "highly-complicated" clauses #2322

Closed
MichaelChirico opened this issue Nov 20, 2023 · 5 comments · Fixed by #2413
Labels
feature a feature request or enhancement
Milestone

Comments

@MichaelChirico
Copy link
Collaborator

if_switch_linter() is too strict -- in some cases,using switch() is actually quite awkward.

if (x == 'a') {
  do_1()
  do_2()
  do_3()
  if (y > 2) {
    do_4()
    do_5()
  }
} else if (x == 'b') {
  do_6()
  do_7()
  do_8()
  do_9()
  do_10()
  for (i in 1:10) {
    do_11()
    do_12(0
  }
} else if (x == 'c') {
  do_13()
  do_14()
  do_15()
  do_16()
  do_17()
} else if (x == 'd') {
  do_18()
  do_19()
  do_20()
  do_21()
  do_22()
}

Would become

switch(x,
  a = {
    do_1()
    do_2()
    do_3()
    if (y > 2) {
      do_4()
      do_5()
    }
  },
  b = {
    do_6()
    do_7()
    do_8()
    do_9()
    do_10()
    for (i in 1:10) {
      do_11()
      do_12(0
    }
  },
  c = {
    do_13()
    do_14()
    do_15()
    do_16()
    do_17()
  },
  d = {
    do_18()
    do_19()
    do_20()
    do_21()
    do_22()
  }
)

Note the increased nesting, and the distance between switch(x and the x value b = {.

It would be good to come up with a rule to weaken if_switch_linter() to not apply to cases where the branches are "overly complicated". The trouble is coming up with a good rule for what that means (e.g. cyclocomp_linter() only cares about branching, not the number of expressions).

One simple option is to just count the number of "child expressions" in each branch, and not lint if, say max(expressions) > complexity where complexity= is an argument set by the user, perhaps with a sensible default like 3L.

@MichaelChirico MichaelChirico added the feature a feature request or enhancement label Nov 20, 2023
@AshesITR
Copy link
Collaborator

What about number of lines as a proxy?

@MichaelChirico
Copy link
Collaborator Author

I think best next step is to pull some "real" examples. not clear to me whether it's strictly LoC or code complexity that's the source of the issue here. Could be both, and we offer two parameters.

@MichaelChirico MichaelChirico added this to the 3.2.0 milestone Dec 5, 2023
@IndrajeetPatil
Copy link
Collaborator

IMO, code complexity would be a better measure.

I could potentially have a single function call in a given branch but many LoC just because the function has many parameters (think ggplot2::theme()) which are formatted to be one per line for readability.

@MichaelChirico
Copy link
Collaborator Author

still the distance between the conditions seems important. with switch(x, ....) we might lose mental context that x is what's switched on, with if/else x is reminded every condition.

so why not two parameters, one for LoC and one for # expressions?

@IndrajeetPatil
Copy link
Collaborator

Fair enough! Let's offer two options then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature a feature request or enhancement
Projects
None yet
3 participants