Computing the sum of all measures for a certain group type

Question

I have a method that receives 2 params:

• first one is an array of objects of type { group1: string, group2: string, measure: number, ... }
• second one is a string

I also have an enum GroupColumns

enum GroupColumns {
  ColumnTypeX = 'Column Type X',
  ColumnTypeY = 'Column Type Y',
  ColumnTypeZ = 'Column Type Z'
}

This method computes the sum of all the measures for a certain group type when the second parameter matches the second group and the first group is equal to any group found in GroupColumns enum.

    private ComputeSpecificColumnMeasure(data: Array<IDynamic<any>>, groupType: String): number {
        let measure: number = 0;
        data.forEach(el => {
            if (el.group2 === groupType && (el.group1 === GroupColumns.ColumnTypeX || el.group1 === GroupColumns.ColumnTypeY || el.group1 === GroupColumns.ColumnTypeZ )) {
                measure += el.measure;
            }
        });

        return measure;
    }

I do not like the verifications inside the if. If I somehow want to add more columns(unlikely) I would need to expand it, or create another method only to check if group1 is equal to any GroupColumns.

Any opinions?

How can I make this code scalable and optimized

Answer 1

If possible, I'd change the GroupColumns to be an array instead. That way, all you need to do is use .includes to see if an item is included in it. Other improvements can be:

Let TS automatically infer types when possible. You only need to denote the type of a variable or the type of a return value when TS can't do so on its own. TS can automatically determine that ComputeSpecificColumnMeasure returns a number. It can also determine that measure will be a number, so there's no need to denote the type of either of those.

Avoid String type The "String" type is something created via the String constructor, eg const str = new String('data');. This is almost never what you want - for a normal string, use the string (primitive) type instead. (TSLint rule: ban-types)

Avoid any You have data: Array<IDynamic<any>>. any should be avoided in TypeScript whenever possible, because it's so flexible - it loses type safety. If you don't know what the type will be, use unknown instead, which is type-safe.

For a small example of a problem with any:

const fn = (arg: any) => {
    console.log(arg.toFixed(2));
}

This will not throw a TS error, but it's likely to throw a runtime error unless the argument passed happens to be a number. Using unknown instead will throw a TS error - it'll force you to narrow the type first before calling a particular method on it.

const fn = (arg: unknown) => {
    if (typeof arg === 'number') {
        console.log(arg.toFixed(2));
    }
}

Even if the value that's any isn't being used in the function, it'd good practice to avoid any when possible anyway. (TSLint rule: no-unsafe-any)

Capitalization Conventionally, only a few things use PascalCase in JS:

• Namespaces (eg React)
• Classes
• Enums, sometimes

A plain method should probably use the standard camelCase instead.

const groupColumns = [
    'Column Type X',
    'Column Type Y',
    'Column Type Z'
] as const; // Use "as const" to prevent automatic widening to `string[]`

private computeSpecificColumnMeasure(data: Array<IDynamic<unknown>>, groupType: string) {
    let measure = 0;
    data.forEach(el => {
        if (el.group2 === groupType && groupColumns.includes(el.group1)) {
            measure += el.measure;
        }
    });
    return measure;
}

Or, you could use .filter and reduce if you wished:

private computeSpecificColumnMeasure(data: Array<IDynamic<unknown>>, groupType: string) {
    return data
        .filter(el => el.group2 === groupType && groupColumns.includes(el.group1))
        .reduce((measureSoFar, el) => measureSoFar + el.measure, 0);
}