Skip to main content
added 702 characters in body
Source Link

Side-effects? The result of the whole function is to set a value (to this.outputArray). Is this required? Code is generally easiest to understand and reason about when it does not contain side-effects. Imagine seeing some code you haven't looked at before, and compare:

this.calculate();
console.log('Result:', this.result);

vs

console.log('Result:', this.calculate());

The second is easier to make sense of because the method is used for its return value instead of for its side-effects (which, to understand, you'd have to read through the method to find).

Could your filterElements method return a value instead of assigning to this.outputArray?

Side-effects? The result of the whole function is to set a value (to this.outputArray). Is this required? Code is generally easiest to understand and reason about when it does not contain side-effects. Imagine seeing some code you haven't looked at before, and compare:

this.calculate();
console.log('Result:', this.result);

vs

console.log('Result:', this.calculate());

The second is easier to make sense of because the method is used for its return value instead of for its side-effects (which, to understand, you'd have to read through the method to find).

Could your filterElements method return a value instead of assigning to this.outputArray?

added 2 characters in body
Source Link

It looks like you're using TypeScript, so a couple pointers:

Avoid any - any effectively disables type-checking, which defeats the whole point of using TypeScript. Better to type variables properly:

let filtered: Array<{ name: string; condition_matrix: Array<number>; }> = [];

Or, even better, let TypeScript infer the types automatically when possible, by transforming the data all at once with array methods (see below). No need to denote types when TS can infer them automatically - eg let isPossibility: boolean = true; can be just let isPossibility = true;

**Avoid let, prefer constAvoid let, prefer const - see here, using const indicates that the variable will never be reassigned, which reduces cognitive overhead.

Semantic array methods While for loops work, they should only be used as a last resort, when other array methods aren't appropriate. Here, rather than breaking inside a for loop to check if an item passes a condition, it would be better to use .some. Also, rather than .pushing to an outer array based on the condition, use .filter instead:

this.outputArray = originalArray.filter(
  item => this.selectedConditions.some(
    condition => item.condition_matrix.includes(condition)
  )
);

(Better to use .includes than to use an indexOf check against 0)

Or:

Time complexity If performance is an issue and you have a lot of conditions, rather than iterating all of the conditions each time, consider constructing a Set of all of the selectedConditions first; Set#has is O(1), which is an improvement over iterating over arrays with O(n).

const allConditions = new Set(this.selectedConditions);

Conditions list typo? The data structure in your code is an array of objects, but you're using item.condition_matrix.indexOf( this.selectedConditions[i] ). Did you mean:

item.condition_matrix.indexOf( this.selectedConditions[i].id )

?

(If so, and you want to use the Set method, map to IDs first:

const allConditions = new Set(
  this.selectedConditions.map(({ id ]) => id)
);

)

It looks like you're using TypeScript, so a couple pointers:

Avoid any - any effectively disables type-checking, which defeats the whole point of using TypeScript. Better to type variables properly:

let filtered: Array<{ name: string; condition_matrix: Array<number>; }> = [];

Or, even better, let TypeScript infer the types automatically when possible, by transforming the data all at once with array methods (see below). No need to denote types when TS can infer them automatically - eg let isPossibility: boolean = true; can be just let isPossibility = true;

**Avoid let, prefer const - see here, using const indicates that the variable will never be reassigned, which reduces cognitive overhead.

Semantic array methods While for loops work, they should only be used as a last resort, when other array methods aren't appropriate. Here, rather than breaking inside a for loop to check if an item passes a condition, it would be better to use .some. Also, rather than .pushing to an outer array based on the condition, use .filter instead:

this.outputArray = originalArray.filter(
  item => this.selectedConditions.some(
    condition => item.condition_matrix.includes(condition)
  )
);

(Better to use .includes than to use an indexOf check against 0)

Or:

Time complexity If performance is an issue and you have a lot of conditions, rather than iterating all of the conditions each time, consider constructing a Set of all of the selectedConditions first; Set#has is O(1), which is an improvement over iterating over arrays with O(n).

const allConditions = new Set(this.selectedConditions);

Conditions list typo? The data structure in your code is an array of objects, but you're using item.condition_matrix.indexOf( this.selectedConditions[i] ). Did you mean:

item.condition_matrix.indexOf( this.selectedConditions[i].id )

?

(If so, and you want to use the Set method, map to IDs first:

const allConditions = new Set(
  this.selectedConditions.map(({ id ]) => id)
);

)

It looks like you're using TypeScript, so a couple pointers:

Avoid any - any effectively disables type-checking, which defeats the whole point of using TypeScript. Better to type variables properly:

let filtered: Array<{ name: string; condition_matrix: Array<number>; }> = [];

Or, even better, let TypeScript infer the types automatically when possible, by transforming the data all at once with array methods (see below). No need to denote types when TS can infer them automatically - eg let isPossibility: boolean = true; can be just let isPossibility = true;

Avoid let, prefer const - see here, using const indicates that the variable will never be reassigned, which reduces cognitive overhead.

Semantic array methods While for loops work, they should only be used as a last resort, when other array methods aren't appropriate. Here, rather than breaking inside a for loop to check if an item passes a condition, it would be better to use .some. Also, rather than .pushing to an outer array based on the condition, use .filter instead:

this.outputArray = originalArray.filter(
  item => this.selectedConditions.some(
    condition => item.condition_matrix.includes(condition)
  )
);

(Better to use .includes than to use an indexOf check against 0)

Or:

Time complexity If performance is an issue and you have a lot of conditions, rather than iterating all of the conditions each time, consider constructing a Set of all of the selectedConditions first; Set#has is O(1), which is an improvement over iterating over arrays with O(n).

const allConditions = new Set(this.selectedConditions);

Conditions list typo? The data structure in your code is an array of objects, but you're using item.condition_matrix.indexOf( this.selectedConditions[i] ). Did you mean:

item.condition_matrix.indexOf( this.selectedConditions[i].id )

?

(If so, and you want to use the Set method, map to IDs first:

const allConditions = new Set(
  this.selectedConditions.map(({ id ]) => id)
);

)

Source Link

It looks like you're using TypeScript, so a couple pointers:

Avoid any - any effectively disables type-checking, which defeats the whole point of using TypeScript. Better to type variables properly:

let filtered: Array<{ name: string; condition_matrix: Array<number>; }> = [];

Or, even better, let TypeScript infer the types automatically when possible, by transforming the data all at once with array methods (see below). No need to denote types when TS can infer them automatically - eg let isPossibility: boolean = true; can be just let isPossibility = true;

**Avoid let, prefer const - see here, using const indicates that the variable will never be reassigned, which reduces cognitive overhead.

Semantic array methods While for loops work, they should only be used as a last resort, when other array methods aren't appropriate. Here, rather than breaking inside a for loop to check if an item passes a condition, it would be better to use .some. Also, rather than .pushing to an outer array based on the condition, use .filter instead:

this.outputArray = originalArray.filter(
  item => this.selectedConditions.some(
    condition => item.condition_matrix.includes(condition)
  )
);

(Better to use .includes than to use an indexOf check against 0)

Or:

Time complexity If performance is an issue and you have a lot of conditions, rather than iterating all of the conditions each time, consider constructing a Set of all of the selectedConditions first; Set#has is O(1), which is an improvement over iterating over arrays with O(n).

const allConditions = new Set(this.selectedConditions);

Conditions list typo? The data structure in your code is an array of objects, but you're using item.condition_matrix.indexOf( this.selectedConditions[i] ). Did you mean:

item.condition_matrix.indexOf( this.selectedConditions[i].id )

?

(If so, and you want to use the Set method, map to IDs first:

const allConditions = new Set(
  this.selectedConditions.map(({ id ]) => id)
);

)