CODEWITHSUNDEEP
javascriptangulartypescript
methuselah
methuselah

Reputation: 13206

Possible way to combine to methods that use duplicate code

I have two methods getTargetComponentById and getTargetComponentByName which perform a lookup on an object based on a particular key and returns an object. The two methods work fine seperated, but I would like to combine them to cut down on duplicated code. What would be the best way of combining them:

  getTargetComponentById(componentId: string): IComponentTarget[] {
    const targetComponents = [];
    const website = this.website.getValue();
    for (let i = 0; i < website.pages.length; i++) {
      for (let j = 0; j < website.pages[i].components.length; j++) {
        if (website.pages[i].components[j].componentId === componentId) {
          targetComponents.push({
            activePageIndex: i,
            activeComponentIndex: j,
          });
        }
      }
    }
    return targetComponents;
  }

  getTargetComponentByName(componentName: string, activeWebsite = null): IComponentTarget[] {
    let website: IWebsite;
    if (activeWebsite === null) {
      website = this.website.getValue();
    } else {
      website = activeWebsite;
    }
    const targetComponents = [];
    for (let i = 0; i < website.pages.length; i++) {
      for (let j = 0; j < website.pages[i].components.length; j++) {
        if (website.pages[i].components[j].componentName === componentName) {
          targetComponents.push({
            activePageIndex: i,
            activeComponentIndex: j,
          });
        }
      }
    }
    return targetComponents;
  }

Upvotes: 0

Views: 107

Answers (4)

Sandman
Sandman

Reputation: 1569

You can expose two different interface methods that internally use only one method for logic. I think this is the most elegant way of doing it. If you are using angular (and therefore typescript) you can also enter the two methods as public and the core method as private.

  public getTargetComponentById(id: string): IComponentTarget[] {
    return getTargetComponentBy(id, null, 'componentId');
  }

  public getTargetComponentByName(name: string, activeWebsite = null): IComponentTarget[] {
    return getTargetComponentBy(name, activeWebsite, 'componentName');
  }

  private getTargetComponentBy(componentKey: string, activeWebsite = null, prop: string): IComponentTarget[] {
    let website: IWebsite;
    if (activeWebsite === null) {
      website = this.website.getValue();
    } else {
      website = activeWebsite;
    }
    const targetComponents = [];
    for (let i = 0; i < website.pages.length; i++) {
      for (let j = 0; j < website.pages[i].components.length; j++) {
        const aComp = website.pages[i].components[j];
        if (website.pages[i].components[j][prop] === componentKey) {
          targetComponents.push({
            activePageIndex: i,
            activeComponentIndex: j,
          });
        }
      }
    }
    return targetComponents;
  }

Upvotes: 1

ABCoder
ABCoder

Reputation: 51

You could declare the property to use as an extra input parameter.

getTargetComponentByProperty(property: 'componentName' | 'componentId', value: string, activeWebsite ?= null): IComponentTarget[] {
  const website: IWebsite = activeWebsite ? activeWebsite : this.website.getValue();
  const targetComponents = [];
  for (let i = 0; i < website.pages.length; i++) {
    for (let j = 0; j < website.pages[i].components.length; j++) {
      if (website.pages[i].components[j][property] === value) {
        targetComponents.push({
          activePageIndex: i,
          activeComponentIndex: j,
        });
      }
    }
  }
  return targetComponents;
}

Upvotes: 1

Barremian
Barremian

Reputation: 31125

You could use bracket notation instead of dot notation property accessor to generalize the statement website.pages[i].components[j].componentName === componentName. Try the following

getTargetComponent(component: string, activeWebsite ?= null): IComponentTarget[] {
  const website: IWebsite = activeWebsite ? activeWebsite : this.website.getValue();
  const targetComponents = [];
  for (let i = 0; i < website.pages.length; i++) {
    for (let j = 0; j < website.pages[i].components.length; j++) {
      if (website.pages[i].components[j][component] === component) {        // <-- bracket notation here
        targetComponents.push({
          activePageIndex: i,
          activeComponentIndex: j,
        });
      }
    }
  }
  return targetComponents;
}

Upvotes: 1

Eliseo
Eliseo

Reputation: 57939

I like this

getTargetComponent(arg: string|number, activeWebsite ?= null): IComponentTarget[] {
  const website = activeWebsite || this.website.getValue();
  if (''+arg===arg)
      console.log("arg is a string")
  else
      console.log("arg is a number")

 //well, you can use
 const field=(''+arg===arg)?'componentName':'componentId'
 //and use components[j][field]==arg
}

Upvotes: 1

Related Questions