1
\$\begingroup\$

I'm new at coding and I'm learning it on freeCodeCamp, that's the most hardest problem that I have already do at that site. After finishing it (after I vibrated as if I had won a world competition of something) I go straight to the internet to visualize another resolutions and try to understand and get better on coding. After all anything I can speak is: my solve seems to different of the others that are mainly searched on google and I want you to try to give me tips to have a better solution.

Initially let me explain what I think about my code: Seing it I can tell that it (actually) only solves the examples from the free code camp but if I repeat the if-statement at the end I will solve any problems but at the same think I think that it will turn on an enormous code and maybe unnecessary. What I want to have some overalls about how turn it smaller and more legible.

function checkCashRegister(price, cash, cid) {
  const monetary = [["PENNY", 0.01], ["NICKEL", 0.05], ["DIME", 0.1], ["QUARTER", 0.25], ["ONE", 1], ["FIVE", 5], ["TEN", 10], ["TWENTY", 20], ["ONE HUNDRED", 100]];
  const newObj = {"status": [], "change": []}
  const newArr2 = [];
  const newArr1 = [];
  const newArr0 = [];
  const newArr = [];
  var inCash = 0;
  var dif = cash - price;
  let result = parseFloat(dif)
  let newValue = 0;
  let count = 0;

  for (let i = 0; i < cid.length; i++) {
    // Filter for cash that can be used as return
    if (cid[i][1] < cash) {
      newArr.push(cid[i])
    }
    inCash += cid[i][1]
    newArr0.push(cid[i])
  }

  if (inCash === dif) {
    newObj.status = "CLOSED";
    newObj.change = newArr0; 

  } else if (inCash < price) {
    newObj.status = "INSUFFICIENT_FUNDS";

  } else {
    var open = [];
    for (let i = newArr.length - 1; i > -1; i--) {
      if (newArr[i][1] >= monetary[i][1]) {

        // Checker if the value is "exactly" then input it
        if (dif % (monetary[i][1]) == 0) {
          let newArr1 = [];
          let num = dif/monetary[i][1];
          newArr1.push(monetary[i][0], + num*monetary[i][1])
          newObj.change = [newArr1];

        } else {  
          let value = parseFloat(cid[i][1])
          result -= value;
          newArr1.push(result.toFixed(2))

      }
    }
    newObj.status = "OPEN";
  }

    // Put the positive values in newArr2
    let index = (newArr1.length - newArr1.filter((item) => item < 0).length)
    for (let i = 0; i < index; i++) {
      newArr2.push(newArr[newArr.length - 1 - i])

    } 

    // After update newArr2; this will be the remanescent change => console.log(newArr1[index - 1])
    // And the next monetary value to use => console.log(cid[cid.length - 2 - index])

    // Using if to reduce the value to the lowest possible value
    let count = 0;
    newValue = newArr1[index - 1];

    // Loop to check how many 5 bills will need
    if (newArr1[index - 1] - 5 > 5) {
      while (newArr1[index - 1] > 5) {
        newArr1[index - 1] -= 5;
        count++;
      }
        newArr2.push([cid[cid.length - 2 - index][0], + 5*(1 + (count - 1))])
        newValue -= 5 + 5*(count - 1);
    }
    
    // Reset the count and repeat the process to separate the bills amount...
    count = 0;
    if (newValue > 1) {
      newValue -= 1 + 1*(count - 1);
      while (newArr1[index - 1] > 1) {
        newArr1[index - 1] -= 1;
        count++;
      }
      newArr2.push([cid[cid.length - 3 - index][0], + 1*(1 + (count - 1))])
      newValue -= 1 + 1*(count - 1);
    }

    count = 0;
    if (newValue > 0.25) {
      newValue -= 0.25 + 0.25*(count - 1);
      while (newArr1[index - 1] > 0.25) {
        newArr1[index - 1] -= 0.25;
        count++;
      }
      newArr2.push([cid[cid.length - 4 - index][0], + 0.25*(1 + (count - 1))])
      newValue -= 0.25 + 0.25*(count - 1);
    } 

    count = 0;
    if (newValue > 0.1) {
      newValue -= 0.1 + 0.1*(count - 1);
      while (newArr1[index - 1] > 0.1) {
        newArr1[index - 1] -= 0.10;
        count++;
      }
      newArr2.push([cid[cid.length - 5 - index][0], + 0.1*(1 + (count - 1))])
      newValue -= 0.1 + 0.1*(count - 1);
    }

    count = 0;
    if (newValue > 0.05) {
      newValue -= 0.05 + 0.05*(count - 1);
      while (newArr1[index - 1] > 0.05) {
        newArr1[index - 1] -= 0.05;
        count++;
      }
      newArr2.push([cid[cid.length - 6 - index][0], + 0.05*(1 + (count - 1))])
      newValue -= 0.05 + 0.05*(count - 1);
    }

    count = 0;
    if (newValue >= 0 || newValue < 0.05) {
      newValue -= 0.01 + 0.01*(count - 1);
      while (newArr1[index - 1] >= 0) {
        newArr1[index - 1] -= 0.01;
        count++;
      }
      newArr2.push([cid[cid.length - 7 - index][0], + 0.01*(1 + (count - 1))])
      newValue -= 0.01 + 0.01*(count - 1);
    }

    if (newArr2.length > index) {
      newObj.change = newArr2;
    }

  }
  console.log(newObj)
  return newObj;
}

