1
\$\begingroup\$

In an interview I was asked to solve two JavaScript questions. I thought I did pretty well because I:

  • Covered the edge cases
  • Wrote comprehensive tests
  • Documented the code using jsdoc

The interviewer responded with this exact feedback:

Overly Complex Solution, Strange Coding Conventions, Poorly Structured, Hard to Understand, Overly Complex Solution but the first solution did handle edge cases well.

I'm a little confused by the feedback (why include "Overly Complex" twice and "Hard to Understand", which is basically the same thing) - I would appreciate a second set of eyes and more detailed feedback on why this is "Overly Complex."

English is not my first language; please let me know if anything is unclear.

Q1. Given transactions calculate the balance between start-end time for a specific category.

// Sample transactions, if we say find category:eating transactions between 
2020-01-01, 2020-02-25 it should return 400
[{
    id:1,
    amount:100,
    category:'eating',
    sourceAccount: 'A',
    targetAccount: 'B',
    time: '2020-01-02T00:00:00Z'
},
{
    id:2,
    amount:210,
    category:'shopping',
    sourceAccount: 'A',
    targetAccount: 'B',
    time: '2020-01-01T00:00:00Z'
},
{
    id:3,
    amount:300,
    category:'eating',
    sourceAccount: 'A',
    targetAccount: 'B',
    time: '2020-02-02T02:02:00Z'
}]
// My code
/**
 * Transaction object from consumer bank account
 * @typedef {Object} Transaction
 * @property {number} id
 * @property {string} sourceAccount
 * @property {string} targetAccount
 * @property {number} amount
 * @property {string} category
 * @property {string} time
 */

/**
 * Calculate the balance in a specific category within the specified time period.
 * @param {Transaction} [transactions=[]] - Account transactions
 * @param {string} category - Target category
 * @param {object} startTime - Beginning period (Date object)
 * @param {object} endTime - End period (Date object)
 * @returns {number} Calculated balance
 */
function getBalanceByCategoryInPeriod(transactions = [], category, startTime, endTime) {
    const  {
        id:1,
        amount:100,
        category:'eating',
        sourceAccount: 'A',
        targetAccount: 'B',
        time: '2018-03-12T12:33:00Z'
    }= transactions
  if (!isString(category)) throw new Error('Parameter category is not a string');
  if (!isValidDateObject(startTime)) throw new Error('Parameter startTime is not a valid date string');
  if (!isValidDateObject(endTime)) throw new Error('Parameter endTime is not a valid date string');
  if (endTime <= startTime) throw new Error('End time cannot be lower than start time');
  return transactions.reduce((acc, cur) => {
    const errors = isValidTransactionObject(cur, transactionSchema);
    if (errors.length > 0) {
      console.warn(`Transaction properties are not valid, calculating total balance without, ${cur.id}`, errors);
      return acc;
    }
    const targetDate = new Date(cur.time).getTime();
    const isBetweenTimeRange = endTime.getTime() > targetDate && targetDate >= startTime.getTime();
    if (cur.category === category && isBetweenTimeRange) return acc + cur.amount;
    return acc;
  }, 0);
}

/*** UTILS ***/
const isString = value => typeof value === 'string';
const isNumber = value => typeof value === 'number' && !isNaN(value);
const isValidDateString = value => /(\d{4}-[01]\d-[0-3]\dT[0-2]\d:[0-5]\d:[0-5]\d\.\d+)|(\d{4}-[01]\d-[0-3]\dT[0-2]\d:[0-5]\d:[0-5]\d)|(\d{4}-[01]\d-[0-3]\dT[0-2]\d:[0-5]\d)/.test(value);
const isValidDateObject = value => value instanceof Date && !isNaN(value);

const transactionSchema = {
  id: isNumber,
  amount: isNumber,
  sourceAccount: isString,
  targetAccount: isString,
  category: isString,
  time: isValidDateString
};

const isValidTransactionObject = (object = {}, schema) => {
  if (!schema) throw new Error('Schema must be provided to validator function');

  return Object.keys(schema)
    .filter(key => !schema[key](object[key]))
    .map(key => new Error(`${key} is invalid.`));
};

