How to refactor a condition into a polymorph object?-CodePudding

i have a question about a refactor from condition to polymorph object. I can't understand how I can generalize a condition into a polymorphic object by making it always return the correct value of the data type based on a type.

I have tried with an abstract class only I don't know how to proceed. Sorry if the question is wrong or trivial, but I don't know how to do it.

my polymorph-model.ts

import { ViewerDataType, XMLImagesValues } from "./models";

export abstract class ViewerDataSource {

    constructor(
        public _type: ViewerDataType
    ){}

    abstract getSource(): string | XMLImagesValues[];
}

export class ManifestSource extends ViewerDataSource {
    getSource(): string {
        return this._type.value.manifestURL
    }
}

export class XMLSource extends ViewerDataSource {
    getSource(): XMLImagesValues[] {
        return this._type.value.xmlImages
    }
}

a snippet of my component.ts for the refactor. Here i need to change the two condition in the @Input and in the ngAfterViewInit

//...
public _viewerDataType: string; // tslint:disable-line: variable-name
public _manifestPath: string; // tslint:disable-line: variable-name
public _xmlImagesPath: XMLImagesValues[]; // tslint:disable-line: variable-name
@Input() set data(v: ViewerDataType) {
    // TODO: need to be refactored with polymorph
    if (v.value.manifestURL !== this._manifestPath) {
      this._manifestPath = v.value.manifestURL;
      this.manifestChange.next(this._manifestPath);
    }
    if (v.value.xmlImages !== this._xmlImagesPath) {
      this._xmlImagesPath = v.value.xmlImages;
      this.xmlImageChange.next(this._xmlImagesPath);
    }
    this._viewerDataType = v.type;
  }
  manifestChange = new BehaviorSubject<string>('');
  xmlImageChange = new BehaviorSubject<XMLImagesValues[]>([]);

  ngAfterViewInit() {
    // TODO: need to be refactored with polymorph
    if (this._viewerDataType === 'manifest'){
      this.tileSources = this.manifestTileSources;
    }else{
      this.tileSources = this.xmlImageTileSources;
    }
  }
//...

Thanks a lot to those who will answer.

CodePudding user response：

In order to take advantage of the abstraction you've created, you first need to change the accepted data type in the set data(...) method to be of type ViewerDataSource, instead of ViewerDataType:

@Input() set data(v: ViewerDataSource) { }

Doing this will allow you to check exactly which concrete implementation you are getting, so that you can make sure that you assign the .getSource() value to the appropriate class property. This is possible through JavaScript's instanceof operator:

@Input() set data(v: ViewerDataSource) {
  // you can now call .getSource() to retrieve the source
  const source: string | XMLImagesValues[] = v.getSource();

  // if the input value that comes is of type ManifestSource, assign the value 
  // returned by .getSource() to _manifestPath; if the value is of type XMLSource, 
  // assign the .getSource() value to _xmlImagesPath
  if (v instanceof ManifestSource) {
    this._manifestPath = source as string;
    this.manifestChange.next(this._manifestPath);
  } else if (v instanceof XMLSource) {
    this._xmlImagesPath = source as XMLImagesValues[];
    this.xmlImageChange.next(this._xmlImagesPath);
  }

  // finally, assign the type internally
  this._viewerDataType = v._type;
}

If you do this, your current logic in ngAfterViewInit() will still work, because you are storing the v._type property in _viewerDataType which is again a string union. If you want to make use of instanceof in ngAfterViewInit() again, you will need to store the ViewerDataSource object that comes as a value for the data input, instead of the ViewerDataType string. So for example, you can have a class property in your component that is called:

public _viewerDataSource: ViewerDataSource;

Then, in the set data(...) method you can use it to store the value that comes:

public _viewerDataSource: ViewerDataSource;
@Input() set data(v: ViewerDataSource) {
  const source: string | XMLImagesValues[] = v.getSource();

  if (v instanceof ManifestSource) {
    this._manifestPath = source as string;
    this.manifestChange.next(this._manifestPath);
  } else if (v instanceof XMLSource) {
    this._xmlImagesPath = source as XMLImagesValues[];
    this.xmlImageChange.next(this._xmlImagesPath);
  }

  // note that we are now assigning the object itself
  this._viewerDataSource = v;
}

And finally, in ngAfterViewInit() you can alter the logic to use instanceof against this._viewerDataSource:

ngAfterViewInit() {
  if (this._viewerDataSource instanceof ManifestSource) {
    this.tileSources = this.manifestTileSources;
  } else {
    this.tileSources = this.xmlImageTileSources;
  }
}

CodePudding user response：

I would like to point out that ardentia's answer would work, and I don't think a much better approach is possible. Despite that, I don't like the if statements in there, but they cannot be avoided. I will explain myself below.

Abstract classes

Abstract classes are meant to abstract away the implementation details of its concrete implementations. Therefore, a component using the abstract class does not need to know how a concrete implementation works, it just needs to know that it provides an interface (abstract classes and interfaces are therefore very similar.)

In your code, you do depend on the exact implementation of the source in your component, because you want to emit on manifestChange() when a manifest source changes, similarly for xmlImageChange. Therefore you cannot abstract away the implementation details; you need to know how a XMLSource works and how a ManifestSource works. An abstract class is not suitable here.

What can you do about it?

Make sure the component does not need to know the implementation details of a ViewDataSource. Example component:

public _viewerDataType: string; // tslint:disable-line: variable-name
public _manifestPath: string; // tslint:disable-line: variable-name
public _xmlImagesPath: XMLImagesValues[]; // tslint:disable-line: variable-name
@Input() set data(v: ViewerDataSource) {
    this.sourceChange.next(v.getSource());
  }
  sourceChange = new BehaviorSubject<string | XMLImagesValues[]>('');

  async ngAfterViewInit() {
      this.tileSources = await this.sourceChange.pipe(take(1)).toPromise()
  }

You can pass both ManifestSource and XMLSource as an input.

But now you don't know if the type is a Manifest or a XML! So we just moved the problem, because you probably need to know it somewhere.

Structural changes

I recommend you to make an abstract component and 2 implementations of it:

abstract class AbstractComponent {
@Input() set data(v: ViewerDataSource) {
    this.sourceChange.next(v.getSource());
  }
  abstract sourceChange: BehaviorSubject<any>;
  abstract titleSources: any;

  async ngAfterViewInit() {
      this.tileSources = await this.sourceChange.pipe(take(1)).toPromise()
  }

  // Do whatever is the same for a manifest and XML here. Since you don't know which one you are going to get, let out any stuff that only applies to one of the two!
}

@Component({
   blablabla...
})
class ManifestComponent extends AbstractComponent {
    public manifestPath: string;
    public sourceChanges = new BehaviourSubject<string>('');
    public titleSources: string;

   // Do whatever you need to do with a manifest here. You are sure the type is a manifest in this component, so you do not need to check it!
}

@Component({
   blablabla...
})
class XMLComponent extends AbstractComponent {
    public xmlImagesPath: XMLImagesValues[];;
    public sourceChanges = new BehaviourSubject<XMLImagesValues[]>([]);
    public titleSources: XMLImagesValues[];

   // Do whatever you need to do with XML image values here. You are sure the type XML in this component, so you do not need to check it!
}

Now you can use XMLComponent and ManifestComponent and you do not need any if's to check which type it is, since you already know it!