function checkCashRegister(price, cash, cid) {
  const monetary = [["PENNY", 0.01], ["NICKEL", 0.05], ["DIME", 0.1], ["QUARTER", 0.25], ["ONE", 1], ["FIVE", 5], ["TEN", 10], ["TWENTY", 20], ["ONE HUNDRED", 100]];
  const newObj = {"status": [], "change": []}
  const newArr2 = [];
  const newArr1 = [];
  const newArr0 = [];
  const newArr = [];
  var inCash = 0;
  var dif = cash - price;
  let result = parseFloat(dif)
  let newValue = 0;
  let count = 0;

  for (let i = 0; i < cid.length; i++) {
    // Filter for cash that can be used as return
    if (cid[i][1] < cash) {
      newArr.push(cid[i])
    }
    inCash += cid[i][1]
    newArr0.push(cid[i])
  }

  if (inCash === dif) {
    newObj.status = "CLOSED";
    newObj.change = newArr0; 

  } else if (inCash < price) {
    newObj.status = "INSUFFICIENT_FUNDS";

  } else {
    var open = [];
    for (let i = newArr.length - 1; i > -1; i--) {
      if (newArr[i][1] >= monetary[i][1]) {

        // Checker if the value is "exactly" then input it
        if (dif % (monetary[i][1]) == 0) {
          let newArr1 = [];
          let num = dif/monetary[i][1];
          newArr1.push(monetary[i][0], + num*monetary[i][1])
          newObj.change = [newArr1];

        } else {  
          let value = parseFloat(cid[i][1])
          result -= value;
          newArr1.push(result.toFixed(2))

      }
    }
    newObj.status = "OPEN";
  }

    // Put the positive values in newArr2
    let index = (newArr1.length - newArr1.filter((item) => item < 0).length)
    for (let i = 0; i < index; i++) {
      newArr2.push(newArr[newArr.length - 1 - i])

    } 

    // After update newArr2; this will be the remanescent change => console.log(newArr1[index - 1])
    // And the next monetary value to use => console.log(cid[cid.length - 2 - index])

    // Using if to reduce the value to the lowest possible value
    let count = 0;
    newValue = newArr1[index - 1];

    // Loop to check how many 5 bills will need
    if (newArr1[index - 1] - 5 > 5) {
      while (newArr1[index - 1] > 5) {
        newArr1[index - 1] -= 5;
        count++;
      }
        newArr2.push([cid[cid.length - 2 - index][0], + 5*(1 + (count - 1))])
        newValue -= 5 + 5*(count - 1);
    }
    
    // Reset the count and repeat the process to separate the bills amount...
    count = 0;
    if (newValue > 1) {
      newValue -= 1 + 1*(count - 1);
      while (newArr1[index - 1] > 1) {
        newArr1[index - 1] -= 1;
        count++;
      }
      newArr2.push([cid[cid.length - 3 - index][0], + 1*(1 + (count - 1))])
      newValue -= 1 + 1*(count - 1);
    }

    count = 0;
    if (newValue > 0.25) {
      newValue -= 0.25 + 0.25*(count - 1);
      while (newArr1[index - 1] > 0.25) {
        newArr1[index - 1] -= 0.25;
        count++;
      }
      newArr2.push([cid[cid.length - 4 - index][0], + 0.25*(1 + (count - 1))])
      newValue -= 0.25 + 0.25*(count - 1);
    } 

    count = 0;
    if (newValue > 0.1) {
      newValue -= 0.1 + 0.1*(count - 1);
      while (newArr1[index - 1] > 0.1) {
        newArr1[index - 1] -= 0.10;
        count++;
      }
      newArr2.push([cid[cid.length - 5 - index][0], + 0.1*(1 + (count - 1))])
      newValue -= 0.1 + 0.1*(count - 1);
    }

    count = 0;
    if (newValue > 0.05) {
      newValue -= 0.05 + 0.05*(count - 1);
      while (newArr1[index - 1] > 0.05) {
        newArr1[index - 1] -= 0.05;
        count++;
      }
      newArr2.push([cid[cid.length - 6 - index][0], + 0.05*(1 + (count - 1))])
      newValue -= 0.05 + 0.05*(count - 1);
    }

    count = 0;
    if (newValue >= 0 || newValue < 0.05) {
      newValue -= 0.01 + 0.01*(count - 1);
      while (newArr1[index - 1] >= 0) {
        newArr1[index - 1] -= 0.01;
        count++;
      }
      newArr2.push([cid[cid.length - 7 - index][0], + 0.01*(1 + (count - 1))])
      newValue -= 0.01 + 0.01*(count - 1);
    }

    if (newArr2.length > index) {
      newObj.change = newArr2;
    }

  }
  console.log(newObj)
  return newObj;
}

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]]);

