Is properly structured code worth it?

JavaScript
1

I thought that I would get fancy and use filter and reduce to to combine checked boxes and options together into one object structure.

The checked items are: [“capitol”, “caves”, “island”]
and the radio option for each is Rules or Odds: [“Rules”, “Odds”, “Rules”]

I want to combine these two together to be one object with the item names as object keys:

{
    "capitol": "Rules",
    "caves": "Odds",
    "island": Rules"
}

I want to filter only for the checked items so that’s a filter, then use reduce.

So, a filter wants a function to be run with it, and we’ll need getRuleType in the reduce function:

        function isChecked(expn) {
            return document.querySelector("#js-" + expn).checked;
        }
        function getRuleType(expn) {
            var radio = document.querySelector("#js-" + expn + "Rules");
            return radio.checked
                ? "rules"
                : "odds";
        }

which we can then use with a filter and a reduce function:

        return allMinis
            .filter(isChecked)
            .reduce(function combine(minis, expn) {
                return Object.defineProperty(minis, expn, getRuleType(expn));
            }, {});

But no, it’s not that easy because defineProperty doesn’t work that way. Instead you must give is all sorts of other attributes instead:

        return allMinis
            .filter(isChecked)
            .reduce(function combine(minis, expn) {
                return Object.defineProperty(minis, expn, {
                    value: getRuleType(expn),
                    writable: true,
                    enumerable: true,
                    configurable: true
                });
            }, {});

And now all of the code for this section is just looking like it’s far too much for what it’s supposed to achieve.

    function getMinis(allMinis) {
        function isChecked(expn) {
            return document.querySelector("#js-" + expn).checked;
        }
        function getRuleType(expn) {
            var radio = document.querySelector("#js-" + expn + "Rules");
            return radio.checked
                ? "rules"
                : "odds";
        }
        return allMinis
            .filter(isChecked)
            .reduce(function combine(minis, expn) {
                return Object.defineProperty(minis, expn, {
                    value: getRuleType(expn),
                    writable: true,
                    enumerable: true,
                    configurable: true
                });
            }, {});
    }

Some of this is so that the code is easily maintainable, but why does it all have to be so complex?

The alternative is nested code that looks like this:

    function getMinis(allMinis) {
        var formEls = document.querySelector("#expansions").elements;
        return allMinis.reduce(function (minis, expn) {
            if (formEls[expn].checked) {
                minis[expn] = formEls[expn + "Choice"].value;
            }
            return minis;
        }, {});
    }

I can’t help but look at the so-called badly nested code comparing it with the “better” more structured code above it, and ask is it all worth it?

2

Why not going for the simple

minis[expn] = getRuleType(expn)

or maybe even a Map?

Object.defineProperty() is for creating fine-tuned properties, not simple maps.

3

I did start doing that, but then wanted a way that would let me return the end-result straight out of the reduce function.
After which the inspiration for this post struck.

4 
minis[expn] = getRuleType(expn)
return minis

or

return minis[expn] = getRuleType(expn), minis

or

return map.set(expn, getRuleType(expn))
5

Those are interesting alternatives.
The top one smells quite badly though, and the bottom one invokes a map which seems to make things even more complex.

6

You could use two lines instead of one (see updated post). Whether the comma operator is good practice, depends on the personal view.

7

What’s wrong with the object type that is intended for the purpose depicted in the code?

You could also use some functional libraries that reduce your lines even further.

8

A hopeful compromise has been reached with the following:

    function getMinis(allMinis) {
        function isChecked(expn) {
            return document.querySelector(expn).checked;
        }
        function getRule(expn) {
            var formEls = document.querySelector("#expansions").elements;
            return formEls[expn + "Choice"].value;
        }
        function addRule(map, expn) {
            return map.set(expn, getRule(expn));
        }
        return allMinis
            .filter(isChecked)
            .reduce(addRule, new Map());
    }

If anyone’s interested, I’m working on an update to the old code of Kingdom Builder tabletopgame randomizer.

9

This may contain a NodeList if the form control name is not unique.

10

That will never occur, but thanks for the heads-up.

11

You can also use a Closure for formEls so you don’t have to hit the DOM for the form element on each iteration cycle.

12

Three of the expansions are mini expansions, and I doubt that more for the boardgame are ever going to be made.

All possible performance hits were accounted for by the time my first word was expressed. Is that good justification?

13

There are probably things that I would attempt differently (e.g. getting all involved elements at the beginning … I’m just a fan of Functional Programming) but I don’t have enough context to say something specific.

14

Why am I fighting on changing this code?

        function getRule(expn) {
            var formEls = document.querySelector("#expansions").elements;
            return formEls[expn + "Choice"].value;
        }

Moving the formEls variable up above the function is easily done, but there are other functions there too, and the formEls variable will be all by itself up there.

        var formEls = document.querySelector("#expansions").elements;
        ...
        function getRule(expn) {
            return formEls[expn + "Choice"].value;
        }
        ...
        return ....

Unless . . . is this a stepping-stone to a further improvement?

15

I think the object spread operator would do the trick here… and it’s officially in the specs now. :-)

return { ...minis, [expn]: getRuleType(expn) }

Or using Object.assign() so you don’t have to create a copy of the accumulator in each iteration:

return Object.assign(minis, { [expn]: getRuleType(expn) })
16

Those are amazingly useful solutions, and correct me if I’m wrong but doesn’t that then guarantee that the code cannot ever run on Internet Explorer?

17

Hehe… but well no, both above lines can safely and completely be polyfilled / transpiled to code that IE understands (as opposed to, say, Symbols or any feature not yet included in the standard).

18

Which polyfilling technique allows Internet Explorer to run that code?

19

you may need more polyfills than that one…

20

There’s one polyfill required for Object.assign(), e.g. from core-js, or the one from the MDN @Dormilich mentioned. The other parts however require syntax transforms (transpilation); for example with babel the first line looks like this.

As you can see, this basically generates the sort of code that you were trying to avoid in your OP, while you can concentrate on writing concise, contemporary JS. And modern browsers don’t even have to load the generated overhead thanks to the type="module" / nomodule fallback.

PS: If you mean running that very code, that’d be possible by loading babel-standalone – although that’s certainly not suitable for production.