Interview Coding Test: Transaction Processing: Get balance by category in given period

Question

I recently gave a coding test for a job which failed. It had two problems and one of them is shared in this question along with my solution.

Problem: Get balance of all transactions in a given category within specified period.

This is how a typical transaction looks like:

{
  id: 123,
  sourceAccount: 'my_account',
  targetAccount: 'grocery_shop',
  amount: -30,
  category: 'groceries',
  time: '2018-03-12T12:34:00Z'
}

Negative value for amount means amount has been spent in that transaction.

Input

1. transactions an array of transactions.
2. category it can be eating_out, groceries or any other type
3. start it is the start date (inclusive)
4. end it is the end date. (exclusive)

General Requirements

Here is what they said they are looking for in my solution:

This is a coding challenge which tests your coding abilities and to make sure you can present us with well written, well tested and not over-engineered code. We're looking for a well structured, tested, simple solution. As mentioned before the engineering teams work in a TDD environment and code is driven by the testing methodology as we are deploying code on a daily basis. It's a very collaborative environment so there is a lot of pair and mob programming happening which is why the code that is written needs to be able to be understood by others in your team.

My Solution

const moment = require('moment')

function getBalanceByCategoryInPeriod(transactions = [], category, start, end) {
  let balance = 0

  if (moment(start).isSame(end)) {
    end = moment(end).endOf('day')
  }

  // Filters the transactions based on category and given duration
  let filteredTransactions = transactions
    .filter(transaction => transaction.category === category)
    .filter(transaction => {
      let transactionDate = transaction.time
      return (moment(transactionDate).isSameOrAfter(start) &&
              moment(transactionDate).isBefore(end))
  })

  // Calculates the balance
  balance = filteredTransactions
    .map(transaction => transaction.amount)
    .reduce((total, amount) => total + amount, 0)

  return balance
}

All the unit tests for this function pass. The summarized feedback of my both solutions was that the code is inefficient.

Here is the detailed feedback to me by those who reviewed my submitted code:

• Solutions are overall inefficient. Naming convention is good the idea behind some unitary functions is well intended but badly executed. (negative feedback)
• Easy to Maintain, (positive feedback)
• Easy to Read, (positive feedback)
• Advanced Grasp of Language (positive feedback)
• Inefficient Solution (negative feedback)
• There is actually no need to have two loops for this exercise. But it's well structured and readable. Since it's doing two different things a better result of this approach would have been to have two different functions. (General Feedback)

I am understanding part of the feedback. It is overall feedback to both coding problems and I have only presented one of them here. I am not sure to which one the feedback applies. I will publish the second problem in next question.

Please let me know what else can be improved in my code and how can I make it efficient. Please give me your complete feedback regarding efficiency, performance etc.

Thank you.

EDIT

Here are my tests as requested in comments:

const assert = require("chai").assert;

describe("getBalanceByCategoryInPeriod()", function() {
  it("returns 0 if there are no transactions", function() {
    assert.equal(
      getBalanceByCategoryInPeriod(
        [],
        "groceries",
        new Date("2018-03-01"),
        new Date("2018-03-31")
      ),
      0
    );
  });
  
  it("returns 0 if there are no transactions for given category", function() {
    assert.equal(
      getBalanceByCategoryInPeriod(
        [{
          id: 123,
          sourceAccount: 'my_account',
          targetAccount: 'coffee_shop',
          amount: -30,
          category: 'eating_out',
          time: '2018-03-12T12:34:00Z'
        }],
        "groceries",
        new Date("2018-03-01"),
        new Date("2018-03-31")
      ),
      0
    );
  });
  
  it("returns 0 if there are no transactions within specified time period", function() {
    assert.equal(
      getBalanceByCategoryInPeriod(
        [{
          id: 123,
          sourceAccount: 'my_account',
          targetAccount: 'coffee_shop',
          amount: -30,
          category: 'groceries',
          time: '2018-02-12T12:34:00Z'
        }],
        "groceries",
        new Date("2018-03-01"),
        new Date("2018-03-31")
      ),
      0
    );
  });
  
  it("returns correct balance if specified period is just one day", function() {
    assert.equal(
      getBalanceByCategoryInPeriod(
        [{
          id: 123,
          sourceAccount: 'my_account',
          targetAccount: 'grocery_shop',
          amount: -30,
          category: 'groceries',
          time: '2018-03-12T12:34:00Z'
        },
        {
          id: 124,
          sourceAccount: 'my_account',
          targetAccount: 'coffee_shop',
          amount: -40,
          category: 'groceries',
          time: '2018-03-12T12:44:00Z'
        }],
        "groceries",
        new Date("2018-03-12"),
        new Date("2018-03-12")
      ),
      -70
    );
  });
  
  it("returns correct balance and exlude the end date day as it is exlusive", function() {
    assert.equal(
      getBalanceByCategoryInPeriod(
        [{
          id: 123,
          sourceAccount: 'my_account',
          targetAccount: 'grocery_shop',
          amount: -40,
          category: 'groceries',
          time: '2018-03-12T12:34:00Z'
        },
        {
          id: 124,
          sourceAccount: 'my_account',
          targetAccount: 'coffee_shop',
          amount: -40,
          category: 'groceries',
          time: '2018-03-16T12:44:00Z'
        },
        {
          id: 125,
          sourceAccount: 'my_account',
          targetAccount: 'coffee_shop',
          amount: -40,
          category: 'eating_out',
          time: '2018-03-17T12:44:00Z'
        },
        {
          id: 126,
          sourceAccount: 'my_account',
          targetAccount: 'coffee_shop',
          amount: -40,
          category: 'groceries',
          time: '2018-03-30T12:44:00Z'
        }],
        "groceries",
        new Date("2018-03-01"),
        new Date("2018-03-30")
      ),
      -80
    );
  });

  // add your tests here
});

Answer 1

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.