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

New linter to simplify negatives in conditional expressions? #1831

Open
IndrajeetPatil opened this issue Dec 10, 2022 · 7 comments
Open

New linter to simplify negatives in conditional expressions? #1831

IndrajeetPatil opened this issue Dec 10, 2022 · 7 comments

Comments

@IndrajeetPatil
Copy link
Collaborator

IndrajeetPatil commented Dec 10, 2022

Preamble

There ain't nobody that doesn't struggle with negatives, and it would be nice to have a linter that suggests a way to simplify boolean tests with negatives.

Here are a couple of ways to do this in a single linter.

1. Re-order if/else

Actual

if (!normal_case) {
  ...1
} else {
  ...2
}

Suggested

if (normal_case) {
  ...2
} else {
  ...1
}

2. Applying De Morgan's laws

Actual = Suggested

  • (not A) or (not B) = not (A and B)
if (!has_fuel || !has_mirrors) {
  can_be_rented <- FALSE
}

if (!(has_fuel && has_mirrors)) {
  can_be_rented <- FALSE
}
  • (not A) and (not B) = not (A or B)
if (!is_tank_empty && !is_mirror_broken) {
  can_be_rented <- TRUE
}

if (!(is_tank_empty || is_mirror_broken)) {
  can_be_rented <- TRUE
}

WDYT? Any other ways to simplify these?

@AshesITR
Copy link
Collaborator

Honestly, I prefer !has_fuel || !has_mirrors because parentheses use additional brainpower.

Also, control flow should be ordered in such a way that the surrounding code reads naturally. Therefor, it depends on context whether if (!cond) or if (cond) is more readable.

@MichaelChirico
Copy link
Collaborator

MichaelChirico commented Dec 11, 2022 via email

@IndrajeetPatil
Copy link
Collaborator Author

Honestly, I prefer !has_fuel || !has_mirrors because parentheses use additional brainpower.

Fair enough. But my preferred approach here would be to extract the conditional expressions to a new variable so that the parentheses can be avoided. Additionally, this introduces a reusable abstraction with an informative name.

# actual
if (!has_fuel || !has_mirrors) {
  can_be_rented <- FALSE
}

# refactored
is_car_in_good_condition <- has_fuel && has_mirrors
if (!is_car_in_good_condition) {
  can_be_rented <- FALSE
}

@IndrajeetPatil
Copy link
Collaborator Author

The first part of this issue was addressed by #2071.

I sense there is no consensus about whether the second part should be supported.

@MichaelChirico
Copy link
Collaborator

I sense there is no consensus about whether the second part should be supported.

I think consensus is more important for setting defaults -- it seems like a reasonable option to add, though I wouldn't enable it by default.

@IndrajeetPatil
Copy link
Collaborator Author

Sounds good to me.

Maybe we can call this parameter enforce_de_morgan_laws and set it FALSE by default?

@AshesITR
Copy link
Collaborator

use_de_morgans_laws = FALSE is more in line with our other parameter names. Also note the genitive "s".

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

3 participants