3
\$\begingroup\$

Sometimes I need to push data to the array, but I have one different key (here I have key color and key name) and another part of the object is the same. I use IF/else to check this, but i wonder is there any better and cleanest way to handle this?

  getData(passedData, dataControl: string) {
    const data = [];

    for (const value of passedData) {
      if (dataControl === 'color') {
        data.push({
          color: value,   ---> diff
          username: this.userForm.value.username,
          test: this.userForm.value.test
        }
        );
        this.userForm.controls['data1'].setValue(data);     ---> diff

      } else {
        data.push({
          name: value,     ---> diff
          username: this.userForm.value.username,
          test: this.userForm.value.test
        }
        );
        this.userForm.controls['data2'].setValue(data);       ---> diff

      }
    }
  }

Thank you

\$\endgroup\$
1
  • \$\begingroup\$ Don't bother testing dataControl at all. An object property is easily made from the value of dataControl : { [dataControl] : passedData.value}. See this SO thread \$\endgroup\$ Commented Jul 14, 2020 at 23:36

2 Answers 2

Reset to default
3
\$\begingroup\$

A few thoughts:

  • First, you might consider whether the name/color property REALLY needs to change as output here. Why return { color: ... } in some cases and { name: ... } in others? This tends to indicate that the return data may not sufficiently be describing itself. Even stranger here is the fact that the caller must pass in the hint dataControl "hint" in order to tell the function to modify the return value. If the calling code is where understanding of "color" vs. "name" exists, then why make this function now have to understand this? Why can the caller not just mutate a returned { name: ... } object into a { color: ... } one if it is really critical to make this distinction in property name. Perhaps your object might be well suited to have properties like username, test, value (instead of name or color) and type (to indicate name vs. color). At least this way to get a consistently formed data objects here (and your if-else goes away).
  • I am struggling to understand why you are looping through the passed data and setting data1 or data2 value repeatedly when, you are basically overwriting this set value X times (where X is the length of the array). Why not just build the array once and then set the value?
  • You really only have two things that are changing when dataControl equals color. You can just use variables and drop the if-else construct altogether. In fact generally-speaking, much of you logic can be put into variables, as these values do not change for each iteration over passed data. Using variables here will also simplify your loop such that perhaps all you really need to do is call map on passedData.
  • Why hard-code your data1 and data2 targets?
  • It looks like this code has TypeScript type hinting, however no TypeScript tag was applied to this question. You should either be consistent with your TypeScript usage (putting type hint for BOTH parameters), or not use it it all.

Putting it all together, might yield something like...

const getData = (passedData, dataControl) {
  let propName = 'name';
  let target = 'data2';

  if (dataControl === 'color') {
    propName = 'color';
    target = 'data1';
  }

  const username = this.userForm.value.username;
  const test = this.userForm.value.test;
  const targetEl = this.userForm.controls[target]; 

  const data = passedData.map((value) => {
    return {
      // if you still REALLY need color vs. name prop
      [propName]: value,
      username,
      test
    };
  });

  targetEl.setValue(data);
}
\$\endgroup\$
2
  • \$\begingroup\$ Thnx for your response. Answer on you question Why return { color: ... } in some cases and { name: ... } in others? - this is because I have one multiple select list for user color and second multiple select list for name. And I need create object for every selected name or color like in my example. This is two different arrays of object, but they have similar data structure, that's why I want set everything in one method. I hope you understant... it is hard to explain here because here I have limitations for number of characters \$\endgroup\$ Commented Jul 15, 2020 at 7:36
  • \$\begingroup\$ @Arter You hit on a key point here. These are arrays if similar items. That would even further suggest that perhaps having a “type” indicator (either a property or actual Object class/constructor type) would make it easy for UI components templates to deal with different types in a more generalized manner . What happens when you want to have other types of dropdowns? Are you going to add new property names for each type? \$\endgroup\$ Commented Jul 18, 2020 at 19:26
0
\$\begingroup\$

Maybe this one code will be helpful. Also, with this code you can get rid of data mutation.

getData(parsedValues, dataControl) {
const key = dataControl === 'name' ? 'data2' : 'data1'
const {
    userForm: {
        value: {
            username,
            test
        }
    }
} = this
const data = passedValues.map(value => {
    const result = {
        [dataControl]: value,
        username,
        test
    }
    return result
});

this.userForm.controls[key].setValue(data);
}
\$\endgroup\$
1
  • \$\begingroup\$ Greetings, welcome to CR. Providing answers that are mostly code are frowned upon in this community, hence the downvotes. Please focus on analyzing the code of the OP. \$\endgroup\$ Commented Jul 16, 2020 at 15:14

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.