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

S4144 should ignore functions with few statements or a single expression #3288

Closed
gabriel-vivas-sonarsource opened this issue Aug 1, 2022 · 1 comment · Fixed by SonarSource/eslint-plugin-sonarjs#366 or SonarSource/rspec#1151
Assignees
Labels
Milestone

Comments

@gabriel-vivas-sonarsource
Copy link

gabriel-vivas-sonarsource commented Aug 1, 2022

Label Description
Rule S4144 "Functions should not have identical implementations"
FP Example * JSX component mapping

Explanation

  • It seems that the current implementation is ignoring functions that have only a couple of lines, which seems fair.
  • However, this also means the rule is finding an issue for functions that have a few statements in multiple lines. This is particularly visible when you have a single expression that takes multiple lines.
  • Particularly in JSX, there can be a single expression that can take many lines. Like when mapping over an Array to create JSX components. See the given example.
  • The coupling of using a common function is not worth the little duplication, since in this case there would be little or no logic involved.

Suggestion

  • The rule should ignore functions with a single expression
  • The rule should ignore functions with few statements
  • We should consider ignoring arrow functions that are passed to Array methods like map, filter, find, etc. unless they have many statements.
@gabriel-vivas-sonarsource gabriel-vivas-sonarsource added this to the 9.6 milestone Aug 1, 2022
@gabriel-vivas-sonarsource gabriel-vivas-sonarsource changed the title S4144 should be based on statements and not lines S4144 should ignore functions with few statements or a single expression Aug 1, 2022
@francoismora francoismora self-assigned this Aug 4, 2022
@francoismora francoismora reopened this Aug 4, 2022
@francoismora
Copy link
Contributor

I changed the rule to look only at function declaration, method definition, and variable declarations. That's to say, it only looks at functions that are defined in a syntactic scope ignoring all function expressions.

// Function declarations
function foo() {
}

class Foo {
  // Method definitions
  foo() {
  }
}

// Variable declarators:
const foo = () => {
};

The motivation is that it's not practically possible to determine when a function returns just an expression, as there are many different types of expressions. Moreover, there are several ways for a function to return a simple expression (block with a return statement, expression in the body).

Therefore we can expect some true positives to be ignored from now on.
image

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