4
\$\begingroup\$

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
});
\$\endgroup\$
  • \$\begingroup\$ Where are your own tests? If they're looking for a TDD approach, it's customary to supply the tests used during your test driven development. \$\endgroup\$ – Mast Aug 22 at 17:09
  • 1
    \$\begingroup\$ I have not added tests to this post here to make the post simple and short. I have tests written somewhere else. Let me add these here if they help. \$\endgroup\$ – SamFast Aug 22 at 18:57
3
\$\begingroup\$

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

  1. 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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks. I will dry run your solution. It looks simpler than mine. I used the maps and filters as interviewer had asked me to demonstrate your knowledge of modern programming language structs too. \$\endgroup\$ – SamFast Aug 23 at 2:42
  • \$\begingroup\$ I worked on this with few variable name changes. It works like a charm. You are right. I just over-engineered it. When they said, you need to use modern programming language features, my just shifted on using them instead of thinking of simple solution for the problem. How stupid I became!! \$\endgroup\$ – SamFast Aug 23 at 3:46
  • \$\begingroup\$ I don't know of course what they meant by "modern programming language features" , that could mean almost anything. But I think shortness, easy to read and maintainable code is a higher priority in any multi-developer commercial team, than modern features used for the sake of being modern. One modern feature might be using "forEach" rather than the older style for loop where you do for (x = 0 ; x < array.size; x++) , where forEach certainly is more readable and maintainable as well as being modern. \$\endgroup\$ – user985366 Aug 23 at 23:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.