Feedback Please: Cash Register Challenge

Hello,

I finished the first working version of this project. I am looking for feedback. :slight_smile: I think next version I would like to refactor so its considered “functional”.


function checkCashRegister(price, cash, cashInDrawer) {
  let changeReturnObject = { status: 'OPEN', change: [] };
  let changeDue = cash - price;
  let temporaryChange;
  let temporaryMultiple;
  let sumOfCashRegister = 0;
  let denominatorNumber;
  let writtenDenominator = '';
  let cashInDrawerIndex;

  console.log(cashInDrawer);

  cashInDrawer.forEach((element) => {
    sumOfCashRegister = sumOfCashRegister += element[1];
  });

  if (sumOfCashRegister == changeDue) {
    return { status: 'CLOSED', change: cashInDrawer };
  }

  if (price > cash) {
    return { status: 'INSUFFICENT_FUNDS', change: [] };
  }
  if (price === cash) {
    return { status: 'OPEN', change: [] };
  }

  const cashRegister = (
    denominatorNumber,
    writtenDenominator,
    cashInDrawerIndex
  ) => {
    // console.log(
    //   'Denominator ',
    //   denominatorNumber,
    //   'Written ',
    //   writtenDenominator
    // );
    // remainder change to be calculated
    temporaryChange = (changeDue % denominatorNumber).toFixed(2);
    // How many potential denominator can we remove from change total
    temporaryMultiple = Math.floor(changeDue / denominatorNumber);
    // How many multiples of currenty denominator are currentlly in the cash register
    let howManyCurrentMultiplesInCashRegister =
      cashInDrawer[cashInDrawerIndex][1] / denominatorNumber;
    // console.log(howManyCurrentMultiplesInCashRegister);
    // Not Enough 100 Dollar Bills In Cash Register
    if (howManyCurrentMultiplesInCashRegister < temporaryMultiple) {
      changeReturnObject.change.push([
        writtenDenominator,
        howManyCurrentMultiplesInCashRegister * denominatorNumber,
      ]);
      changeDue =
        changeDue -
        (howManyCurrentMultiplesInCashRegister * denominatorNumber).toFixed(2);
      // console.log('Change due: ', changeDue);
    }
    // Enough 100 Multiples in Cash Register for Change
    if (howManyCurrentMultiplesInCashRegister >= temporaryMultiple) {
      changeReturnObject.change.push([
        writtenDenominator,
        temporaryMultiple * denominatorNumber,
      ]);
      changeDue = (changeDue - temporaryMultiple * denominatorNumber).toFixed(
        2
      );
      // console.log('Change due: ', changeDue);
    }
  };

  if (changeDue / 100 >= 1) {
    denominatorNumber = 100;
    writtenDenominator = 'ONE HUNDRED';
    cashInDrawerIndex = 8;
    cashRegister(denominatorNumber, writtenDenominator, cashInDrawerIndex);
  }

  if (changeDue / 20 >= 1) {
    denominatorNumber = 20;
    writtenDenominator = 'TWENTY';
    cashInDrawerIndex = 7;
    cashRegister(denominatorNumber, writtenDenominator, cashInDrawerIndex);
  }
  if (changeDue / 10 >= 1) {
    denominatorNumber = 10;
    writtenDenominator = 'TEN';
    cashInDrawerIndex = 6;
    cashRegister(denominatorNumber, writtenDenominator, cashInDrawerIndex);
  }
  if (changeDue / 5 >= 1) {
    denominatorNumber = 5;
    writtenDenominator = 'FIVE';
    cashInDrawerIndex = 5;
    cashRegister(denominatorNumber, writtenDenominator, cashInDrawerIndex);
  }

  if (changeDue / 1 >= 1) {
    denominatorNumber = 1;
    writtenDenominator = 'ONE';
    cashInDrawerIndex = 4;
    cashRegister(denominatorNumber, writtenDenominator, cashInDrawerIndex);
  }

  if (changeDue / 0.25 >= 1) {
    denominatorNumber = 0.25;
    writtenDenominator = 'QUARTER';
    cashInDrawerIndex = 3;
    cashRegister(denominatorNumber, writtenDenominator, cashInDrawerIndex);
  }
  if (changeDue / 0.1 >= 1) {
    denominatorNumber = 0.1;
    writtenDenominator = 'DIME';
    cashInDrawerIndex = 2;
    cashRegister(denominatorNumber, writtenDenominator, cashInDrawerIndex);
  }

  if (changeDue / 0.05 >= 1) {
    denominatorNumber = 0.05;
    writtenDenominator = 'NICKEL';
    cashInDrawerIndex = 1;
    cashRegister(denominatorNumber, writtenDenominator, cashInDrawerIndex);
  }

  if (changeDue / 0.01 >= 1) {
    denominatorNumber = 0.01;
    writtenDenominator = 'PENNY';
    cashInDrawerIndex = 0;
    cashRegister(denominatorNumber, writtenDenominator, cashInDrawerIndex);
  }

  if (changeDue != 0) {
    return { status: 'INSUFFICIENT_FUNDS', change: [] };
  }

  // console.log('Return Object ', changeReturnObject);

  return changeReturnObject;
}

checkCashRegister(3.26, 100, [
  ['PENNY', 1.01],
  ['NICKEL', 2.05],
  ['DIME', 3.1],
  ['QUARTER', 4.25],
  ['ONE', 90],
  ['FIVE', 55],
  ['TEN', 20],
  ['TWENTY', 60],
  ['ONE HUNDRED', 100],
]);

module.exports = checkCashRegister;

Thanks,

:+1:t2:

It’s nice to keep the most of the complex code in a single place (cashRegister function), this allows the rest to be quite straightforward and simpler. It works and it isn’t overly complicated, that’s definitely a plus.

Did you had another look at the code after reaching the passing version, with having in mind cleaning it up and looking for places that can be improved? Some notes for that below.


Declaring variables before they are used/needed. To see what variable is needed for, often it is required to go back and forth thought the code. They could be declared a bit closer to the place where they are actually used.

Some of them also don’t seem to be needed in the outer function, or needed at all - for example temporaryChange is assigned in the inner cashRegister function, but it’s not used later.

temporaryMultiple might use some better name.


sumOfCashRegister = sumOfCashRegister += element[1];

This looks more complicated than it should be :slightly_smiling_face:


Notice all the ifs like the one below are doing exactly the same, just with different values. Any ideas how that repeating could be removed?

  if (changeDue / 100 >= 1) {
    denominatorNumber = 100;
    writtenDenominator = 'ONE HUNDRED';
    cashInDrawerIndex = 8;
    cashRegister(denominatorNumber, writtenDenominator, cashInDrawerIndex);
  }

In the cashRegister, it looks a bit that these two ifs were made separate (instead of if/else just to add a comment before them explaining the condition (which comment isn’t correct anymore).

if (howManyCurrentMultiplesInCashRegister < temporaryMultiple) {
if (howManyCurrentMultiplesInCashRegister >= temporaryMultiple) {

With more appropriate name for temporaryMultiple this still could be a bit mouthful. One way could be assigning the evaluating the result of condition before the if , assigning it to another variable (with some appropriate name) and then using that variable in if.


I don’t know how much troubles you had with the calculations made on the floating point values, but one thing that still could be considered to do is finding way to make as many internal calculations as possible on integers. And not need to deal with possible precision issues in the intermediate results.