Writing overcomplicated code and simplifying it

JavaScript
1

Hello there. I’ve recently finished JS Cash Register challenge and I think that I overcomplicated the code. What are the ways/tricks to simplify it? What are the patterns for simplification? Are there people that have/had the same problem?
My code is below:

    let cidmap = [];
    let temp = [];
    let changeinitial = cash - price;
    changeinitial = Number(changeinitial.toFixed(2));
    let change = cash - price;
    change = Number(change.toFixed(2));
    let temp2 = [];
    let allcash = cid.reduce((acc, currval) => acc + currval[1], 0);
    allcash = Number(allcash.toFixed(2));
    let open = {
        status: "OPEN",
        change: []
    };
    let insufficient = {
        status: "INSUFFICIENT_FUNDS",
        change: []
    };
    let closed = {
        status: "CLOSED",
        change: []
    };
    if (changeinitial > allcash) {
        return insufficient;
    } else if (changeinitial === allcash) {
        closed.change = cid;
        return closed; // new code
    } else
        cidmap = cid.map(function(cv) {
            let obj = {};
            switch (cv[0]) {
                case 'PENNY':
                    cv[0] = 0.01;
                    break;
                case 'NICKEL':
                    cv[0] = 0.05;
                    break;
                case 'DIME':
                    cv[0] = 0.1;
                    break;
                case 'QUARTER':
                    cv[0] = 0.25;
                    break;
                case 'ONE':
                    cv[0] = 1;
                    break;
                case 'FIVE':
                    cv[0] = 5;
                    break;
                case 'TEN':
                    cv[0] = 10;
                    break;
                case 'TWENTY':
                    cv[0] = 20;
                    break;
                case 'ONE HUNDRED':
                    cv[0] = 100;
                    break;
            }
            obj[cv[0]] = cv[1];
            return obj;
        }).slice().reverse();

    for (let i = 0; i < cidmap.length; i++) {
        for (var key in cidmap[i]) {
            if (cidmap[i].hasOwnProperty(key)) {
                if (change >= Number(key)) {
                    //var x = Number(key);
                    if (cidmap[i][key] !== 0) {
                        temp.push(Number(key));
                        cidmap[i][key] = cidmap[i][key] - Number(key);
                        change = change - Number(key);
                        change = change.toFixed(2)
                        i = i - 1;
                        continue;
                    }
                } else if (change === 0) {
                    break;
                }
            }
        }
    }
    if (temp.reduce((a, b) => a + b) < change) {
        return insufficient;
    } else
        temp.forEach(function(c, i) {
            switch (c) {
                case 0.01:
                    temp2.push(['PENNY', c]);
                    break;
                case 0.05:
                    temp2.push(['NICKEL', c]);
                    break;
                case 0.25:
                    temp2.push(['QUARTER', c]);
                    break;
                case 0.1:
                    temp2.push(['DIME', c]);
                    break;
                case 1:
                    temp2.push(['ONE', c]);
                    break;
                case 5:
                    temp2.push(['FIVE', c]);
                    break;
                case 10:
                    temp2.push(['TEN', c]);
                    break;
                case 20:
                    temp2.push(['TWENTY', c]);
                    break;
                case 100:
                    temp2.push(['ONE HUNDRED', c]);
                    break;

            }

        })

    for (let a = 1; a < temp2.length; a++) {
        if (temp2[a][0] === temp2[a - 1][0]) {
            temp2[a][1] = temp2[a][1] + temp2[a - 1][1];
            delete temp2[a - 1];
        }
    }

    temp2 = temp2.filter((c) => c !== "");
    open.change = temp2;
    return open;
}

checkCashRegister(19.5, 20, [["PENNY", 1.01], ["NICKEL", 2.05], ["DIME", 3.1], ["QUARTER", 4.25], ["ONE", 90], ["FIVE", 55], ["TEN", 20], ["TWENTY", 60], ["ONE HUNDRED", 100]]);
2

High five on that. This one was hard.