Q2. Given transactions find duplicate transactions (same amount, category, sourceAccount, targetAccount) within one minute period and return them as grouped, grouped arrays should be ordered by first element's id inside of that array. (This one is pretty hard to explain, so just check out input and expected output)

//Sample input 
[
    {
        id: 3,
        sourceAccount: 'A',
        targetAccount: 'B',
        amount: 100,
        category: 'eating_out',
        time: '2018-03-02T10:34:30.000Z'
    },
    {
        id: 5,
        sourceAccount: 'A',
        targetAccount: 'B',
        amount: 100,
        category: 'eating_out',
        time: '2018-03-02T10:33:00.000Z'
    },
    {
        id: 6,
        sourceAccount: 'A',
        targetAccount: 'C',
        amount: 250,
        category: 'other',
        time: '2018-03-02T10:33:05.000Z'
    },
    {
        id: 4,
        sourceAccount: 'A',
        targetAccount: 'B',
        amount: 100,
        category: 'eating_out',
        time: '2018-03-02T10:36:00.000Z'
    },
    {
        id: 2,
        sourceAccount: 'A',
        targetAccount: 'B',
        amount: 100,
        category: 'eating_out',
        time: '2018-03-02T10:33:50.000Z'
    },
    {
        id: 1,
        sourceAccount: 'A',
        targetAccount: 'C',
        amount: 250,
        category: 'other',
        time: '2018-03-02T10:33:00.000Z'
    }
];
//Expected Output
    [
        [
            {
                id: 1,
                sourceAccount: 'A',
                targetAccount: 'C',
                amount: 250,
                category: 'other',
                time: '2018-03-02T10:33:00.000Z'
            },
            {
                id: 6,
                sourceAccount: 'A',
                targetAccount: 'C',
                amount: 250,
                category: 'other',
                time: '2018-03-02T10:33:05.000Z'
            }
        ],
        {
            id: 5,
            sourceAccount: 'A',
            targetAccount: 'B',
            amount: 100,
            category: 'eating_out',
            time: '2018-03-02T10:33:00.000Z'
        },
        {
            id: 2,
            sourceAccount: 'A',
            targetAccount: 'B',
            amount: 100,
            category: 'eating_out',
            time: '2018-03-02T10:33:50.000Z'
        },
        {
            id: 3,
            sourceAccount: 'A',
            targetAccount: 'B',
            amount: 100,
            category: 'eating_out',
            time: '2018-03-02T10:34:30.000Z'
        }
    ]
] 

My code:

/**
 * Transaction object from consumer bank account
 * @typedef {Object} Transaction
 * @property {number} id
 * @property {string} sourceAccount
 * @property {string} targetAccount
 * @property {number} amount
 * @property {string} category
 * @property {string} time
 */

const MUST_MATCH_PROPS = ['sourceAccount', 'targetAccount', 'amount', 'category'];

/**
 * Groups sorted transactions array based on conditions
 * @param {[Transaction]} arr
 * @returns {Array.<Array.<Transaction>>]} - Duplicate Transactions
 */
const groupDuplicateTransactions = arr => {
  const subArrays = [];
  for (let runner = 1, prevDuplicate = false, arrInd = -1; runner < arr.length; runner++) {
    if (isDuplicate(arr[runner], arr[runner - 1])) {
      if (!prevDuplicate) arrInd++;
      if (!subArrays[arrInd]) subArrays[arrInd] = [];
      subArrays[arrInd].push(arr[runner - 1]);
      if (runner === arr.length - 1) subArrays[arrInd].push(arr[runner]);
      prevDuplicate = true;
    } else {
      if (prevDuplicate) subArrays[arrInd].push(arr[runner - 1]);
      prevDuplicate = false;
    }
  }
  return subArrays;
}

/**
 * Finds duplicate transactions in n*log(n)
 * @param {[Transaction]} [transactions=[]] - Account transactions
 * @returns {[Transaction]} - Duplicate Transactions
 */
