Return to Answer

It is inefficient and overengineered because the code

1. loops through the transactions several times when once would be enough, both by the multiple filters and the two separate loops.

2. uses more memory than needed, partly due to the above and partly due to the creation of the array "filteredTransactions"

3. calls moment() up to twice to convert the timestamp, when once would suffice.

It is less maintainable / understandable by others in the team because

4. some may not commonly use map/filter and most may not commonly use reduce which is particularly complex. (See also any article on KISS)

You are overthinking this with filter and reduce and consuming more memory and cpu than needed in the process. If I got this code I would think that the interviewee was trying to impress me that they know about filter and reduce but the code ends up way longer and more complex than needed.

A simpler version would be

sum = 0
transactions.forEach(t => {
    if (t.category == category) {
        let d = moment(transactionDate)
        if (d.isSameOrAfter(start) && d.isBefore(end)) {
            sum += d.amount
        }
    }
})
return sum

This code is half the lines and almost no intermediate variables.

It does not require the next guy to understand filter, map or reduce.

It does not look at any transaction more than once.

It does not create 3 intermediate lists of transactions / numbers.

See also https://stackoverflow.com/questions/45691907/is-using-several-filter-calls-on-a-big-array-bad-for-performance-in-javascrip

It is inefficient and overengineered because the code

1. loops through the transactions several times when once would be enough, both by the multiple filters and the two separate loops.

2. uses more memory than needed, partly due to the above and partly due to the creation of the array "filteredTransactions"

3. calls moment() up to twice to convert the timestamp, when once would suffice.

It is less maintainable / understandable by others in the team because

4. some may not commonly use map/filter and most may not commonly use reduce which is particularly complex. (See also any article on KISS)

You are overthinking this with filter and reduce and consuming more memory and cpu than needed in the process. If I got this code I would think that the interviewee was trying to impress me that they know about filter and reduce but the code ends up way longer and more complex than needed.

A simpler version would be

sum = 0
transactions.forEach(t => {
    if (t.category == category) {
        let d = moment(transactionDate)
        if (d.isSameOrAfter(start) && d.isBefore(end)) {
            sum += d.amount
        }
    }
})
return sum

This code is half the lines and almost no intermediate variables.

It does not require the next guy to understand filter, map or reduce.

It does not look at any transaction more than once.

It does not create 3 intermediate lists of transactions / numbers.

See also this.

It is inefficient and overengineered because your code is

1. looping through the transactions several times when once would be enough, both by the multiple filters and the two separate loops.

2. using more memory than needed, partly due to the above and partly due to the creation of the array "filteredTransactions"

3. calling moment() up to twice to convert the timestamp, when once would suffice.

It is less maintainable / understandable by others in your team because

4. some may not commonly use map/filter and most may not commonly use reduce which is particularly complex. (See also any article on KISS)

You are overthinking this with filter and reduce and consuming more memory and cpu than you need in the process. If I got this code I would think that you were trying to impress me that you know about filter and reduce but the code you wrote is way longer than needed.

What you could do is sort of

sum = 0
transactions.forEach(t => {
    if (t.category == category) {
        let d = moment(transactionDate)
        if (d.isSameOrAfter(start) && d.isBefore(end)) {
            sum += d.amount
        }
    }
})
return sum

This code is half the lines and almost no intermediate variables.

It does not require the next guy to understand filter, map or reduce.

It does not look at any transaction more than once.

It does not create 3 intermediate lists of transactions / numbers.

See also https://stackoverflow.com/questions/45691907/is-using-several-filter-calls-on-a-big-array-bad-for-performance-in-javascrip

It is inefficient and overengineered because the code

1. loops through the transactions several times when once would be enough, both by the multiple filters and the two separate loops.

2. uses more memory than needed, partly due to the above and partly due to the creation of the array "filteredTransactions"

3. calls moment() up to twice to convert the timestamp, when once would suffice.

It is less maintainable / understandable by others in the team because

4. some may not commonly use map/filter and most may not commonly use reduce which is particularly complex. (See also any article on KISS)

You are overthinking this with filter and reduce and consuming more memory and cpu than needed in the process. If I got this code I would think that the interviewee was trying to impress me that they know about filter and reduce but the code ends up way longer and more complex than needed.

A simpler version would be

sum = 0
transactions.forEach(t => {
    if (t.category == category) {
        let d = moment(transactionDate)
        if (d.isSameOrAfter(start) && d.isBefore(end)) {
            sum += d.amount
        }
    }
})
return sum

This code is half the lines and almost no intermediate variables.

It does not require the next guy to understand filter, map or reduce.

It does not look at any transaction more than once.

It does not create 3 intermediate lists of transactions / numbers.

See also https://stackoverflow.com/questions/45691907/is-using-several-filter-calls-on-a-big-array-bad-for-performance-in-javascrip

It is inefficient and overengineered because you are

1. looping through the transactions several times when once would be enough, both by your multiple filters and your two separate loops.

2. using more memory than needed, partly due to the above and partly due to the creation of the array "filteredTransactions"

3. You are calling moment() up to twice to convert the timestamp, when once would suffice.

It is less maintainable / understandable by others in your team because

4. some may not commonly use map/filter and most may not commonly use reduce which is particularly complex.

You are overthinking this with filter and reduce and consuming more memory and cpu than you need in the process. If I got this code I would think that you were trying to impress me that you know about filter and reduce but the code you wrote is way longer than needed.

What you could do is sort of

sum = 0
transactions.forEach(t => {
    if (t.category == category) {
        let d = moment(transactionDate)
        if (d.isSameOrAfter(start) && d.isBefore(end)) {
            sum += d.amount
        }
    }
})
return sum

This code is half the lines and almost no intermediate variables.

It does not require the next guy to understand filter, map or reduce.

It does not look at any transaction more than once.

It does not create 3 intermediate lists of transactions / numbers.

See also https://stackoverflow.com/questions/45691907/is-using-several-filter-calls-on-a-big-array-bad-for-performance-in-javascrip

It is inefficient and overengineered because your code is

1. looping through the transactions several times when once would be enough, both by the multiple filters and the two separate loops.

2. using more memory than needed, partly due to the above and partly due to the creation of the array "filteredTransactions"

3. calling moment() up to twice to convert the timestamp, when once would suffice.

It is less maintainable / understandable by others in your team because

4. some may not commonly use map/filter and most may not commonly use reduce which is particularly complex. (See also any article on KISS)

You are overthinking this with filter and reduce and consuming more memory and cpu than you need in the process. If I got this code I would think that you were trying to impress me that you know about filter and reduce but the code you wrote is way longer than needed.

What you could do is sort of

sum = 0
transactions.forEach(t => {
    if (t.category == category) {
        let d = moment(transactionDate)
        if (d.isSameOrAfter(start) && d.isBefore(end)) {
            sum += d.amount
        }
    }
})
return sum

This code is half the lines and almost no intermediate variables.

It does not require the next guy to understand filter, map or reduce.

It does not look at any transaction more than once.

It does not create 3 intermediate lists of transactions / numbers.

See also https://stackoverflow.com/questions/45691907/is-using-several-filter-calls-on-a-big-array-bad-for-performance-in-javascrip