0
\$\begingroup\$

I want to refactor this code but am wondering how I should start it. I'm not sure this code is totally against clean code or not. I'm thinking to use RxJS or a JavaScript builtin method. I'd like to have some help here to learn how to have clean code or how I can minimize the code to few line or chain multiple method of RxJS or JS to get rid of if conditions.

public loadChart(): void {
    this.chartDataSets = [];
    let allData = this.availableDataSet;
    if(this.selectedAttribute === Dashbaord.All && this.selectedComponent === Dashbaord.All) {
    this.prepareIntialData();
}
if (this.selectedAttribute === Dashbaord.All && this.selectedComponent !== Dashbaord.All) {
    const groupedByComponent = this.pendingActionDataSet.groupByArray(x => x.component);
    allData = groupedByComponent[this.selectedComponent];
}
if (this.selectedAttribute !== Dashbaord.All && this.selectedComponent === Dashbaord.All) {
    allData = this.groupedByAttribute[this.selectedAttribute].flattenArray();//["",""]
    const components = allData.groupByArray(x => x.component);//["key":["values"]]
    this.componentOptions = Mapper.mapToSelectItemWithAll(Object.keys(components)); 
}
if (this.selectedAttribute !== Dashbaord.All && this.selectedComponent !== Dashbaord.All) {
    const allDataByAttribute = this.groupedByAttribute[this.selectedAttribute].flattenArray();
    const groupedByComponent = allDataByAttribute.groupByArray(x => x.component);
    this.componentOptions = Mapper.mapToSelectItemWithAll(Object.keys(groupedByComponent));
    if (!this.componentOptions.filter(x => x.value === this.selectedComponent).length) {
        this.selectedComponent = Dashbaord.All;
        allData = allDataByAttribute;
    } else {
        allData = groupedByComponent[this.selectedComponent];
    }
}
this.prepareChartData(allData);
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Indentation Your current indentation is a bit confusing - the function's opening block's { line is not balanced with the closing block } line. At a glance, it looks like the function is 4 lines long, and like there's an extra } at the very bottom of the snippet. Consider using an IDE which can prettily format code automatically, such as VSCode - it makes things much easier when you and anyone who reads the code can identify each block and its nesting level at a glance.

Spelling Spelling matters in programming - misspellings are a very common cause of bugs. Dashbaord should probably be renamed to Dashboard, rather than using the misspelled word everywhere (which would probably eventually cause a headache or few, especially if other developers have to go over the code).

Prefer const - don't use let unless you must reassign the variable name. Using const makes code more readable when you know there's no chance of the variable name being reassigned. Here, rather than reassigning the allData variable name, you may refactor to const by calling prepareChartData immediately, rather than reassign allData. For example:

if (this.selectedAttribute === Dashbaord.All && this.selectedComponent !== Dashbaord.All) {
    const groupedByComponent = this.pendingActionDataSet.groupByArray(x => x.component);
    allData = groupedByComponent[this.selectedComponent];
}

can turn into

if (this.selectedAttribute === Dashbaord.All && this.selectedComponent !== Dashbaord.All) {
    const groupedByComponent = this.pendingActionDataSet.groupByArray(x => x.component);
    this.prepareChartData(groupedByComponent[this.selectedComponent]);
}

If you don't like repeating this.prepareChartData everywhere, you could make it a bit shorter with:

// replace Data with the appropriate type below:
const prepareData = (data: Data) => {
  this.prepareChartData(data);
};

Comparisons As for your if statements, you can save the two comparisons being done in variables first, then use those variables:

const { All } = Dashboard; // note spelling
const attribIsAll = this.selectedAttribute === All;
const selectedIsAll = this.selectedComponent === All;
if (attribIsAll && selectedIsAll) {
  // ...
} else if (attribIsAll && !selectedIsAll) {
  // ...

Another option would be to group one of the truthy comparisons together, like so:

if (attribIsAll) {
  if (selectedIsAll) {
    this.prepareIntialData();
  } else {
    const groupedByComponent = this.pendingActionDataSet.groupByArray(x => x.component);
    prepareData(groupedByComponent[this.selectedComponent]);
  }
  return; // return early; avoid dense indentation later
}
if (selectedIsAll) {
  // ...
} else {
  // ...
}

Array test You have

if (!this.componentOptions.filter(x => x.value === this.selectedComponent).length) {

In general, if you want to check that no elements in an array satisfy a condition, it would be more semantically appropriate to use .some or .every:

const componentOptionSelected = this.componentOptions.some(
  option => option.value === this.selectedComponent
);
if (componentOptionSelected) {
  prepareData(groupedByComponent[this.selectedComponent]);
} else {
  this.selectedComponent = Dashboard.All; // note spelling
  prepareData(allDataByAttribute);
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.