Why is the data in my array changing when using an array item to locate index of another array?-CodePudding

I have a problem that I just can't get my head around. I think that it basically boils down to these two lines of code:

const currentDenomination = CASHVALUES[i][1];

change[change.indexOf(CASHVALUES[i])][1]  = currentDenomination;

The value at CASHVALUES[i][1] is doubling each time this is carried out. As far as I can see the data in the change array is all that should be affected. I have tried giving currentDenomination a clone of CASHVALUES in case that was causing the issue, but I got the same result. Please see the full offending snippet below.

I hope I have been clear enough, but if any more information is required, please let me know.

function checkCashRegister(price, cash, cid) {
  let cid = [["PENNY", 1.01], ["NICKEL", 2.05], ["DIME", 3.1], ["QUARTER", 4.25], ["ONE", 90], ["FIVE", 55], ["TEN", 20], ["TWENTY", 60], ["ONE HUNDRED", 100]];

  const CASHVALUES = [["ONE HUNDRED", 100], ["TWENTY", 20], ["TEN", 10], ["FIVE", 5], ["ONE", 1], ["QUARTER", 0.25], ["DIME", 0.1], ["NICKEL", 0.05], ["PENNY", 0.01]];

  let changeDue = (cash - price);
  const TotalChangeDue = changeDue
  let change = [];
  let status = "OPEN";

  cid.reverse();

  for(let i=0;i<CASHVALUES.length;i  ) {
    const currentDenomination = CASHVALUES[i][1];
    
    if(changeDue >= currentDenomination) {
      if(cid[i][1] > 0) {
        cid[i][1] -= currentDenomination;
        cid[i][1] = cid[i][1].toFixed(2);
        changeDue -= currentDenomination;
        changeDue = changeDue.toFixed(2);
        if(!change.includes(CASHVALUES[i])) {
          change.push(CASHVALUES[i]);
        }
        else {
          change[change.indexOf(CASHVALUES[i])][1]  = currentDenomination;
        } 
          i--;     
        
      }
          
    }
  }
}

CodePudding user response：

I can spot quite a few problems in this code.

someNumber.toFixed(2) returns a string, not a number, which could lead to unexpected behavior, like in this line:

cid[i][1] = cid[i][1].toFixed(2);

cid.reverse() will reverse cid's content in-place, so for one invocation of checkCashRegister it'll order descending, but next invocation it'll order ascending. Really not ideal way to go.
These lines involves the strange doubling you see:

change.push(CASHVALUES[i]);
change[change.indexOf(CASHVALUES[i])][1]  = currentDenomination;

You think you're modifying just change[index][1], in fact you're modifying CASHVALUES[i][1] at the same time. this is because, change[index] is in fact a REFERENCE to the object at CASHVALUES[i], not a clone of the object.

JS does not have explicit pointer, non-primitive data types (e.g. object, array, function) are implicitly passed by reference, not clone.

For this particular problem, nested array isn't a good fit. You'll make your life easier using plain object. I also find Map useful cus unlike plain object, it retains entry ordering, just like an array does, but it provides a friendlier API to work with.

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

  let changeDue = cash - price;
  const change = {};

  // convert it to an object
  cid = Object.fromEntries(cid);

  let loop_guard = 100;
  for (const key of CASHVALUES.keys()) {
    const currentDenomination = CASHVALUES.get(key);
    while (changeDue >= currentDenomination) {
      if (cid[key] > 0) {
        cid[key] -= currentDenomination;
        cid[key] = Math.floor(cid[key] * 100) / 100;
        changeDue -= currentDenomination;
        changeDue = Math.floor(changeDue * 100) / 100;

        if (!change[key]) {
          change[key] = 0;
        }

        change[key]  = currentDenomination;
      } else {
        // insufficient cash
        // throw error maybe? I'll leave it to you
        break;
      }

      loop_guard--;
      if (!loop_guard) {
        // have I miss anything?
        break;
      }
    }
  }

  // convert them from object back to array, if you prefer
  return { change: Object.entries(change), cid: Object.entries(cid) };
}

const cid = [
  ["PENNY", 1.01],
  ["NICKEL", 2.05],
  ["DIME", 3.1],
  ["QUARTER", 4.25],
  ["ONE", 90],
  ["FIVE", 55],
  ["TEN", 20],
  ["TWENTY", 60],
  ["ONE HUNDRED", 100],
];

const result = checkCashRegister(15, 100, cid);
console.log(result);

CodePudding user response：

Arrays are passed by reference

You're changing your data when you don't intend to, probably because you are unaware that these values are magically linked.

Some programming languages give you explicit control over whether a variable is passed by reference or passed by value. Javascript does not: it passes all scalars by value, and all other types by reference. Scalars are "simple" values, like booleans, numbers, and strings. Pretty much everything else gets passed by value, whether you like it or not.

Consider this example:

I create an array that holds two sub-arrays
I "copy" the first sub-array into a new array
I modify the "copy" in the new array
The first array is changed!!

// create both arrays
let a1 = [
  ['one', 'alpha'], // notice is lowercase
  ['two', 'beta']
]
let a2 = []

// "copy" first sub-array into a2
a2.push(a1[0])

// modify the "copy"
a2[0][1] = 'ALPHA'

// look at the "original" sub-array in the first array
console.log(a1)
//> [ [ 'one', 'ALPHA' ], [ 'two', 'beta' ] ]
// notice that "ALPHA" is now uppercase
>

This happened because Javascript arrays are always passed by reference. Always.

One consequence of this is that this line will break your function on every even-numbered invocation:

cid.reverse()

You've got a hard-coded data structure that holds the monetary values of several denominations, and your algorithm requires that list to be sorted with highest-value first. Since you hard-coded the structure in ascending order, it seems like you should reverse the array. But .reverse mutates the original array, which means that the second time this function is called, it will re-reverse the array, placing the smallest-value currencies first!

The better solution is to hard-code your denominations once in the correct order. Or, create a copy of the that structure here and then do a proper sort based on value -- that way it doesn't matter what order the raw data structure is in.

The easiest way to copy an array is to slice it, but in this case that won't be good enough because the elements in the cid array are themselves arrays, which means they will be copied by reference.

The poor man's clone technique is to convert the data to a string and then back again, like so:

let clone = JSON.parse(JSON.stringify(original))

This will work for most kinds of data, and is reasonably fast, but there are cases where it's not good enough (some data types, like Dates, don't get revived by JSON.parse; circular references in the source data will throw). I think it's fine in your case.