You probably could avoid one or both of those lengthy switch statements and some of the layers of your nested for loop / if statement block if you consider some things about the cid parameter passed to your function and the array of change you return back.

  1. The cid ( the drawer) is always presented in order so you don’t need to worry about drawer order - in fact you can use that to your advantage.
  2. The various denominations are always represented in cid even if the quantity is zero so you don’t have to worry about skipped denominations.

That would allow you to hard code the values of each denomination without packing cidmap with a switch statement. Depending on your logic it could be as simple as let values = [0.01, 0.05, 0.25 ...., 100.00]

That also allows you to loop through your drawer and the numeric face values in order with one simple for loop using the same index to access both. You know the order of the cid and you know the order of the face values. No need to check to check if properties exist or to convert strings to numbers. This would for example give you access to “PENNY”, 1.01 and 0.01 all with one index - cid[0] is [“PENNY”, 1.01] and values[0] is 0.01. (Remember that for loops can start at the end of an array and increment backwards by i-- too.)

  1. The change array in the returned object is expected in a certain order too.

While looping through cid in order calculating how much of each denomination you will be paying out you might as well pack the change array that you will be returning at the same time, also in order. That avoids the second switch statement and the subsequent sort.

That alone will save you both switch statements, an inner for loop, a couple of if statements and some sorting and mapping.

You might also reconsider declaring three separate return objects, open,insufficient and closed, at the top of your function - just declare one empty object when it is needed. The status and change properties can be added to that generic object as those values are determined.

1 Like
3

Thanks a lot for such profound answer! I will try to re-write my code using your tips.

4

Here’s my re-written code, what do you think of it?:

function checkCashRegister(price, cash, cid) {
    let cidmap = cid;
    let values = [0.01, 0.05, 0.1, 0.25, 1, 5, 10, 20, 100];
    let names = ['PENNY', 'NICKEL', 'DIME', 'QUARTER', 'DOLLAR', 'FIVE', 'TEN', 'TWENTY', 'ONE HUNDRED'];
    let temp = [];
    let x = 0;
    let changeinitial = cash - price;
    changeinitial = Number(changeinitial.toFixed(2));
    let change = cash - price;
    change = Number(change.toFixed(2));
    let allcash = cid.reduce((acc, currval) => acc + currval[1], 0);
    allcash = Number(allcash.toFixed(2));
    let result = {
        status: '',
        change: []
    };
    if (changeinitial > allcash) {
        result.status = 'INSUFFICIENT_FUNDS';
        return result;
    } else if (changeinitial === allcash) {
        result.change = cid;
        result.status = 'CLOSED';
        return result;
    } else
        for (let i = cid.length - 1; i >= 0; i--) {
            if (change >= values[i]) {
                if (cidmap[i][1] !== 0) {
                    temp.push([cidmap[i][0], values[i]]);
                    cidmap[i][1] = cidmap[i][1] - values[i];
                    change = change - values[i];
                    change = change.toFixed(2);
                    i++;
                    continue;
                }
            } else if (change === 0) {
                break;
            }
        }
    for (let a = 1; a < temp.length; a++) {
        if (temp[a][0] === temp[a - 1][0]) {
            temp[a][1] = temp[a][1] + temp[a - 1][1];
            delete temp[a - 1];
        }
    }
    temp = temp.filter((c) => c !== "");

    for (let i = 0; i < temp.length; i++) {
        x += temp[i][1];
    }
    if (x < change) {
        result.status = 'INSUFFICIENT_FUNDS';
        return result;
    }
    result.status = 'OPEN';
    result.change = temp;
    return result;

}

checkCashRegister(19.5, 20, [
    ["PENNY", 1.01],
    ["NICKEL", 2.05],
    ["DIME", 3.1],
    ["QUARTER", 4.25],
    ["ONE", 90],
    ["FIVE", 55],
    ["TEN", 20],
    ["TWENTY", 60],
    ["ONE HUNDRED", 100]
])
5

Awesome! That’s good work.

1 Like