Final JS Exercise: Please Criticize My Code

Code Feedback
1

The exercise is: JavaScript Algorithms and Data Structures Projects: Cash Register

The answer sheet is here.

I struggled to understand the suggested solutions on the answer sheet. So I applied what I knew from the Roman Numeral Converter. While the answer worked, I think there’s room for improvement.

Please read and criticize my code. After many hours working on this exercise, I need a fresh pair of eyes to help me improve.

Here’s my code:

function checkCashRegister(price, cash, cid) {
  //all money values are multiplied by 100 to deal with precision errors involved with decimals 
  const denomination = [10000, 2000, 1000, 500, 100, 25, 10, 5, 1,];

  function transaction(price, cash, cid) {
    let changeNeeded = (cash - price) * 100;
    //money will be pushed to the second value in each array
    let moneyProvided = [
    ["ONE HUNDRED", 0], 
    ["TWENTY", 0], 
    ["TEN", 0], 
    ["FIVE", 0], 
    ["ONE", 0], 
    ["QUARTER", 0], 
    ["DIME", 0], 
    ["NICKEL", 0], 
    ["PENNY", 0],
  ];
  //take the cid, reverse it (like in Roman Numerals exercise), multiply values by 100
  let availCash = [...cid].reverse().map(el => [el[0], el[1] * 100]);
  //get the total sum of all cash and divide by 100
  let sumOfCash = availCash.reduce((a, b) => (a + b[1]),0) / 100;
  //if sumOfCash is exact change needed return
  if (sumOfCash === changeNeeded / 100) {
    return {status: "CLOSED", change: [...cid]};
  }
  //else, run this function
  else for (let i = 0; i < availCash.length; i++) {
    //if denomination values are less than changeNeeded and availableCash values are greater than 0, run the while loop
      while (denomination[i] <= changeNeeded && availCash[i][1] > 0) {
        //1. moneyProvided array is increased by denomination value
        moneyProvided[i][1] += denomination[i];
        //2. changeNeeded is decreased by same denomination value
        changeNeeded -= denomination[i];
        //3. availCash is also decreased by same denomination value
        availCash[i][1] -= denomination[i];
      }
    };
    
   //clean up the moneyProvided array by
    let change = moneyProvided
    //1. resetting the money values by dividing by 100
    .map(el => [el[0], el[1] / 100])
    //2. filtering out all non-empty dollar and value arrays
    .filter(el => el[1] !== 0);
    //calculate the total of the change array
    let changeTotal = change.reduce((a, b) => (a + b[1]),0);
    //if the total change is less than the change needed
    if (changeTotal < changeNeeded) {
        return {status: "INSUFFICIENT_FUNDS", change: []};
    }
    return {status: "OPEN", change};
  }

  //this is where the transaction function is called
  let answer = transaction(price, cash, cid);
  //here the final answer is provided if the 2 if statements don't catch it first
  return answer;
};
5 Likes
2

@damsalem I think that if your code is working that is the important thing. As you progress and learn more, look at the code of others, you can come back to this exersize and make improvements. I have looked at code that I wrote last year and I say to myself, “Oh my, I cannot believe I wrote that”.

I would say that you should make sure the variables make sense. I prefer to write complete words out like this
availableCash[i][1]
instead of
availCash[i][1]
for readability, but no big deal. I can understand your variables fine.

3 Likes
3

Without diving deep, I see following structure:

function outer() {
  const array = [];
  function inner() {}
  return inner();
}

The only case where this structure makes sense if inner is recursive and array is some sort of dynamic memo for inner, which is not your case and therefore nested inner makes no sense to me.

3 Likes
4

Ahhh that makes so much sense!

When I was writing it, my thought process was, I should make these parts modular, or at least separate concerns. Afterwards, I saw that it looked a little bit weird, but wasn’t certain.

If I remove the function inner() {}, would you suggest organizing the code differently? Right now I do think it all goes in sequence.

5

First of all, you want to separate “global” logic from “local” logic, for instance function that deals with currencies and operations with currencies, as it “globally” applies to any “local” function that you have (or might have in future).

Additionally, if you feel that your “local” logic is a bit bulky - too many if...else for example, you normally always can break it down in few “local” functions/modules. Generally it’s healthier and easier to debug.

Regarding your approach with currencies, AND I SEE IT TOO OFTEN, you really shouldn’t expect that price * 100 will save you from precision errors. Consider this example:

const moneyInDrawer = 7;
const paid = .07;
const rest = moneyInDrawer - paid * 100;

const status = rest === 0 ? 'CLOSED' : 'OPEN'; // OPEN (Nasty bug!)

I have no idea why so many people are making this assumption, but generally, if you want to make assumption that any amount would be surely rounded to 2 decimals you would just round it to 2 decimals:

const currencify = (expression, precision = 2) => +(expression.toFixed(precision));

console.log(currencify(.2 + .1)); // 0.3
4 Likes
6

I appreciate your detailed feedback!

I suspected there was flawed thinking around multiplying by 100 to handle precision errors. Using .toFixed() looks like a much improved solution.

7

I could only understand a possible solution by looking at your code. Thanks and congrats!