Criticize my solution [Cash Register]

Programming JavaScript
1

Here is the problem is known as Cash Register.
I solved this and got a certificate :relaxed: :partying_face:
But, my code is pretty big.
Please suggest to me a better way to fix this, if you have some.
Here is my code below,

function checkCashRegister(price, cash, cid) {
  let x = cash-price;
  let budget = {};
  for(let i=0; i<cid.length; i++){
    budget[cid[i][0]] = cid[i][1];
  }
  let result =[];
  let eqObj = {status: "CLOSED", change: cid};
  let insaObj = {status: "INSUFFICIENT_FUNDS", change: []};
  let valBug = Object.values(budget);
  let sum = 0;
  for(let i=0; i<valBug.length; i++){
    sum += valBug[i];
  }
  if(x===sum){
    return eqObj;
  }else if(x>sum){
    return insaObj
  }else{
    if(x>100){
      let c = Math.min(Math.floor(x/100),Math.floor(budget["ONE HUNDRED"]/100));
      if(budget["ONE HUNDRED"] >= c && c!= 0){
        result.push(["ONE HUNDRED", c*100]);
        x = (x-(c*100)).toFixed(2);
      }else{
        return insaObj;
      }
    }
    if(x>20){
      let c = Math.min(Math.floor(x/20),Math.floor(budget["TWENTY"]/20));
      if(budget["TWENTY"] >= c && c!= 0){
        result.push(["TWENTY", c*20]);
        x = (x-(c*20)).toFixed(2);
      }else{
        return insaObj;
      }
    }
    if(x>10){
      let c = Math.min(Math.floor(x/10),Math.floor(budget["TEN"]/10));
      if(budget["TEN"] >= c && c!= 0){
        result.push(["TEN", c*10]);
        x = (x-(c*10)).toFixed(2);
      }else{
        return insaObj;
      }
    }
    if(x>5){
      let c = Math.min(Math.floor(x/5),Math.floor(budget["FIVE"]/5));
      if(budget["FIVE"] >= c && c!= 0){
        result.push(["FIVE", c*5]);
        x = (x-(c*5)).toFixed(2);
      }else{
        return insaObj;
      }
    }
    if(x>1){
      let c = Math.min(Math.floor(x/1),Math.floor(budget["ONE"]/1));
      if(budget["ONE"] >= c && c!= 0){
        result.push(["ONE", c*1]);
        x = (x-(c*1)).toFixed(2);
      }else{
        return insaObj;
      }
    }
    if(x>0.25){
      let c = Math.min(Math.floor(x/0.25),Math.floor(budget["QUARTER"]/0.25));
      if(budget["QUARTER"] >= c && c!= 0){
        result.push(["QUARTER", c*0.25]);
        x = (x-(c*0.25)).toFixed(2);
      }else{
        return insaObj;
      }
    }
    if(x>0.10){
      let c = Math.min(Math.floor(x/0.10),Math.floor(budget["DIME"]/0.10));
      if(budget["DIME"] >= c && c!= 0){
        result.push(["DIME", c*0.10]);
        x = (x-(c*0.10)).toFixed(2);
      }else{
        return insaObj;
      }
    }
    if(x>0.05){
      let c = Math.min(Math.floor(x/0.05),Math.floor(budget["NICKEL"]/0.05));
      if(budget["NICKEL"] >= c && c!= 0){
        result.push(["NICKEL", c*0.05]);
        x = (x -(c*0.05)).toFixed(2);
      }else{
        return insaObj;
      }
    }
    if(x > 0.01){
      let c = Math.min(Math.floor(x/0.01),Math.floor(budget["PENNY"]/0.01));
      if(budget["PENNY"] >= c*0.01  && c!= 0){
        result.push(["PENNY", c*0.01]);
        x = (x - (c*0.01)).toFixed(2);
      }else{
        return insaObj;
      }
    }
  }
  return {status: "OPEN", change: result};
}
2

Hi @Plabon_Kumer_Sarker !

Congrats on getting the javascript certification!

I moved your posts out of project feedback and into the javascript section because you might get more feedback here.

3

My main suggestion would be to fix some of these variable names.

One letter variable names are not that descriptive.
Especially when you get into building longer projects it gets confusing if you are using names like x,y,w,etc.

Picking more descriptive names will help people looking at your code know exactly what that variable is supposed to do.

4

You have a lot of repetition in your code, which is usually a sign that there’s room for optimisation. All those if blocks do essentially the same, only with different parameters, so I’d probably write a function which does all that, and call it with different arguments depending on the if condition.

5

@jsdisco Did you mean this? It’s really shorter than before :relaxed:

function checkCashRegister(price, cash, cid) {
  let x = cash-price;
  let budget = {};
  for(let i=0; i<cid.length; i++){
    budget[cid[i][0]] = cid[i][1];
  }
  let result =[];
  let eqObj = {status: "CLOSED", change: cid};
  let insaObj = {status: "INSUFFICIENT_FUNDS", change: []};
  let valBug = Object.values(budget);
  let keyBug = (Object.keys(budget)).reverse();
  let allCurr = [100,20,10,5,1,0.25,0.10,0.05,0.01];
  let sum = 0;
  for(let i=0; i<valBug.length; i++){
    sum += valBug[i];
  }
  if(x===sum){
    return eqObj;
  }else if(x>sum){
    return insaObj
  }else{
    for(let i=0; i<keyBug.length; i++){
      if(x>allCurr[i]){
        let c = Math.min(Math.floor(x/allCurr[i]),Math.floor(budget[keyBug[i]]/allCurr[i]));
        if(allCurr[i] === 0.01){
          if(budget[keyBug[i]] >= c*0.01 && c!= 0){
            result.push([keyBug[i], c*allCurr[i]]);
            x = (x-(c*allCurr[i])).toFixed(2);
          }else{
            return insaObj;
          }
        }else{
          if(budget[keyBug[i]] >= c && c!= 0){
            result.push([keyBug[i], c*allCurr[i]]);
            x = (x-(c*allCurr[i])).toFixed(2);
          }else{
            return insaObj;
          }
        }
      }
    }
  }
  return {status: "OPEN", change: result};
}
6

@jwilkins.oboe Thank you :hugs: I will try to use descriptive variable names next time.

7

Yeah that’s a lot more readable. As for variable names, I’d probably rename keyBug and valBug to keyBudg and valBudg or similar, so they don’t indicate that you’re collecting bugs here :smiley: