# Cash Register Project - better solutions?

Hi guys! I made this ungodly abomination of a code to work and pass all the tests So I guess now I have a moral right to ask what would be a better solution? Could you please share some of your ideas on how to optimize this code or, better, suggest some completely different more efficient approaches?

Here’s my code:

``````function checkCashRegister(price, cash, cid) {
const rate = [ //"exchange" rate
["PENNY", 0.01],
["NICKEL", 0.05],
["DIME", 0.1],
["QUARTER", 0.25],
["ONE", 1],
["FIVE", 5],
["TEN", 10],
["TWENTY", 20],
["ONE HUNDRED", 100],
];
let changeArr = []; // an array with all the change we are to return
let coin = []; //array with the exact amount of different banknotes/coins in the register
let change = cash - price;
let total = cid.reduce((sum, category) => sum + category, 0).toFixed(2);
let sum = 0; //sum of change for an each specific banknote/coin
let midArr = [];

for (let i = 0; i < Object.keys(rate).length; i++) {
coin.push(Math.round(cid[i] / rate[i]));
} // to figure out how many banknotes/coins of each denomination there are in the regester

rate.reverse(); // I thought it was easier to reverse these two rather than rewrite loops below
coin.reverse();
// loop through each type of money in the register (start with the hundreds)
for (let i = 0; i <= coin.length; i++) {
for (let j = 0; j < coin[i]; j++) {
//loop through amount of the said type of money...
if (change >= rate[i]) {
//...and substract it coin by coin from the change variable
change -= rate[i];
change = change.toFixed(2); // (numbers act funny without this...
total -= rate[i];
total = total.toFixed(2); // ...and this. Is there a better solution?..)
sum += rate[i];

if (sum > 0) {
//if sum for the coin of this denomination is there, push it to changeArr
midArr = [rate[i], Number(sum.toFixed(2))];
changeArr.push(midArr);
midArr = [];
}
// however if there are two or more coins of the same denomination
// the changeArr will have them all ([DIME, 0.1], [DIME, 0.2] etc),
// the sum will increase though, so I deleted all the repeating coins from the changeArr
// except for the one with the largest sum
if (change == 0 && total > 0) {
for (let i = 0; i < changeArr.length; i++) {
if (!changeArr[i + 1]) {
break;
} else if (changeArr[i + 1] == changeArr[i]) {
changeArr[i] = changeArr[i + 1];
delete changeArr[i];
}
}
// remove empty arrays from changeArr
changeArr = changeArr.filter(function (el) {
return el != null;
});
return { status: "OPEN", change: changeArr };
} else if (change == 0 && total == 0) {
return { status: "CLOSED", change: cid };
}
}
}
sum = 0;
}
//if after all of that there's still change to give away, means there's not enough money
// or no sutable coins/banknotes
if (change > 0) {
return { status: "INSUFFICIENT_FUNDS", change: [] };
}
}
``````

I’d start with asking to think a bit about your solution. What do you consider good about it, what bad? What was easier than expected, what was hard? Why do you think it needs optimizing or something more efficient? What place in code would you consider as a starting point during improvement?

(post deleted by author)

I’d say my code is too long and convoluted. There must be a more elegant approach: I assume the part with the loops can be rewritten (probably with .reduce method) and it would rid the code of good half of the patches I added afterwards to make it work - that’d be both the starting point and the hard part, since I just can’t figure out how to do it

Wouldn’t removing code that made it work, make it stop working? Reduce will also not make it more elegant or less convoluted on it own. This sounds like you’d like to make it more clear/clean/readable or simpler. There’s couple good news about that. Having working code is a good point to start thinking how it can be made simpler and it can be done in small steps. One change at a time.

Right now I’d say the most noise in code is caused by the comments. If they are explaining what happens in code, it usually means they can be removed. Unless code is extremely complicated, such comments don’t add much, as somebody reading code would know what is going on. Comments may include why code is doing something, not what.

To help with clarity of code, variable names are important, for example

``````  let coin = []; //array with the exact amount of different banknotes/coins in the register
``````

From the variable name it’s rather hard to figure out the indented usage of this variable. Naming things isn’t easy (some say, naming things it’s one of the only two hard things in computer science), but some more appropriate variable name, would help with making easier to understand code. Without the need for explaining it in comment.

Another thing to look for are places that might have code not really needed (with or without some other changes).
For example:

``````  rate.reverse(); // I thought it was easier to reverse these two rather than rewrite loops below
coin.reverse();
``````

I can understand changing long loops below doesn’t look encouraging. What about changing code above, so both arrays at that point are already reversed? From the brief look that’s changing one short loop, and order in initial `rate` array.

Some way of shortening loops could be trying to find code that’s not really needed in the loop, for example:

``````        if (change == 0 && total > 0) {
for (let i = 0; i < changeArr.length; i++) {
if (!changeArr[i + 1]) {
break;
} else if (changeArr[i + 1] == changeArr[i]) {
changeArr[i] = changeArr[i + 1];
delete changeArr[i];
}
}
// remove empty arrays from changeArr
changeArr = changeArr.filter(function (el) {
return el != null;
});
return { status: "OPEN", change: changeArr };
} else if (change == 0 && total == 0) {
return { status: "CLOSED", change: cid };
}
``````

Does this part of code needs to be in the

``````  for (let i = 0; i <= coin.length; i++) {
for (let j = 0; j < coin[i]; j++) {
``````

loops?

1 Like

Wow, thank you! The reverse() things were actually easy to remove, why didn’t I think of it myself? Also I get it about about comments and naming, I’m going to be more considerate with the variable names in the future

I also moved the chunk of the code outside the loop (and found a line of unnecessary code in it and removed it), so it actually more readable now.

It seems the small improvement approach only goes this far thought?..

As mentioned this is a step-by-step process. With each change other places that can be improved can reveal itself. It can help with deeper understanding of the problem, and being able to easier identify where something is overly complicated. Unless something is trivial, there’s no magical way to go from complex code to short one-liner.

How your code looks now?

Here you are

``````function checkCashRegister(price, cash, cid) {
const rate = [         //'exchange' rate
["PENNY", 0.01],
["NICKEL", 0.05],
["DIME", 0.1],
["QUARTER", 0.25],
["ONE", 1],
["FIVE", 5],
["TEN", 10],
["TWENTY", 20],
["ONE HUNDRED", 100],
];
let changeArr = [];
let coinCounter = [];
let change = cash - price;
let total = cid.reduce((sum, category) => sum + category, 0).toFixed(2);
let sum = 0;
let auxiliaryArr = [];

for (let i = 0; i < Object.keys(rate).length; i++) {
coinCounter.unshift(Math.round(cid[i] / rate[i]));
}

rate.reverse(); //still easier and more readable than rewriting the loop above and the rate-object ...or is it?

for (let i = 0; i <= coinCounter.length; i++) {
for (let j = 0; j < coinCounter[i]; j++) {
if (change >= rate[i]) {
change -= rate[i];
change = change.toFixed(2); // (numbers act funny without this...
total -= rate[i];
total = total.toFixed(2); // ...and this)
sum += rate[i];

if (sum > 0) {
auxiliaryArr = [rate[i], Number(sum.toFixed(2))];
changeArr.push(auxiliaryArr);
auxiliaryArr = [];
}
}
}
sum = 0;
}

if (change == 0 && total > 0) {
for (let i = 0; i < changeArr.length; i++) {
if (!changeArr[i + 1]) {
break;
} else if (changeArr[i + 1] == changeArr[i]) {
delete changeArr[i];
}
}
changeArr = changeArr.filter(function (el) {
return el != null;
});
return { status: "OPEN", change: changeArr };
} else if (change == 0 && total == 0) {
return { status: "CLOSED", change: cid };
} else if (change > 0) {
return { status: "INSUFFICIENT_FUNDS", change: [] };
}
}
``````

This is better, it’s already much easier to follow. I’d still say, that writing array in different order is more convenient, than keeping its order and requiring to explain that in comment. Ultimately that’s your decision though.

I’m seeing two next places that can help with making function simpler and cleaner.

First one:

``````        if (sum > 0) {
auxiliaryArr = [rate[i], Number(sum.toFixed(2))];
changeArr.push(auxiliaryArr);
auxiliaryArr = [];
}
``````

Is there situation, when `sum` can be less than or equal `0` at this point in code?

This part touches a bit different thing that can be looked at:

``````  let sum = 0;
let auxiliaryArr = [];
``````

Defining variables at the start of function, rather than closer to where they are used. It could be argued if `auxiliaryArr` variable is needed at all, but it could be defined with `const` just before line that’s pushing it to `changeArr`. Re-assigning it after that to the empty array is also not needed.

Bit similar is with `sum`, where it’s re-assigned to `0` after it’s used, instead of declaring it in a right point of the loop.

Second place:

``````    for (let i = 0; i < changeArr.length; i++) {
if (!changeArr[i + 1]) {
break;
} else if (changeArr[i + 1] == changeArr[i]) {
delete changeArr[i];
}
}
changeArr = changeArr.filter(function (el) {
return el != null;
});
``````

Looking at this part two things are sticking out - using of `break` and `delete`.

``````      if (!changeArr[i + 1]) {
break;
``````

Doesn’t this simply check if there’s one more element in the `changeArr`? How loop could be defined that it would be ending looping at the same point, as `break` here?

Once that part is dealt with, there’s one emerging pattern, that’s first using `delete` to delete specific element, and then filtering such deleted elements. These both can be simplified to just using `filter` method, with appropriate filtering callback function.

1 Like

I’m back! You’re right, sum is always > 0 I’ve fixed it and tactically placed the variables, so that chunk of code looks like this now:

``````for (let i = 0; i <= coinCounter.length; i++) {
let sum = 0;
for (let j = 0; j < coinCounter[i]; j++) {
if (change >= rate[i]) {
change -= rate[i];
change = change.toFixed(2); // (numbers act funny without this...
total -= rate[i];
total = total.toFixed(2); // ...and this)
sum += rate[i];

let auxiliaryArr = [rate[i], Number(sum.toFixed(2))];
changeArr.push(auxiliaryArr);
}
}
}
``````

Also I’ve got rid of at least `break` thing but couldn’t figure out a proper callback function for the `filter`

``````if (change == 0 && total > 0) {
for (let i = 0; i < changeArr.length; i++) {
if (changeArr[i + 1] && changeArr[i + 1] == changeArr[i]) {
delete changeArr[i];
}
}
changeArr = changeArr.filter(function (el) {
return el != null;
});
``````

I don’t give up, but … I could use a hint Also the `rate.reverse();` thing, if we reverse the order of the elements in the rate-object, we’ll have to reverse the `cid` as well, otherwise

``````for (let i = 0; i < Object.keys(rate).length; i++) {
coinCounter.unshift(Math.round(cid[i] / rate[i]));
}
``````

won’t work, right? So there’s no point, since we have to reverse something anyway… Or am I missing something?

``````for (let i = 0; i <= coinCounter.length; i++) {
let sum = 0;
for (let j = 0; j < coinCounter[i]; j++) {
if (change >= rate[i]) {
change -= rate[i];
change = change.toFixed(2); // (numbers act funny without this...
total -= rate[i];
total = total.toFixed(2); // ...and this)
sum += rate[i];

let auxiliaryArr = [rate[i], Number(sum.toFixed(2))];
changeArr.push(auxiliaryArr);
}
}
}
``````

Now, when inner loop is clearer, question arises what can be done, so only one entry is pushed to `changeArr`. In the next step all duplicated entries of the same denomination are removed from `changeArr`, leaving just one entry per denomination (if denomination will be used).

``````if (change == 0 && total > 0) {
for (let i = 0; i < changeArr.length; i++) {
if (changeArr[i + 1] && changeArr[i + 1] == changeArr[i]) {
delete changeArr[i];
}
}
changeArr = changeArr.filter(function (el) {
return el != null;
});
``````

I was thinking more about changing `for` loop condition to `i < changeArr.length - 1`, but the way it is now is even more convenient. Callback function of `filter` can accept more than one argument. First one is element from the array, but the second one is the index of the element. If you have index of the element, then the same condition as in the `for` loop can be used in the `filter`. Remember callback function returns `false` when element shouldn’t be included in new array, and `true` if it should.

I’ll come back to the order of `rate` array. For now I’d encourage considering whether it’s actually used in the code like an array - is it actually being iterated over.

My `filter` solution would be

``````changeArr = changeArr.filter(function (e, i, arr) {
if (
arr[i + 1] &&
(arr[i + 1] !== arr[i] || arr[i + 1] < arr[i])
) {
return e;
} else if (!arr[i + 1]) {
return e;
}
});
``````

Not sure it’s the optimal way, but it works. And then I figured how to drop it altogether As you said, with the clearer code it was quite easy

``````function checkCashRegister(price, cash, cid) {
const rate = [
//exchange rate
["PENNY", 0.01],
["NICKEL", 0.05],
["DIME", 0.1],
["QUARTER", 0.25],
["ONE", 1],
["FIVE", 5],
["TEN", 10],
["TWENTY", 20],
["ONE HUNDRED", 100],
];

let changeArr = [];
let coinCounter = [];
let change = cash - price;
let total = cid.reduce((sum, category) => sum + category, 0).toFixed(2);

for (let i = 0; i < Object.keys(rate).length; i++) {
coinCounter.unshift(Math.round(cid[i] / rate[i]));
}

rate.reverse(); //still easier?..

for (let i = 0; i <= coinCounter.length; i++) {
let sum = 0;
for (let j = 0; j < coinCounter[i]; j++) {
if (change >= rate[i]) {
change -= rate[i];
change = change.toFixed(2); // (numbers act funny without this...
total -= rate[i];
total = total.toFixed(2); // ...and this)
sum += rate[i];

let auxiliaryArr = [rate[i], Number(sum.toFixed(2))];
if (changeArr.length == 0) {
changeArr.push(auxiliaryArr);
} else if (changeArr.slice(-1) != auxiliaryArr) {
changeArr.push(auxiliaryArr);
} else if (changeArr.slice(-1) < auxiliaryArr) {
changeArr.slice(-1) = auxiliaryArr;
}
}
}
}

if (change == 0 && total > 0) {
return { status: "OPEN", change: changeArr };
} else if (change == 0 && total == 0) {
return { status: "CLOSED", change: cid };
} else if (change > 0) {
return { status: "INSUFFICIENT_FUNDS", change: [] };
}
}
``````

``````  for (let i = 0; i < Object.keys(rate).length; i++) {
coinCounter.unshift(Math.round(cid[i] / rate[i]));
}
``````

Taking a look at this, what is actually iterated here? What is more important - `cid` array, or our `rate` array? Is it necessary for `rate` to be an array? Currently that’s convenient, because specific index in `cid` corresponds to the specific index in `rate`, but as long as, based on the denomination from `cid`, we’d be able to get rate with the same denomination, it wouldn’t matter if `rate` is array, would it?

``````    for (let j = 0; j < coinCounter[i]; j++) {
if (change >= rate[i]) {
change -= rate[i];
change = change.toFixed(2); // (numbers act funny without this...
total -= rate[i];
total = total.toFixed(2); // ...and this)
sum += rate[i];

let auxiliaryArr = [rate[i], Number(sum.toFixed(2))];
if (changeArr.length == 0) {
changeArr.push(auxiliaryArr);
} else if (changeArr.slice(-1) != auxiliaryArr) {
changeArr.push(auxiliaryArr);
} else if (changeArr.slice(-1) < auxiliaryArr) {
changeArr.slice(-1) = auxiliaryArr;
}
}
}
``````

Looking at the first `if` here. It contains the whole code, that can be executed in the loop, increasing indenting of it. Equivalent of this, would be adding at the front `if` with condition, that’s exact opposite and continuing or breaking loop (depending what’s appropriate), while removing current `if` and decreasing indentation. That will make this part a bit more flat, and easier to follow.

Second thing to look here is what’s happening in the loop. As long as `change` is higher than denomination rate, `change` and `total` is decreased. Starting at `0` times those values are decreased, `1`, `2`, `3`… times and so on, until finally `x` times in total that operation is repeated. Question here is - is it necessary to do that from the start? Having a way to either find, or skip, to that last number might be more convenient.

``````        let auxiliaryArr = [rate[i], Number(sum.toFixed(2))];
if (changeArr.length == 0) {
changeArr.push(auxiliaryArr);
} else if (changeArr.slice(-1) != auxiliaryArr) {
changeArr.push(auxiliaryArr);
} else if (changeArr.slice(-1) < auxiliaryArr) {
changeArr.slice(-1) = auxiliaryArr;
}
``````

When we are past the second condition (denominations are the same in the `auxiliaryArr` and at last element of `changeArr`. Is it possible for the `changeArr.slice(-1)` to be `>= auxiliaryArr`?

Woah, we don’t even need to check whether auxiliaryArr should be pushed to changeArr or not

``````for (let i = 0; i < coinCounter.length; i++) {
let sum = 0;
if (change < rate[i]) {
continue;
} else {
for (let j = 0; j < coinCounter[i] && change - rate[i] >= 0; j++) {
change -= rate[i];
change = change.toFixed(2); // (numbers act funny without this...
total -= rate[i];
total = total.toFixed(2); // ...and this)
sum += rate[i];
}

let auxiliaryArr = [rate[i], Number(sum.toFixed(2))];

changeArr.push(auxiliaryArr);
}
}
``````

But with

``````if (change < rate[i]) {
continue;
``````

indenting has increased it seems As for the rate, I guess it could look like this

``````const rate = {
"ONE HUNDRED": 100,
TWENTY: 20,
TEN: 10,
FIVE: 5,
ONE: 1,
QUARTER: 0.25,
DIME: 0.1,
NICKEL: 0.05,
PENNY: 0.01,
};
``````

And the we can do

``````or (let i = 0; i < Object.keys(rate).length; i++) {
coinCounter.unshift(Math.round(cid[i] / rate[cid[i]]));
}
``````

And get rid of `rate.reverse()` but then I have no idea how to handle this part

``````for (let j = 0; j < coinCounter[i] && change - rate[i] >= 0; j++) {
change -= rate[i];
change = change.toFixed(2); // (numbers act funny without this...
total -= rate[i];
total = total.toFixed(2); // ...and this)
sum += rate[i];
}
``````

It seems the solution would require to iterate over the rate-object in order, starting from the highest value to the lowest, but how to do that I have no idea. Key-value pairs order gets mixed up and there’s no indexes or anything so how to iterate over it in the order I need?..

``````    if (change < rate[i]) {
continue;
} else {
for (let j = 0; j < coinCounter[i] && change - rate[i] >= 0; j++) {
change -= rate[i];
// (...)
}
``````

Consider what is `if`'s condition and code inside `if` block does. When `change >= rate[i]`, condition of `if` isn’t met, other code is executed. When `change < rate[i]`, then code from `if` is executed, resulting in continuing to the next iteration of outer loop.
In any case that’s `if` condition is true, no other code after it will be executed. This means `else` is not needed and that whole part can have reduced indentation, `if` with `continue` will anyway prevent that code from executing in not wanted cases.

``````for (let i = 0; i < Object.keys(rate).length; i++) {
coinCounter.unshift(Math.round(cid[i] / rate[cid[i]]));
}
``````

Yes, `rate` doesn’t need to be an array. Making it object will give some flexibility, that requires only knowing which denomination is needed. This loop is looping over `cid` really. For a bit of clarity, current `cid[i]` could be destructured inside of the loop into two variables, which are being used in calculation.

Later `rate` can be used in similar manner, but there’s couple additional conditions. Order of denominations in `cid` and `coinCounter` is the same, and `coinCounter` is still iterated over in order that gives the correct answer. There’s couple other ways to deal with this too, what is important is knowing which denomination is being used in which iteration, to get the correct rate.

How about this? ``````function checkCashRegister(price, cash, cid) {
const rate = {
PENNY: 0.01,
NICKEL: 0.05,
QUARTER: 0.25,
ONE: 1,
FIVE: 5,
TEN: 10,
DIME: 0.1,
TWENTY: 20,
"ONE HUNDRED": 100,
};

let changeArr = [];
let coinCounter = [];
let change = cash - price;
let total = cid.reduce((sum, category) => sum + category, 0).toFixed(2);

for (let i = 0; i < Object.keys(rate).length; i++) {
coinCounter.push(Math.round(cid[i] / rate[cid[i]]));
}

for (let i = coinCounter.length - 1; i >= 0; i--) {
let sum = 0;
let denom = cid[i]; //denomination
if (change < rate[denom]) {
continue;
}
for (let j = 0; j < coinCounter[i] && change - rate[denom] >= 0; j++) {
change -= rate[denom];
change = change.toFixed(2);
total -= rate[denom];
total = total.toFixed(2);
sum += rate[denom];
}
let auxiliaryArr = [denom, sum.toFixed(2)];
changeArr.push(auxiliaryArr);
}

if (change == 0 && total > 0) {
return { status: "OPEN", change: changeArr };
} else if (change == 0 && total == 0) {
return { status: "CLOSED", change: cid };
} else if (change > 0) {
return { status: "INSUFFICIENT_FUNDS", change: [] };
}
}
``````

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