3
\$\begingroup\$

I want to run the function addItem() for every item. Multiple items will belong to the same slot (category). For each category I have to run the function with the same slot. I don't want to manually write down the slot for each and every item, because there could be 100's for each category.

The goal is to get the best result in terms of: readability, maintainability and effort to add items.

Here is my current code:

let equipment = {
    head: {},
    cape: {},
    neck: {},
    ammunition: {},
    weapon: {},
    body: {},
    shield: {},
    legs: {},
    hand: {},
    feet: {},
    ring: {}
};

function createShortName(name) {
    return name.toLowerCase().replace(/\s/g, '');
}

function addItem(slot, name, cost, img, income=0, atk_bonus=0, str_bonus=0, def_bonus=0, rngd_bonus=0, mage_bonus=0) {

    let newItem = {
        slot: slot,
        name: name,
        cost: cost,
        img: img,
        income: income,
        atk_bonus: atk_bonus,
        str_bonus: str_bonus,
        def_bonus: def_bonus,
        rngd_bonus: rngd_bonus,
        mage_bonus: mage_bonus
    }

    equipment[slot][createShortName(name)] = newItem;
}

let currentSlot = 'head';
addItem(currentSlot, 'head_item_1', 100, 'img');
addItem(currentSlot, 'head_item_2', 200, 'img');

currentSlot = 'cape';
addItem(currentSlot, 'cape_item_1', 100, 'img');

currentSlot = 'neck';
addItem(currentSlot, 'neck_item_1', 100, 'img');
addItem(currentSlot, 'neck_item_2', 200, 'img');
addItem(currentSlot, 'neck_item_3', 400, 'img');
addItem(currentSlot, 'neck_item_4', 800, 'img');

// etc. etc. etc.

As you can see, I add items to the equipment object at the bottom of the code. Is there a better approach?

\$\endgroup\$
4
  • \$\begingroup\$ For what reason don't you want to write down the currentSlot as a string literal? It would make the code even easier to read since then the calls to addItem would have only literal parameters. As a reader I could then focus on the meaning of the values, without having to search half of them in the preceding lines of code. \$\endgroup\$ – Roland Illig Jun 17 '19 at 15:09
  • \$\begingroup\$ @RolandIllig I have 100's of items for each slot, I prefer not to write (copy+paste) it every single time and I am looking for the best solution in terms of effort, readability, and maintainability. \$\endgroup\$ – Daan Jun 17 '19 at 15:13
  • \$\begingroup\$ What about removing all this data from the code, and putting it in a spreadsheet instead? That allows for quick editing, and the main code would just have to read the spreadsheet data. Also, your current code does not demonstrate how the optional parameters of `addItem´ are used. \$\endgroup\$ – Roland Illig Jun 17 '19 at 15:20
  • \$\begingroup\$ @RolandIllig The optional parameters are used exactly the same, for example: addItem(currentSlot, 'neck_item_2', 200, 'img', 1, 5, 10, 0, 2, 4); It's simply adding all stats to the object and then I can read it on another file with equipment.head.head_item_1 for example. Or I can retrieve all head items by equipment.head. How would putting it in a spreadsheet work? \$\endgroup\$ – Daan Jun 17 '19 at 18:02
2
\$\begingroup\$

With regards to the last couple of lines, one thing you could do from a functional perspective is to bind the first argument like so:

function bindFirst(func, firstArgument) {
    return function(...arguments) {
        return func(firstArgument, ...arguments);
    }
}

const addToHead = bindFirst(addItem, 'head');

addToHead('head_item_1', 100, 'img');
...

You could also use data structure like a Map and loop through it:

let newItems = new Map([
    [
        'head',
        [{
            name: 'head_item_1',
            cost: 100,
            img: 'img'
        },
        {
            name: 'head_item_1',
            cost: 100,
            img: 'img'
        }]
    ], [
        'cape',
        [{
            name: 'cape_item_1',
            cost: 100,
            img: 'img'
        },
        {
            name: 'cape_item_1',
            cost: 200,
            img: 'img'
        }]
    ]
]);

items.forEach((items, slot) => {
    const addToSlot = bindFirst(addItem, slot);
    items.forEach(item => {
        addToSlot(item.name, item.cost, item.img);
    });
});

Have you considered that maybe equipment and item should be classes? A class called Equipment could have a method like addItem(slot, item) where item is an object of type Item, which has a constructor with a similar signature to your addItem function.

\$\endgroup\$
2
  • \$\begingroup\$ The reason why I did a function was because all these slot, name, cost, img, income, atk_bonus, str_bonus, def_bonus, rngd_bonus, mage_bonus parameters are possible for the items. If I had to write all parameters for each item, it would be even more work. \$\endgroup\$ – Daan Jun 17 '19 at 18:00
  • \$\begingroup\$ If I have an object like let o = {x: 1}, o.y returns undefined. If you pass undefined to a function, the default value takes its place. So you can replace the line with addToSlot(item.name, item.cost, item.img, item.income, ...) and it will behave fine even for the items that don't have income supplied. \$\endgroup\$ – Ian Jun 17 '19 at 19:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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