Thanks for trying to understand it xD

Improve this question
\$\endgroup\$
2
  • \$\begingroup\$ I would start by creating a habit of trying to come up with good Variable names as this will help u a lot down the line when debugging. It also helps other people to read it in Situations like this. Instead of using newValue or newArr etc. what does this variable hold? bills? coins? \$\endgroup\$
    – George R
    Sep 17, 2021 at 13:05
  • \$\begingroup\$ @LuciferUchiha I will keep it in mind on my next projects! \$\endgroup\$
    – Breno Lira
    Sep 17, 2021 at 13:22

1 Answer 1

Reset to default
0
\$\begingroup\$

First thing that stands out is that your function is 140 line long, which is just too much. You have put the comments in, which is good, but it would be simpler if you extracted that piece of code into a function.

This for loop does three things: Filters the cash for return, calculates the total in register and copies the register. You can split it in three different functions.

  for (let i = 0; i < cid.length; i++) {
    // Filter for cash that can be used as return
    if (cid[i][1] < cash) {
      newArr.push(cid[i])
    }
    totalCash += cid[i][1]
    newArr0.push(cid[i])
  }

What you end up with is this:

  function calculateTotalInRegister(cashRegister) {
      let total = 0
      for (let i = 0; i < cashRegister.length; i++) {
          total += cashRegister[i][1]
      }

     return total
  }

  function cashUsedToReturn(cid) {
    const forReturn = []
    for (let i = 0; i < cid.length; i++) {
      if (cid[i][1] < cash) {
        forReturn.push(cid[i])
      }
    }

    return forReturn
  }

  function copyRegister(cid) {
    const copy = []
    for (let i = 0; i < cid.length; i++) {
      copy.push(cid[i])
    }

    return copy
  }

...

  const newArr0 = copyRegister(cid);
  const newArr = cashUsedToReturn(cid);
  var inCash = calculateTotalInRegister(cid);

Then you have these checks:

  if (inCash === dif) {
    newObj.status = "CLOSED";
    newObj.change = newArr0;
  } else if (inCash < price) {
    newObj.status = "INSUFFICIENT_FUNDS";
  }

What is weird about this is that you pass in the register and return the status of the register and change, but not the new state of the register. Also, I'm pretty sure that second condition is a bug. It should be:

  else if (inCash < dif) {
    newObj.status = "INSUFFICIENT_FUNDS";
  }

