 # Final JS Exercise: Please Criticize My Code

Final JS Exercise: Please Criticize My Code
0

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, el * 100]);
//get the total sum of all cash and divide by 100
let sumOfCash = availCash.reduce((a, b) => (a + b),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] > 0) {
//1. moneyProvided array is increased by denomination value
moneyProvided[i] += denomination[i];
//2. changeNeeded is decreased by same denomination value
changeNeeded -= denomination[i];
//3. availCash is also decreased by same denomination value
availCash[i] -= denomination[i];
}
};

//clean up the moneyProvided array by
let change = moneyProvided
//1. resetting the money values by dividing by 100
.map(el => [el, el / 100])
//2. filtering out all non-empty dollar and value arrays
.filter(el => el !== 0);
//calculate the total of the change array
let changeTotal = change.reduce((a, b) => (a + b),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
};
``````

@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]`
`availCash[i]`
for readability, but no big deal. I can understand your variables fine.

1 Like

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.

1 Like

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.

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
``````