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.

Hello,

First. Thanks for taking the time to write feedback!
I apologize for the response delay.

I did review/change the code a few times before and after first functioning version.

Here is the entire history and another newer version container most of the changes suggested.

Github link

Since you did the review I updated the code significantly. taking into account most of your suggestions.

Removed UN used variables

sumOfCashRegister = sumOfCashRegister += element[1];

All I can say to that is it has been replaced. lol

Moved Variables Closer to where they are needed
Refactored the code to reduce code
Made more code readable by improving naming convention(would love some feedback on this)
Changed"IF IF" to “IF ELSE”

I did have problems with the floating point value and came up with a duct tape sort of solution as you surely noticed. :slight_smile:

Can you elaborate on what you mean by “internal calculations as possible on integers”.

Thanks again!

That’s really nice progression :+1:t2:


What I meant basically - before starting calculations, make PENNY correspond to 1 instead of 0.01 - with changes to other denominations, to conform that. Do all calculations on integers. At the last step before returning result, change the values back, so PENNY means 0.01 again.


Regarding naming, there’s saying pointing to naming things as one of the only two hard things in the Computer Science. They are now more descriptive and say more about variables.

I’m not entirely sure what is the javascript convention for capitalization. What I’ve seen variables defined with const are usually still following the camelCase capitalization.


  let index = 8;
  LEGAL_TENDER_ARR.forEach((denominations) => {
    if (changeDue / denominations[1] >= 1) {
      CashRegister(denominations[1], denominations[0], index);
    }
    index -= 1;
  });

This looks a bit, like changing order in LEGAL_TENDER_ARR might make some things easier.

Some more clarity could be probably achieved by destructuring each denomination (['PENNY', 1.01]), when defining callback function in forEach.


    if (
      CURRENT_MULTIPLES_CASH_REGISTER < POTENTIAL_DENOMINATIONS_REMOVABLE_CHANGE
    ) {
      STATUS_AND_CHANGE_DUE.change.push([
        denominatorString,
        CURRENT_MULTIPLES_CASH_REGISTER * denomination,
      ]);
      changeDue =
        changeDue - (CURRENT_MULTIPLES_CASH_REGISTER * denomination).toFixed(2);
    } else {
      STATUS_AND_CHANGE_DUE.change.push([
        denominatorString,
        POTENTIAL_DENOMINATIONS_REMOVABLE_CHANGE * denomination,
      ]);
      changeDue = (
        changeDue -
        POTENTIAL_DENOMINATIONS_REMOVABLE_CHANGE * denomination
      ).toFixed(2);
    }

There’s some similar actions taken in both of the cases here, some simplification might be possible.

This topic was automatically closed 182 days after the last reply. New replies are no longer allowed.