Improve S4144: detects duplicates when variable names differ

[vilchik-elena]

function foo(p1, p2) {
  var a = p1 + p2;
  return a * 42;
}

function bar(p3, p4) {
  var b = p3 + p4;
  return b * 42;
}

[vilchik-elena]

relates to #599

[christophercr]

Hi,

Any new about this issue? I also have this problem and it is quite annoying because in my case it detects such duplication in many files and this is preventing me from upgrading from v1.6 to 1.7

[stas-vilchik]

@christophercr this is planned for 1.8.

[vilchik-elena]

@christophercr this issue is extension of rule, it's not fixing any bugs. Could you elaborate more on the problem you have? I think you mistakenly think that this issue will fix it

(don't hesitate to create a new issue)

[christophercr]

Ok, so the problem I have when upgrading to v1.7 is that I get some errors due to the "no-identical-functions" rule.

As an example, the following code causes such violation:

public get code(): string {
    if (this.isoCode.length === 5) {
        return this.isoCode.substr(0, 2);
    } else {
        return "";
    }
}

// TSLint fails here with the message: `Update or refactor this function so that its implementation doesn't duplicate the one on line 26. (no-identical-functions)`
// Where line 26 is the line containing `public get code(): string`
public get region(): string {
    if (this.isoCode.length === 10) {
        return this.isoCode.substr(3, 4);
    } else {
        return "someValue";
    }
}

So SonarTS identifies these 2 blocks as duplicates although the numerical values, the string values and the parameters of the functions is not the same.

Is this Github issue related to the problem I just describe?

[vilchik-elena]

Nope, this issue is not related to your problem:) read it carefully.

The "problem" you have is due to improvement to the rule #599 (it's a feature, not bug :D). Strictly speaking you indeed have a duplication, these functions doing the same thing. But then comes the question, does it make sense to refactor the code to avoid duplication of logic. I can see reasoning for both doing and not doing it (functions are smallish but doing the same thing!). I would suggest to disable such issues or even consider refactoring. WDYT?

[christophercr]

Indeed, as you mentioned, there is some duplication in my example but in my opinion in these cases it is not worth the effort to refactor them because it will not have any added value and it will even add a level of indirection to the code that will make it a bit more difficult to understand.

For example, let's continue with the code I mentioned above, so the refactoring needed to avoid the duplication will be something like this right?:

public someGenericFunction(param1, param2, param3, returnValueElse): any {
   if (this.isoCode.length === param1) {
        return this.isoCode.substr(param2, param3);
    } else {
        return returnValueElse;
    }
}

// so the methods can be "simplified"
public get code(): string {
   return someGenericFunction(5, 0, 2, "");
}

public get region(): string {
   return someGenericFunction(10, 3, 4, "someValue");
}

As I said, this will make the code a bit harder to understand and it is not flexible. What if we need to add another parameter or a return value? We would need to add another parameter to the generic function and then such generic function becomes confusing because we cannot tell exactly what it does just by looking at the way we call it (we are merging params and return values).

IMHO I think this rule should not treat all literals and return values the same, it could at least not check the return values to avoid this kind of useless refactoring.

[stas-vilchik]

@christophercr , as @vilchik-elena said, you have two options: refactor the code or disable such issues. If in your case the rule generates too many issues that you don't agree with, just disable the rule.

[christophercr]

I have those 2 options indeed, I just want to let you know what I think could be a good enhancement for this rule, specially when I see that the return values were not taken into account in #596 and #599.

I think the "exceptions" for which one would need to disable a linting rule should not be more common that the actual use cases where it is needed 😉

[stas-vilchik]

Agree with you @christophercr. What I meant is until this issue is resolved, you could disable the rule temporarily and unblock the plugin upgrade. On our side we'll do our best to fix the issue as soon as possible 😉