Return to Revisions

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