Cash Register Project - better solutions?

[mawaru]

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?

Link to the challenge: Link

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[1], 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][1] / rate[i][1]));
  } // 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][1]) {
        //...and substract it coin by coin from the change variable
        change -= rate[i][1];
        change = change.toFixed(2); // (numbers act funny without this...
        total -= rate[i][1];
        total = total.toFixed(2); // ...and this. Is there a better solution?..)
        sum += rate[i][1];

        if (sum > 0) {
          //if sum for the coin of this denomination is there, push it to changeArr
          midArr = [rate[i][0], 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][0] == changeArr[i][0]) {
              changeArr[i][1] = changeArr[i + 1][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: [] };
  }
}

[sanity]

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?

[mawaru]

(post deleted by author)

[mawaru]

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

[sanity]

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][0] == changeArr[i][0]) {
              changeArr[i][1] = changeArr[i + 1][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?

[mawaru]

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?..

[sanity]

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?

[mawaru]

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[1], 0).toFixed(2);
  let sum = 0;
  let auxiliaryArr = [];

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

  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][1]) {
        change -= rate[i][1];
        change = change.toFixed(2); // (numbers act funny without this...
        total -= rate[i][1];
        total = total.toFixed(2); // ...and this)
        sum += rate[i][1];

        if (sum > 0) {
          auxiliaryArr = [rate[i][0], 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][0] == changeArr[i][0]) {
        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: [] };
  }
}

[sanity]

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][0], 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][0] == changeArr[i][0]) {
        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.

[mawaru]

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][1]) {
        change -= rate[i][1];
        change = change.toFixed(2); // (numbers act funny without this...
        total -= rate[i][1];
        total = total.toFixed(2); // ...and this)
        sum += rate[i][1];

        let auxiliaryArr = [rate[i][0], 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][0] == changeArr[i][0]) {
        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][1] / rate[i][1]));
  }

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

[sanity]

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

        let auxiliaryArr = [rate[i][0], 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][0] == changeArr[i][0]) {
        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.

[mawaru]

My filter solution would be

changeArr = changeArr.filter(function (e, i, arr) {
    if (
      arr[i + 1] &&
      (arr[i + 1][0] !== arr[i][0] || arr[i + 1][1] < arr[i][1])
    ) {
      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[1], 0).toFixed(2);

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

  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][1]) {
        change -= rate[i][1];
        change = change.toFixed(2); // (numbers act funny without this...
        total -= rate[i][1];
        total = total.toFixed(2); // ...and this)
        sum += rate[i][1];

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

  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: [] };
  }
}

[sanity]

---

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

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][1]) {
        change -= rate[i][1];
        change = change.toFixed(2); // (numbers act funny without this...
        total -= rate[i][1];
        total = total.toFixed(2); // ...and this)
        sum += rate[i][1];

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

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][0], Number(sum.toFixed(2))];
        if (changeArr.length == 0) {
          changeArr.push(auxiliaryArr);
        } else if (changeArr.slice(-1)[0][0] != auxiliaryArr[0]) {
          changeArr.push(auxiliaryArr);
        } else if (changeArr.slice(-1)[0][1] < auxiliaryArr[1]) {
          changeArr.slice(-1)[0][1] = auxiliaryArr[1];
        }

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)[0][1] to be >= auxiliaryArr[1]?

[mawaru]

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][1]) {
      continue;
    } else {
      for (let j = 0; j < coinCounter[i] && change - rate[i][1] >= 0; j++) {
        change -= rate[i][1];
        change = change.toFixed(2); // (numbers act funny without this...
        total -= rate[i][1];
        total = total.toFixed(2); // ...and this)
        sum += rate[i][1];
      }

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

      changeArr.push(auxiliaryArr);
    }
  }

But with

if (change < rate[i][1]) {
      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][1] / rate[cid[i][0]]));
  }

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][1] >= 0; j++) {
        change -= rate[i][1];
        change = change.toFixed(2); // (numbers act funny without this...
        total -= rate[i][1];
        total = total.toFixed(2); // ...and this)
        sum += rate[i][1];
      }

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?..

[sanity]

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

Consider what is if's condition and code inside if block does. When change >= rate[i][1], condition of if isn’t met, other code is executed. When change < rate[i][1], 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][1] / rate[cid[i][0]]));
  }

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.

[mawaru]

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[1], 0).toFixed(2);

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

  for (let i = coinCounter.length - 1; i >= 0; i--) {
    let sum = 0;
    let denom = cid[i][0]; //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: [] };
  }
}