5
\$\begingroup\$

I am learning object oriented JavaScript and come up with below exercise. There are two multi-select lists, one is source and other is target. User can move the item from source to target and vice versa.

I tried to encapsulate the functionality in an object to initialize it any number of times on the page. However, I feel somehow I am tightly coupling the UI with JS code. Meaning, I am accessing the click events of buttons inside the defined object.

EDIT Any inputs on the OOPs implementation in Javascript. Is this example a valid use case of OOPs.

<html>
<head>
</head>
<body>
    <table>
        <tr>
            <td>
                <select multiple id="source1">
                    <option> Milk </option>
                    <option> Vegetables </option>
                    <option> Bread </option>
                </select>
            </td>
            <td>
                <div style="display:inline;vertical-align:top;">
                    <button id="shift1">>></button> <br /><br />
                    <button id="rshift1"><<</button>
                </div>
            </td>
            <td><select multiple id="target1"></select></td>
        </tr>
    </table>

    <table>
        <tr>
            <td>
                <select multiple id="source2">
                    <option> Milk </option>
                    <option> Vegetables </option>
                    <option> Bread </option>
                </select>
            </td>
            <td>
                <div style="display:inline;vertical-align:top;">
                    <button id="shift2">>></button> <br /><br />
                    <button id="rshift2"><<</button>
                </div>
            </td>
            <td><select multiple id="target2"></select></td>
        </tr>
    </table>

    <button onclick="submit()">Submit</button>

    <script>
        var hybridSelector = function (source, target, shift, rshift) {
            var ddlSource = source;
            var ddlTarget = target;
            var btnShift = shift;
            var btnRShift = rshift;

            btnShift.addEventListener("click", function () {
                var selectedItems = getSelectOptions(source);

                if (selectedItems) {
                    for (var i = 0; i < selectedItems.length; i++) {
                        var option = new Option(selectedItems[i].text, selectedItems[i].text);
                        ddlTarget.appendChild(option);
                        selectedItems[i].remove();
                    }
                }
            });

            btnRShift.addEventListener("click", function () {
                var selectedItems = getSelectOptions(target);
                if (selectedItems) {
                    for (var i = 0; i < selectedItems.length; i++) {
                        var option = new Option(selectedItems[i].text, selectedItems[i].text);
                        ddlSource.appendChild(option);
                        selectedItems[i].remove();

                    }
                }
            });

            function getSelectOptions(select) {
                var result = [];
                var options = select.options;
                var opt;

                for (var i = 0, iLen = options.length; i < iLen; i++) {
                    opt = options[i];

                    if (opt.selected) {
                        result.push(opt);
                    }
                }
                return result;
            }

            return ddlTarget.options;
        };

        //can instantiate as many as i want
        var hybridSelector1 = new hybridSelector(document.getElementById('source1'), document.getElementById('target1'), document.getElementById('shift1'), document.getElementById('rshift1'));
        var hybridSelector2 = new hybridSelector(document.getElementById('source2'), document.getElementById('target2'), document.getElementById('shift2'), document.getElementById('rshift2'));

        function submit() {
            var options1 = hybridSelector1;
            var options2 = hybridSelector2;

            debugger;
            return false;
        }

    </script>
</body>
</html>

\$\endgroup\$
1
\$\begingroup\$

First of all some basic concerns about markup to voice

  • Tables? Is this actually a table? I don't know what your going to use this for but unless you are displaying data please do not use the tables for layout. see

    Tables existed in HTML for one reason: To display tabular data

  • Similarly the <br /> tag should never be used for layout. see

    <br /> is meant to be used as to represent a break in said content

The Code

Reduce Complexity of instantiation

The first thing I see is that for every one of these you wan't to instantiate you have to provide a list of elements. However the dom structure you've made doesn't seem to change. I would provide the elem ref to the most parent element and then let the code use that element to find what it needs based on structured markup it can depend on.

var hybridSelector = function (source, target, shift, rshift) {
    var ddlSource = source;
    var ddlTarget = target;
    var btnShift = shift;
    var btnRShift = rshift;
    ...
}

could become something like this

var hybridSelector = function(hybridContainerElem) {
    var ddlSource = hybridContainerElem.childNodes....
     var ddlSource = hybridContainerElem.childNodes....
     var btnShift = hybridContainerElem.childNodes....
     var btnRShift = hybridContainerElem.childNodes....
     ...
}

This also means you don't have to name elements with suffix 1, 2, etc. You only have to have a single unique container name for each hybrid select.

Reduce Event Listners

Your currently listening to each button individually but they are the only elems in their parent container so you could easily listen to any clicks on that container, look at the details of the event, then delegate it to the correct function to handle it.

btnContainer.addEventListener("click", function (e) {
    if (e.target.id === "rshift") {
        onRightShiftButtonClick();
    } else if (e.target.id === "shift") {
        onLeftShiftButtonClick();
    }
}

If you have 12 hybrid selects your event listeners just went from 24 down to 12.

Combine The Code To Move Selected Choices

Your code in the click callbacks is nearly identical. This should be a red flag for anything. You only have two vars that are unique, I would look at the event and decide how to set them, then run the code. Lets say you refactored the above further to just be.

btnContainer.addEventListener("click", moveSelections);

function moveSelections(e) {
    var source, target;

    if (e.target.id === "rshift") {
        source = target;
        target = source;
    } else if (e.target.id === "shift") {
        source = source;
        target = target;
    } else {
        return; // something other than a btn was clicked
    }

    var selectedItems = getSelectOptions(source);

    if (selectedItems) {
        for (var i = 0; i < selectedItems.length; i++) {
            var option = new Option(selectedItems[i].text, selectedItems[i].text);
            target.appendChild(option);
            selectedItems[i].remove();
         }
     }
}

More Concise Naming

In the last code snippet it becomes obvious the naming is a little vague, I'm still not sure what the 'ddl' means prefixing source and target (I might just be blind to something though). I would definitely go through and make some name changes to be a little more specific.

Hope you can find these useful!

\$\endgroup\$
  • \$\begingroup\$ Thank you, its helpful. Any inputs on OOPs implementation in Javacript? I am trying to learn it but not sure if this is a valid use case whether I am right in the approach? \$\endgroup\$ – Sunny Apr 10 '17 at 6:47
  • \$\begingroup\$ Did you intentionally leave out the parenthesis around your if (..) { ... } else { ... } blocks? \$\endgroup\$ – le_m Apr 17 '17 at 12:44
  • 1
    \$\begingroup\$ No, that was a mistake. I'm spoiled with IDE's like Visual Studio and Atom. Writing code in a markdown editor leads to mistakes ha \$\endgroup\$ – rayepps Apr 17 '17 at 16:44

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.