Sonar smell typescript:S6647 Unnecessary constructors should be removed

Clean Code Report False-positive / False-negative...
,
1

Hello all, I was wondering if this rule could be visited. typescript:S6647 Unnecessary constructors should be removed.

We’re using Enterprise Edition Version 10.2.1 (build 78527).

The rule itself, seems to be have introduced on the 5th of September.

“If the class declaration does not include a constructor, one is automatically created, so there is no need to provide an empty constructor, or one that just delegates to the parent class.”

Where the above is true when extending from a class in typescript, the above is not true when extending from an abstract class in typescript. (please someone correct me if I am wrong)

The smell keeps appearing for classes extended from abstract classes in our projects.

For the time being I have removed this rule from the projects/ resolved them as false positives, but I just wondered if the rule itself could be altered to check if the extending class is from an abstract class.

1 Like
3

We regularly extend TypeScript abstract classes with no dedicated constructor with no issues.
Note that abstract classes are not a JavaScript concept but just a TypeScript one: they have no special runtime behaviour and the abstract keyword is removed.

1 Like
4

Can you give me an example of where you would use a class extended from an abstract class without a constructor?

You cant initialize an abstract class, only classes extended from it.

The whole concept is to have a base template and extend from there and expand the params.

Abstract classes are not really a typescript concept its used in other languages.

Here’s the example from the docs:

abstract class Person {
    abstract name: string;

    display(): string{
        return this.name;
    }
}

class Employee extends Person {
    name: string;
    empCode: number;

    constructor(name: string, code: number) {
        super(); // must call super()

        this.empCode = code;
        this.name = name;
    }
}

The constructor and super inside “Employee” would be picked up in a sonar smell. But its required, because its an extension of an abstract class, which has an additional param.

5

I meant this works, following your example:

abstract class Person {
    abstract name: string;

    display(): string{
        return this.name;
    }
}

// This works
class User extends Person {
    name: string;
}

// This too
let u = new User();

Of course the name field must be initialized elsewhere.

A code conceptually similar to our application code (we use Aurelia 1 with dependency injection)

abstract class BasePage {
    constructor(protected service: SomeService) { }
}

class SomePage extends BasePage {
    someParameter: string;
    // no need to add a constructor here, unless parameters are added
}

// this works
let page = new SomePage(getServiceSomehow());

Again, this works if the parameters are kept the same (unlike C#, for example). If parameters are added and expected at construction time of course a constructor is required.

6

That’s my point, sonar is smelling them as useless constructors, but they are required as the params are different…

7

Then I misunderstood your report, my apologies for this detour and let the discussion continue.

9

Hi @jamie.b,

thanks for reporting this. Can you please provide a reproducer? With the code sample you provided here, I cannot see any issue raised:

Thanks,
Victor

11

Hello, we have been able to reproduce the issue after another user reported the same behavior here.

Proceed to that post for future updates.

Cheers,
Victor

1 Like