const findDuplicateTransactions = (transactions = []) => {
  const sortedTransactions = [...transactions] // To ensure immutability. If there is memory constraints I'd prefer mutating
    .filter(transaction => isValidTransactionObject(transaction, transactionSchema).length === 0) // Just to be sure objects are valid transaction objects, objects which are not can be ignored while running algorithm but for the sake of cleanness I have decided to filter out beforehands
    .sort(sorter);
  return groupDuplicateTransactions(sortedTransactions).sort((x, y) => x[0].id - y[0].id);
}

/*** UTILS ***/
const compareFields = (x, y, fields) => fields.every(p => x[p] === y[p]);
const isInOneMinute = (x, y) => Math.abs(new Date(x.time) - new Date(y.time)) < 60000;
const isDuplicate = (x, y) => compareFields(x, y, MUST_MATCH_PROPS) && isInOneMinute(x, y);
const sorter = (a, b) => {
  if (a.sourceAccount === b.sourceAccount) {
    if (a.targetAccount === b.targetAccount) {
      if (a.amount === b.amount) {
        if (a.category === b.category) {
          return b.time < a.time ? 1 : -1;
        }
        return b.category < a.category ? 1 : -1;
      }
      return b.amount < a.amount ? 1 : -1;
    }
    return b.targetAccount < a.targetAccount ? 1 : -1;
  }
  return b.sourceAccount < a.sourceAccount ? 1 : -1;
};
const isString = value => typeof value === 'string';
const isNumber = value => typeof value === 'number' && !isNaN(value);
const isValidDateString = value =>
  /(\d{4}-[01]\d-[0-3]\dT[0-2]\d:[0-5]\d:[0-5]\d\.\d+)|(\d{4}-[01]\d-[0-3]\dT[0-2]\d:[0-5]\d:[0-5]\d)|(\d{4}-[01]\d-[0-3]\dT[0-2]\d:[0-5]\d)/.test(
    value
  );
const isValidDateObject = value => value instanceof Date && !isNaN(value);

const transactionSchema = {
  id: isNumber,
  amount: isNumber,
  sourceAccount: isString,
  targetAccount: isString,
  category: isString,
  time: isValidDateString
};

