AG-NONE Cleanup destroy

[olegat]

No description provided.

[alantreadway]

These inline assignment and returns are not very readable, can you adjust the pattern here to avoid this and the need for // NOSONAR?

[olegat]

It is readable.

Sonar argues that assignments in expressions are discouraged is because they can have an unexpected side-effect. For example:

if (isEnabled && x = getValue()) {
  doSomething();
}

In this scenario, it is not clear what the programmer meant:

• Assign x to a value if isEnabled is true?
• Do something if isEnabled is true and x is assigned to a true-y value?
• Or did the programmer mean == / === instead of =?

That can be confusing, because x might be assigned or it might not, and doSomething() might be called or it might not.

However, this argument is not applicable in this case. How can (this.annotationManager = new AnnotationManager(chart.annotationRoot)), // NOSONAR possibly be misunderstood? There no other logical operators like && and || so there is only 1 possible outcome from this expression.

Also, it is abundantly clear the intent is to assign a value and we didn't intend == or === (because this expression is part of ChartContext's constructor and also why would we want to compare an unassigned value to a newly constructed object?).

The aim objective of ObjectDestroyer is to follow a RAII model to ensure that:

1. We do not pass uninitialised variables to the constructors
2. We run the destructors in reverse

The only option to avoid the // NOSONAR could be to use lambdas methods that ObjectDestroyer can call:

  this.owned = new ObjectDestroyer(
    () => this.annotationManager = new AnnotationManager(chart.annotationRoot),
    // ...
);

However, this will violate objective 1). We could end up accidentally creating a circular dependency amongst constructors.

We could also apply the changes that Sonar suggests, which involve putting the assignments as separate expressions

this.annotationManager = new AnnotationManager(chart.annotationRoot);
this.ariaAnnouncementService = new AriaAnnouncementService(this.scene.canvas.element);
this.chartEventManager = new ChartEventManager();
// ...

this.owned = new ObjectDestroyer(
  this.domManager,
  this.annotationManager,
  this.ariaAnnouncementService,
  this.chartEventManager,
  // ..
);

However, this is not a good suggestion because objective 2) is to ensure that destructors are run in reverse. But with this approach we need to manually ensure the ordering is reversed, which is basically what we're doing now. So we might as well not bother adding ObjectDestroyer if we're going with this approach.

It is a pain to have to write // NOSONAR in each line. I would much prefer multi-line enable/disable options but I can't find something like that. Open to other suggestions to disable the Sonar warnings, but Sonar is wrong in this case.

[alantreadway]

Why allow objects without a destroy() - I think that will lead to more cases where a missing destroy() is not flagged up?

[olegat]

ACK. Some of the objects in ChartContext do not have a destructors, but we can add empty destroy() methods to these.

[olegat]

Discussed offline. We've decided to ditch the ObjectDestroyer class