Cash Register code clean up

Code Feedback
1

I’m so, so, so SORRY to do a code dump.

I spent a long time trying to complete this and I need to step away from the pc. That being said. At some point I would like to refactor and improve the code.

I know my code is shocking and I humbly ask you to help review my code and offer suggestions for improvement.

Current Solution 
function checkCashRegister(price, cash, cid) {
  debugger
  let remaining = customRound(getRemaining(cash, price), 2)
  remaining = multiplyByOneHundred(remaining)
  let GLOBAL_COUNT = 0;
  
  let currentCid = deepCloneArray(cid).reverse()
  let fullCid = deepCloneArray(cid).reverse()
  let changeGivenArray = []

  while (remaining > 0) {

    let check = checkSufficientFunds(remaining, currentCid)

    if (check) {
      let [bestValue, count] = getBestValue(remaining, currentCid, GLOBAL_COUNT)
      GLOBAL_COUNT = count
      currentCid = deepCloneArray(cid).reverse().slice(GLOBAL_COUNT)


      let [r, subtracted] = subtractFromRemaining(remaining, currentCid, fullCid, GLOBAL_COUNT)
      remaining = r
      updateChangeGiven(changeGivenArray, bestValue, subtracted)
    }
    if (remaining == 0) {
      return {
        status: tillIsEmpty(fullCid) ? "CLOSED" : "OPEN",
        change: tillIsEmpty(fullCid) ? cid : changeGivenArray
      }
    }
    else {
      continue
    }
  }

  return {
    status: "INSUFFICIENT_FUNDS",
    change: []
  }

  function subtractFromRemaining(remaining, currentCid, fullCid) {
    const proposedSubtraction = multiplyByOneHundred(currentCid[0][1])
    const check = compareNum(proposedSubtraction, remaining)
    const currencyDefault = currencyLookup(currentCid[0][0])
    let subtracted;
    console.log("currentCid: ", currentCid[0][0], "fullCid", fullCid[GLOBAL_COUNT][0])
    const conditional = currentCid[0][0] == fullCid[GLOBAL_COUNT][0]
    console.log("This is the conditional check value: ", conditional);
    

    // if proposedSubtraction is greater than remaining
    if (check) {

      // https://stackoverflow.com/a/4228376/15592981
      const quotient = Math.floor(remaining / currencyDefault)
      subtracted = quotient * currencyDefault
      currentCid[0][1] -= subtracted / 100
      // will conditional always be true?
      if (conditional) {
        fullCid[0][1] -= subtracted / 100
      }
    }
    else {
      subtracted = multiplyByOneHundred(currentCid[0][1])

      currentCid[0][1] = 0
      if (conditional) {
        fullCid[GLOBAL_COUNT][1] = 0;
      }
    }
    remaining -= subtracted
    return [remaining, subtracted]

  }

  function checkSufficientFunds(remaining, cid) {
    // Iterate through cid to check if sufficient funds
    // check which is best value later 
    // let cidDictionary = new Map();

    let bool = false;
    let currencyInTill = 0

    for (let level of cid) {

      currencyInTill += customRound(
        multiplyByOneHundred(level[1]),
        2
      )

      let currencyType = currencyLookup(level[0])

      if (currencyType <= remaining && currencyInTill >= remaining) {
        bool = true
        break
      }
    }
    // return everything after loop finishes
    return bool
  }

  function getBestValue(remaining, currentCid, GLOBAL_COUNT) {
    // Work needs to be done here <-----

    // check which is best value. Iterate through cash in draw and check
    // if currencyDefault is less than or equal to remaining
    // if currencyInTill is greater than or equal to remaining


    let bestChangeFound = false
    let bestChangeValuePackage = undefined
    let count = GLOBAL_COUNT
    let currencyType;
    let currencyInTill;
    let check = checkSufficientFunds((remaining), currentCid)

    for (let [type, inTill] of currentCid) {

      currencyType = type
      currencyInTill = multiplyByOneHundred(inTill)
      let currencyDefault = currencyLookup(type)

      if (check && bestChangeFound == false && currencyDefault <= remaining && inTill > 0) {
        bestChangeFound = true
        break
      }
      count++
    }

    if (bestChangeFound == true) {
      bestChangeValuePackage = [currencyType, count]
    }

    return bestChangeValuePackage
  }

}

function customRound(num, precision) {
  const factor = Math.pow(10, precision);
  return Math.round(num * factor) / factor;
}

function compareNum(x, y) {
  // Can be used to check if cash is greater than price
  // Can be used to check if current iteration of cid is greater than change needed
  if (x > y) {
    return true;
  }
  else {
    return false;
  }
}

function getRemaining(x, y) {
  return x - y;
}

function multiplyByOneHundred(float) {
  // convert float to number
  return float * 100;
}

function currencyLookup(key) {
  // returns int from object
  const currencyObject = {
    'PENNY': 1,
    'NICKEL': 5,
    'DIME': 10,
    'QUARTER': 25,
    'ONE': 100,
    'FIVE': 500,
    'TEN': 1000,
    'TWENTY': 2000,
    'ONE HUNDRED': 10000
  }
  return currencyObject[key]
}

function updateChangeGiven(arr, key, subtracted) {
  arr.push([key, subtracted / 100])
}

/*
  https://dev.to/samanthaming/how-to-deep-clone-an-array-in-javascript-3cig
  Deep clone with recursion
  Tareq Al-Zubaidi
*/
function deepCloneArray(arr) {
  return arr.map(val => {
    return Array.isArray(val) ? deepCloneArray(val) : val
  })
}

function tillIsEmpty(cid) {
  return cid.every(till => {
    return till[1] == 0
  })
}

// console.log(checkCashRegister(3.26, 100, [["PENNY", 1.01], ["NICKEL", 2.05], ["DIME", 3.1], ["QUARTER", 4.25], ["ONE", 90], ["FIVE", 55], ["TEN", 20], ["TWENTY", 60], ["ONE HUNDRED", 100]]))

console.log(checkCashRegister(19.5, 20, [["PENNY", 0.5], ["NICKEL", 0], ["DIME", 0], ["QUARTER", 0], ["ONE", 0], ["FIVE", 0], ["TEN", 0], ["TWENTY", 0], ["ONE HUNDRED", 0]]))
2

Is it working now? That’s the natural point, which can be used to try and improve.

Since you call your code shocking, you might have something in mind as for what needs changes. Which parts would that be?

I’d recommend to keep changes small and implement them step-by-step. While checking if still everything works.

3

The code passes the tests. Good advice on keeping changes small.

My ick factors come from

  1. multiple iterations
  2. verbose code and uncertainty on if all variables I declared really needed to be declared
  3. A sense that the ‘design’ is pretty obtuse and I wonder if there are better abstractions.

I thought that having someone to talk about it might help.

4

Those are good points. I’d leave design and abstractions as last. It’s possible that during making things simpler some patters will still reveal themselves.

You can consider questions below

    • What values are iterated multiple times?
    • Is that necessary? What changes in the values between iterations?
    • If yes, what would need to change to not require multiple iterations?
    • Function names, variable names - is it clear what they do?
    • Are variables declared with right keyword - let/const? If variable is not changed, no reason to not declare it with const. Similarly when it’s later only mutated.
    • Are all variables necessary?
    • Are all functions necessary? Are they used through the code multiple times, or do they allow to keep specific complexity contained in a function, to warrant existence of specific function?
1 Like