const isValidTransactionObject = (object = {}, schema) => {
  if (!schema) throw new Error('Schema must be provided to validator function');

  return Object.keys(schema)
    .filter(key => !schema[key](object[key]))
    .map(key => new Error(`${key} is invalid.`));
};
```
\$\endgroup\$
  • 1
    \$\begingroup\$ The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Toby Speight Feb 25 at 11:27
  • 2
    \$\begingroup\$ Welcome to posting on CodeReview@SE. why someone just write Overly Complex 2 times my first thought was that the assessments quoted seem to be from more than one someone. At least until you see reason to assume someone tries to hurt you (how unprofessional!), think non-appreciative. \$\endgroup\$ – greybeard Feb 25 at 11:56
  • 2
    \$\begingroup\$ For me the feedback seems like it was automatically created by tags that the reviewer can click. And for one of the tags ("Overly Complex Solution") it seems the reviewer added a custom explanation (which said something nice), which made it show up a second time. \$\endgroup\$ – KaPy3141 Feb 25 at 12:40
  • 1
    \$\begingroup\$ @KaPy3141: that would explain the conspicuous capitalisation. \$\endgroup\$ – greybeard Feb 25 at 14:07
  • 1
    \$\begingroup\$ @Pod Yes, I feel I can't see things that I am doing wrong. That's why I need \$\endgroup\$ – Batu G. Feb 25 at 15:27
6
\$\begingroup\$

why someone just write Overly Complex 2 times

I don't know what format you originally received this information in, but it looks like there are 4 "tags" applied to your application:

  1. Overly Complex Solution
  2. Strange Coding Conventions
  3. Poorly Structured
  4. Hard to Understand

With someone having modified the first one to include some extra information:

  • Overly Complex Solution but the first solution did handle edge cases well.

Which says to me that multiple people look at your solution and "rate" it, and multiple people thought it was Overly Complex, with one of those people adding extra detail.

why someone just write Overly Complex 2 times and Hard to Understand again which means the same.

I don't know their system, but overly complex and hard to understand are two different things. Here is an pseudo-code example that is overly complex yet easy to understand:

list = [
    -1 + 1,
    0 + 1,
    1 + 1,
    2 + 1,
    2 + 2,
    1 + 4,
    3 + 3,
    1 + 6,
]

It is a pointlessly complex way of saying "[0, 1, 2, 3, 4, 5, 6, 7]", yet I think it's pretty easy to understand?

Therefore they are two separate categories in their system.

I was just waiting for the result and didn't even asked for feedback yet

The feedback is the result :)

English is not my first language but what I understand from feedback is something that can help me to improve also this mail sounds so offensive and destructive. I am not going to ask for detailed feedback because of that reason.

Feedback doesn't have to help you improve, it could simply be an "explanation" of why they rejected you.


In terms of an actual code review, I'll point out that I don't know their system so I'll guess at what they meant for each category:

  1. Overly Complex Solution

This is the hardest for me to explain, because there's no much code here really, and the steps you take don't seem that complex, but then again I don't know what the ideal, "simple" solution is. Your code is very functional and callback-based. Perhaps they were after something more straight forward and procedural?

Perhaps something like this:

const groupDuplicateTransactions = arr => {
  const subArrays = [];
  for (let runner = 1, prevDuplicate = false, arrInd = -1; runner < arr.length; runner++) {
    if (isDuplicate(arr[runner], arr[runner - 1])) {
      if (!prevDuplicate) arrInd++;
      if (!subArrays[arrInd]) subArrays[arrInd] = [];
      subArrays[arrInd].push(arr[runner - 1]);
      if (runner === arr.length - 1) subArrays[arrInd].push(arr[runner]);
      prevDuplicate = true;
    } else {
      if (prevDuplicate) subArrays[arrInd].push(arr[runner - 1]);
      prevDuplicate = false;
    }
  }
  return subArrays;
}

could be expressed differently? e.g. you declare 3 things in the for-initialiser, one called arrInd and one called runner. These are unidiomatic names for such iterating variables. And what's the difference between the two? To figure that out we must deeply inspect the code. We don't want to do that -- we want the code to obviously tell us what these things are.

Perhaps the interviewers felt this method of iterating was too complex for them?

  1. Strange Coding Conventions

This I have to agree with. Coding conventions are usually designed to make code easer to read, so that we can read a lot of it quickly. Your code is incredibly dense and requires a lot of careful reading. I have a hard time even figuring out which line belongs with which other statements. You could have written the exact same thing in a way that doesn't require careful reading.

As an example, let's look at groupDuplicateTransactions again:

const groupDuplicateTransactions = arr => {
  const subArrays = [];
  for (let runner = 1, prevDuplicate = false, arrInd = -1; runner < arr.length; runner++) {
    if (isDuplicate(arr[runner], arr[runner - 1])) {
      if (!prevDuplicate) arrInd++;
      if (!subArrays[arrInd]) subArrays[arrInd] = [];
      subArrays[arrInd].push(arr[runner - 1]);
      if (runner === arr.length - 1) subArrays[arrInd].push(arr[runner]);
      prevDuplicate = true;
    } else {
      if (prevDuplicate) subArrays[arrInd].push(arr[runner - 1]);
      prevDuplicate = false;
    }
  }
  return subArrays;
}

could be:

const groupDuplicateTransactions = (arr) => {
  const subArrays = [];

  for (let runner = 1, prevDuplicate = false, arrInd = -1; runner < arr.length; runner++) {
    if (isDuplicate(arr[runner], arr[runner - 1])) {
      if (!prevDuplicate) arrInd++;
      if (!subArrays[arrInd]) subArrays[arrInd] = [];

      subArrays[arrInd].push(arr[runner - 1]);

      if (runner === arr.length - 1) subArrays[arrInd].push(arr[runner]);

      prevDuplicate = true;
    } else {
      if (prevDuplicate) subArrays[arrInd].push(arr[runner - 1]);

      prevDuplicate = false;
    }
  }
  return subArrays;
}

All I've done here is add a bit of whitespace, and suddenly things are more obvious and quickly-readable.

Most coding standards don't like single-line if-statements. So taking it a step further:

const groupDuplicateTransactions = (arr) => {
  const subArrays = [];

  for (let runner = 1, prevDuplicate = false, arrInd = -1; runner < arr.length; runner++) {
    if (isDuplicate(arr[runner], arr[runner - 1])) {
      if (!prevDuplicate) {
        arrInd++;
      }
      if (!subArrays[arrInd]) { 
        subArrays[arrInd] = [];
      }

      subArrays[arrInd].push(arr[runner - 1]);

      if (runner === arr.length - 1) {
        subArrays[arrInd].push(arr[runner]);
      }
      prevDuplicate = true;
    } else {
      if (prevDuplicate) {
        subArrays[arrInd].push(arr[runner - 1]);
      }
      prevDuplicate = false;
    }
  }
  return subArrays;
}

Additionally, your code is not consistent. The average line is (approx) 20 characters in length, but then you have some whoppers like this:

   .filter(transaction => isValidTransactionObject(transaction, transactionSchema).length === 0) // Just to be sure objects are valid transaction objects, objects which are not can be ignored while running algorithm but for the sake of cleanness I have decided to filter out beforehands

That comment could have easily been a multi-line comment on the line before. Instead you chose to put it all on one line. Why? I imagine most people wouldn't even see the comment as it's way off screen, and they've not had to scroll for any reason before this.

  1. Poorly Structured

I'm not sure how their system differentiates this from 1 or 2, as one person's "Strange Coding Conventions" is another person's "poorly structured".

If they mean the placement of functions within a file, then it seems ok to me... based on the assumption that you were restricted to a single file. Otherwise it would have been best to use multiple files, e.g. "utils.js" for all the stuff under /*** UTILS ***/

  1. Hard to Understand

I think this category is a combination of 1,2,3. But if we are to separate it out then we should remember the maxim that code is read more often than it's written, and your code doesn't appear to be optimised for reading.

e.g.

const sorter = (a, b) => {
  if (a.sourceAccount === b.sourceAccount) {
    if (a.targetAccount === b.targetAccount) {
      if (a.amount === b.amount) {
        if (a.category === b.category) {
          return b.time < a.time ? 1 : -1;
        }
        return b.category < a.category ? 1 : -1;
      }
      return b.amount < a.amount ? 1 : -1;
    }
    return b.targetAccount < a.targetAccount ? 1 : -1;
  }
  return b.sourceAccount < a.sourceAccount ? 1 : -1;
};

vs

const sorter = (a, b) => {
    if (a.sourceAccount !== b.sourceAccount) {
        return b.sourceAccount < a.sourceAccount ? 1 : -1;
    }
    else if (a.targetAccount !== b.targetAccount) {
        return b.targetAccount < a.targetAccount ? 1 : -1;
    }
    else if (a.amount !== b.amount) {
        return b.amount < a.amount ? 1 : -1;
    }
    else if (a.category !== b.category) {
        return b.category < a.category ? 1 : -1;
    }
    else {
        return b.time < a.time ? 1 : -1;
    }
};

The second version has more locality. i.e. you check sourceAccount, then on the next line you use it. No more lines after the first two refer to sourceAccount. Whereas in your version you have to "remember" that we've failed that check then go down 10 lines to the use of the fields.

It also requires less indentation. If you structure had 50 fields, would you indent 50 times?

At the end of the day your interviewers will take the code "as a whole", and if it's too dense and indented for them then they'll just reject it out right. You, as the code-writer, should go out of your way to make it very simple and obvious to them what the code does.

was just waiting for the result and didn't even asked for feedback yet but I got this one which make me feel like garbage after 5-6 years experience. I am not going to ask for detailed feedback because of that reason.

Don't let a single bad interview disheartend you. Remember: A lot of this is subjective. You might like tight, dense code with single line ifs. The interviewers probably didn't. Maybe your next interviewer will?

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Hello Pod, thank you very much for taking time and reviewed the code detailed. After I looked at code assesment platform blog posts I saw this evaluation criterias, bit.ly/2SXcV8u . I agree almost everything you said, most of the things you mentioned I didn't even consider during the task, because I was focused more on how I solve the task efficient way (time complexity). \$\endgroup\$ – Batu G. Feb 25 at 16:36
  • \$\begingroup\$ 1. Overly Complex=I implemented Q2. with more declarative on first run but I realized there are so many redundant operations I am doing, I could simply write in single loop with two pointer (js pointer =) ). So I think that is kind of a wrong assumption. And I couldn't find a way to solve that question more expressive and performant way. Because I declare more than one variable I couldn't come up with meaningful names the other option was using i,j,k var names which personally I found harder to read while I see those answers. But I guess that's kind of personal mistake that I am doing. \$\endgroup\$ – Batu G. Feb 25 at 16:37
  • \$\begingroup\$ 2. Strange coding convention: I totally agree what you are saying I guess I didn't find those deal breaker and focused only the algorithmic solution and testing with every possible case. 3. Yes you are right the solution should be in single file thats why I write same functions in both questions. Just tried to show 'utils' is different. I'd personally extract validators, utility functions and typedefs in different files if it was possible. 4. Yes that's the most stupid thing that I see in this task and felt embarrassed. \$\endgroup\$ – Batu G. Feb 25 at 16:38
  • \$\begingroup\$ Out of interest: What is the name of the assessment platform? And remember, for your next interview, your interviewers are often looking at your code quality as much as they are the correctness. :) \$\endgroup\$ – Pod Feb 25 at 17:06
  • \$\begingroup\$ For the second version of sorter the else keywords can be eliminated because the previous conditional blocks have returns... \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Mar 4 at 21:08
4
\$\begingroup\$

I have little to add to what Pod said.

There is nothing wrong with compact code. Your code could be very efficient, and there is clearly a lot of effort in it, but I agree readability could be improved. Indeed the small details count, even whitespace is important in code.

The important takeaways:

Good code should have a natural flow and should be visually appealing. As an experiment look at the shape of your code standing in front of your screen at a distance of one meter (or 3 feet). Do you like what you see ? It is cliché but you only get one chance to make a first impression, and code leaves an impression too. An interview is highly subjective too. Even a good product needs some dressing-out to sell better.

Just for fun I did a copy-paste in Notepad++ and zoomed out. It looks like this: enter image description here

I have to say I have seen worse. But maybe you will be looking at things through new lenses now.

Next point: avoid all redundancy, that means your checks should take place in a logical order.

I totally agree about the single-line if statements, avoid them. Quite a bad habit, possibly laziness for which I can't find justification. Opening and closing brackets (with proper indentation of course) make the block more readable and less ambiguous. Shorter is not always better.

Rather than put a lot of ifs, sometimes a switch statement will make more sense. Possibly it will make the code purpose even clearer, depending on the conditions you are testing.

Nested if blocks or long sequences of else if can be avoided by returning early or branching out early while maintaining a single return point. Discussion about the concept: A better alternative to “return early” – PHP / WordPress edition

Not to mention the ternary operator.

Another good practice: comments. While good code should speak for itself, it doesn't hurt to put a few comments here and there. It shows that: 1) you had a clear idea in mind, 2) you are trying to help whomever will have to work on this code, including yourself (in 6 months you will have lost your train of thought and you'll have to re-analyze your own code), 3) you are capable of documenting your code, 4) you put yourself in the shoes of another person (including interviewers).

Even if you know you will be judged on your technical proficiency rather than your English writing skills, even if you are short on time, take the time to comment even if it's just two lines. Essentials parts of the code are not commented at all. The parts that are less obvious to understand deserve some explanation.

What I gather is that they just didn't like your style. One thing to keep in mind is that development is often teamwork rather than solo. The interviewers may be figuring out if you would be capable of working in a team, if you already follow good practices, consistent naming conventions etc.

Don't let this unfortunate experience put you out. The good news is that minor changes in style can have a huge impact as exemplified in the useful comments you've got here. Hopefully you will like our feedback better !

| improve this answer | |
\$\endgroup\$

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.