Anyway, you again seem to be doing two things: getting the new register status and calculating the change. So let's split this into two more functions.

  function getStatus(inCash, dif) {
    if (inCash === dif) {
      return "CLOSED";
    } else if (inCash < dif) {
      return "INSUFFICIENT_FUNDS";
    } else {
      return "OPEN"
    }
  }

  function calculateChange(inCash, dif, newArr, newArr0) {
    ...
  }

  ...

  newObj.status = getStatus(inCash, dif)

This leaves us with the original function that looks like this:

function checkCashRegister(price, cash, cid) {
  const monetary = [["PENNY", 0.01], ["NICKEL", 0.05], ["DIME", 0.1], ["QUARTER", 0.25], ["ONE", 1], ["FIVE", 5], ["TEN", 10], ["TWENTY", 20], ["ONE HUNDRED", 100]];
  const newObj = {"status": [], "change": []}
  const newArr2 = [];
  const newArr1 = [];
  const newArr0 = copyRegister(cid);
  const newArr = cashUsedToReturn(cid);
  var inCash = calculateTotalInRegister(cid);
  var dif = cash - price;
  let result = parseFloat(dif)
  let newValue = 0;
  let count = 0;

  newObj.status = getStatus(inCash, dif)
  newObj.change = calculateChange(inCash, dif, newArr, newArr0)

  console.log(newObj)
  return newObj;
}

Before looking into calculateChange, it is about time that we rename the variables. Monetary is kind of fine, the other ones are not.

Parameter cid should be cashRegister.

newObj should be named result with register.status and change properties like so:

    const result = {
        register: {
            status: getStatus(inCash, dif)
        },
        change: calculateChange(inCash, dif, newArr, newArr0)
    }

newArr1, newArr2, result, newValue and count can be left out, newArr0 -> cashRegisterCopy, newArr -> cashForReturn, inCash -> totalInRegister, dif -> amountToReturn.

The original function is now just this:

function checkCashRegister(price, cash, cashRegister) {
    const monetary = [["PENNY", 0.01], ["NICKEL", 0.05], ["DIME", 0.1], ["QUARTER", 0.25], ["ONE", 1], ["FIVE", 5], ["TEN", 10], ["TWENTY", 20], ["ONE HUNDRED", 100]];
    const cashRegisterCopy = copyRegister(cashRegister);
    const cashForReturn = cashUsedToReturn(cashRegister);
    var totalInRegister = calculateTotalInRegister(cashRegister);
    var amountToReturn = cash - price;

    const result = {
        register: {
            status: getStatus(totalInRegister, amountToReturn)
        },
        change: calculateChange(totalInRegister, amountToReturn, cashForReturn, cashRegisterCopy)
    }

    console.log(result)

    return result;
}

It turns out that cashRegisterCopy is unnecessary. You could just pass the cash register directly. Which brings me to the main issue with the way you coded this - instead of passing cash register everywhere, you could create a CashRegister class.

Another thing is the use of magic strings everywhere (PENNY, NICKEL, etc.). Turn them into constants.

I would also use a different data structure, just so the order doesn't matter.

One more tip when working on monetary apps: do not use floats, but use integers. If you want to display the amount you can just divide the amount by 100.

If you do the refactoring to the class, the code will look something like this:

class CashRegister {
  constructor(money) { this.money = money }

  // status of the register
  get status() { ... }

  // total amount of money in register
  get total() { ... }

  // returns the change and subtracts from this.money
  returnChange(price, cashGiven) { ... }
}

const cashRegiser = new CashRegister({
    PENNY: 101,
    NICKEL 205,
    DIME: 310,
    QUARTER: 425,
    ONE: 9000,
    FIVE: 5500,
    TEN: 2000,
    TWENTY: 6000,
    HUNDRED: 10000
})

const change = cashRegister.returnChange(3.26, 100)
const status = cashRegister.status

As for the algorithm for returning change, you can watch some videos on change making problem in javascript on youtube. Maybe instead of passing the amount of money you have for each coin, you can pass the number of coins/bills you have in the register and work with that :)

Improve this answer
\$\endgroup\$
1
  • \$\begingroup\$ I know that isn't the way I should use the comments BUT I really need to thank you for your amazing review. I will make sure to read all of this and try to absorb the vital part of it. :) \$\endgroup\$
    – Breno Lira
    Sep 19, 2021 at 13:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.