Using tests to help improve structure of some code

This thread is my attempt to achieve several things:

  • retain sanity throughout the process of improving some code
  • let others know that I don’t have improved code, yet
  • give you an interesting insight into behind the scenes work

I have some some code that I want to restructure, so that the if/else parts aren’t as nested as they currently are.

        menuItems.forEach(function (menuItem, menuIndex) {
            const config = tabs[tabId].config;
            if (menuItem.getAttribute("rel")) {
                tabs[tabId].menuItems[menuIndex].hasSubContent = true;
                if (config.disableTabLinks) {
                    menuItem.onclick = disableClick;
                }
                if (config.snapToOriginal.snap === true) {
                    id = menuItem.getAttribute("rel");
                    submenu = document.getElementById(id);
                    initWithSubmenu(tabId, menuItem, submenu);
                }
            } else {
                // for items without a submenu, add onMouseout effect
                initWithoutSubmenu(menuItem);
            }
            initSubmenu(menuItem);
            if (
                defaultSelected === "auto" &&
                defaultIsShown !== true &&
                isSelected(menuItem.href)
            ) {
                showSubmenu(tabId, menuItem);
                tabs[tabId].defaultSelected = menuItem;
                defaultIsShown = true;
            } else if (parseInt(defaultSelected) === menuIndex) {
                showSubmenu(tabId, menuItem);
                tabs[tabId].defaultSelected = menuItem;
            }
        });

The problem that I’m faced with is that I fear breaking something when I make any structural changes. That fear prevents the code from improving. Tests free us from that fear, as they give almost immediate feedback on changes that we make.

This code wasn’t written with tests in mind. That doesn’t mean that it’s impossible to test though, just difficult.

Can we run tests on the code?

Tests mean repeatedly running the code with different inputs, so that we can check for different desired outcomes.

The first thing to figure out is if we can change the environment and run the code, to get different results.

I have an HTML page all set up with a set of tabs being shown, and [Mocha] with [Chai] for the testing framework:

test/index.html

<html>
<head>
    <title>Dynamic Drive DHTML Scripts- DD Tab Menu Demos</title>
    <!-- CSS for Tab Menu #2 -->
    <link rel="stylesheet" type="text/css" href="../glowtabs.css" />
    <link rel="stylesheet" type="text/css" href="lib/mocha.css" />
</head>
<body>
    <div id="ddtabs1" class="glowingtabs">
        <ul>
            <li><a href="http://www.dynamicdrive.com" rel="gc1"><span>Home</span></a></li>
            <li><a href="http://www.dynamicdrive.com/new.htm" rel="gc2"><span>DHTML</span></a></li>
            <li><a href="http://www.dynamicdrive.com/style/" rel="gc3"><span>CSS</span></a></li>
            <li><a href="http://www.dynamicdrive.com/forums/"><span>Forums</span></a></li>
            <li><a href="http://tools.dynamicdrive.com/imageoptimizer/"><span>Gif Optimizer</span></a></li>
        </ul>
    </div>
    <div class="tabcontainer">
        <div id="gc1" class="tabcontent">
            Return to the <a href="http://www.dynamicdrive.com">frontpage</a> of Dynamic Drive.
        </div>
        <div id="gc2" class="tabcontent">
            See the new scripts recently added to Dynamic Drive. <a href="http://www.dynamicdrive.com/new.htm">Click here</a>.
        </div>
        <div id="gc3" class="tabcontent">
            Original, practical <a href="http://www.dynamicdrive.com/style/">CSS codes and examples</a> such as CSS menus for your site.
        </div>
    </div>
    <div id="mocha"></div>
    <script type="text/javascript" src="../ddtabmenu.js"></script>
    <script src="lib/mocha.js"></script>
    <script src="lib/chai.js"></script>
    <script class="mocha-init">
      mocha.setup("bdd");
      mocha.checkLeaks();
      const expect = chai.expect;
    </script>
    <script src="tabmenu.test.js"></script>
    <script class="mocha-exec">
      mocha.run();
    </script>
</body>
</html>

The mocha.setup and mocha.run parts are from the instructions on running Mocha in the browser.

Testing something

The ddtabmenu code has an initMenu method, that takes parameters of tabId and defaultSelected. We get an error when just using that, so we need to use definemenu before doing anything else.

describe("menutabs", function () {
    it("when tab 0 init'd, tab 0 is current", function () {
        ddtabmenu.definemenu("ddtabs1", 0);
        const tabs = document.querySelectorAll("#ddtabs1 a");
        ddtabmenu.initMenu("ddtabs1", 0);
        const tab = tabs[0];
        expect(tab.className).to.contain("current");
    });
});

It can be confusing to think of and write about the first tab and second tab, when they are zero-indexed the first tab is tab 0, and the second tab is tab 1. To avoid that confusion I will deal with them using a zero-index notation, as tab 0 and tab 1 instead.

Add tests, bringing common pieces together

We can then check if the other tabs don’t have the current class, moving some of the code out to things that should be updated before all of the tests run, and ones that should update before each individual test is run.

    before(function () {
        ddtabmenu.definemenu("ddtabs1", 0);
    });
    let tabs;
    beforeEach(function () {
        tabs = document.querySelectorAll("#ddtabs1 a");
    });
    it("when tab 0 init'd, tab 0 is current", function () {
        ddtabmenu.initMenu("ddtabs1", 0);
        const tab = tabs[0];
        expect(tab.className).to.contain("current");
    });
    it("when tab 0 init'd, tab 1 is not current", function () {
        ddtabmenu.initMenu("ddtabs1", 0);
        const tab = tabs[1];
        expect(tab.className).to.not.contain("current");
    });
    it("when tab 0 init'd, tab 2 is not current", function () {
        ddtabmenu.initMenu("ddtabs1", 0);
        const tab = tabs[2];
        expect(tab.className).to.not.contain("current");
    });

It’s important that each of those expect statements is in a separate test section. Here’s what that could look like when the expect statements are all in the same test section:

    it("when tab 0 init'd, tab 0 is current", function () {
        ddtabmenu.initMenu("ddtabs1", 0);
        const tab = tabs[0];
        expect(tab.className).to.contain("current");
        expect(tab.className).to.not.contain("current");
        expect(tab.className).to.not.contain("current");
    });

While that’s more condensed, the problem is that we don’t get to know much about which of the expect statements failed when a problem happens. That makes it harder to figure out what went wrong and further investigation is then needed to work out what went wrong and why. That’s not a problem that we need. Putting each expect statement in a separate test section helps to give us more details information about what went wrong, letting us more easily understand what went wrong and why.

Before doing other tests when initing other tabs, we should group these tests for tab 0 together. We can put them inside of a describe section, and rename the descriptions so that they make more sense together. The initMenu command can also be lifted up to be out of the it sections too.

    describe("when tab 0 init'd", function () {
        beforeEach(function () {
            ddtabmenu.initMenu("ddtabs1", 0);
        });
        it("tab 0 is current", function () {
            expect(tabs[0].className).to.contain("current");
        });
        it("tab 1 isn't current", function () {
            expect(tabs[1].className).to.not.contain("current");
        });
        it("tab 2 isn't current", function () {
            expect(tabs[2].className).to.not.contain("current");
        });
    });

Test when other tabs are current

We can now use similar code to check what happens when tab 1 and tab 2 are init’d too.

    describe("when tab 1 init'd", function () {
        beforeEach(function () {
            ddtabmenu.initMenu("ddtabs1", 1);
        });
        it("tab 0 isn't current", function () {
            expect(tabs[0].className).to.not.contain("current");
        });
        it("tab 1 is current", function () {
            expect(tabs[1].className).to.contain("current");
        });
        it("tab 2 isn't current", function () {
            expect(tabs[2].className).to.not.contain("current");
        });
    });
    describe("when tab 2 init'd", function () {
        beforeEach(function () {
            ddtabmenu.initMenu("ddtabs1", 2);
        });
        it("tab 0 is current", function () {
            expect(tabs[0].className).to.not.contain("current");
        });
        it("tab 1 isn't current", function () {
            expect(tabs[1].className).to.not.contain("current");
        });
        it("tab 2 isn't current", function () {
            expect(tabs[2].className).to.contain("current");
        });
    });

Those initial tests work, so we look to be ready to get started on testing the if/else code structure in the initMenu function.

We can set up different environment situations and try to trigger each of the different parts of the if/else code structure, which I’ll investigate in the next post.

4 Likes

When it comes to writing tests for the if/else code, at each potential branching location we want to come up with an effective way to confirm that one branch or the other was taken.

Testing the “rel” if condition

The first part of the if/else code that we are testing is about the rel attribute:

            if (menuItem.getAttribute("rel")) {
                tabs[tabId].menuItems[menuIndex].hasSubContent = true;
                if (config.disableTabLinks) {
                    menuItem.onclick = disableClick;
                }
                if (config.snapToOriginal.snap === true) {
                    id = menuItem.getAttribute("rel");
                    submenu = document.getElementById(id);
                    initWithSubmenu(tabId, menuItem, submenu);
                }
            } else {
                // for items without a submenu, add onMouseout effect
                initWithoutSubmenu(menuItem);
            }

In the else section, the initWithoutSubmenu function assigns an onmouseout event:

        function initWithoutSubmenu(tab) {
            //...
            tab.onmouseout = function revertWithoutSubmenu() {
                //...
            };
        }

As a preliminary test, we can check the name of the function assigned to the onmouseout event handler.

    describe("when tab has submenu", function () {
        it("investigate onmouseout", function () {
            ddtabmenu.initMenu("ddtabs1", 0);
            console.log(tabs[0].onmouseout.name);
            console.log(tabs[1].onmouseout.name);
            console.log(tabs[2].onmouseout.name);
            console.log(tabs[3].onmouseout.name);
            console.log(tabs[4].onmouseout.name);
        });
    });

The first three tabs have a submenu, and the last two don’t. We get the following information sent tot he browser console:

tabmenu.test.js:56 revert
tabmenu.test.js:57 revert
tabmenu.test.js:58 revert
tabmenu.test.js:59 revertWithoutSubmenu
tabmenu.test.js:60 revertWithoutSubmenu

That’s good news. We can use the onmouseout name to confirm that one branch or the other was taken, letting us replace the above investigation code with the following:

    describe("initMenu forEach if/else structure", function () {
        describe("submenu tabs have different onmouseout functions", function () {
            before(function () {
                ddtabmenu.initMenu("ddtabs1", 0);
            });
            it("has revert on tab 0", function () {
                expect(tabs[0].onmouseout.name).to.equal("revert");
            });
            it("has revert on tab 1", function () {
                expect(tabs[0].onmouseout.name).to.equal("revert");
            });
            it("has revert on tab 2", function () {
                expect(tabs[0].onmouseout.name).to.equal("revert");
            });
            it("hasrevertWithoutSubmenu on tab 3", function () {
                expect(tabs[0].onmouseout.name).to.equal("revert");
            });
            it("hasrevertWithoutSubmenu on tab 4", function () {
                expect(tabs[0].onmouseout.name).to.equal("revert");
            });
        });
    });
});

Testing the disableTabLinks config

This disableTabLinks condition is one that doesn’t change with different tabs.

                if (config.disableTabLinks) {
                    menuItem.onclick = disableClick;
                }

We need to setup a different environment before checking each state. At least we can confidently use the onclick attribute to test this one.

In this case, the tabmenu uses a property value to initialize disabletablinks when the definemenu method is being run.

        describe("disableTabLinks config", function () {
            beforeEach(function () {
                tabs[0].onclick = null;
            });
            it("disables tab links", function () {
                ddtabmenu.disabletablinks = true;
                ddtabmenu.definemenu("ddtabs1", 0);
                ddtabmenu.initMenu("ddtabs1", 0);
                expect(tabs[0].onclick.name).to.equal("disableClick");
            });
            it("doesn't disable tab links", function () {
                ddtabmenu.disabletablinks = false;
                ddtabmenu.definemenu("ddtabs1", 0);
                ddtabmenu.initMenu("ddtabs1", 0);
                expect(tabs[0].onclick).to.be.null;
            });
        });

Testing snap2original config

The next condition to test is for the following condition:

                if (config.snapToOriginal.snap === true) {
                    id = menuItem.getAttribute("rel");
                    submenu = document.getElementById(id);
                    initWithSubmenu(tabId, menuItem, submenu);
                }

That initWithSubmenu function sets some onmouseout and onmouseover events,

        function initWithSubmenu(tabId, tab, submenu) {
            //...
            tab.onmouseout = revert;
            submenu.onmouseover = clearRevert;
            submenu.onmouseout = revert;
        }

which we should be able to use to easily test that the condition.

        describe("snapToOriginal config", function () {
            beforeEach(function () {
                ddtabmenu.snap2original = [true, 300];
                tabs[0].onmouseout = null;
            });
            it("sets the onmouseout revert function", function () {
                ddtabmenu.snap2original[0] = true;
                ddtabmenu.definemenu("ddtabs1", 0);
                ddtabmenu.initMenu("ddtabs1", 0);
                expect(tabs[0].onmouseout.name).to.equal("revert");
            });
            it("doesn't set the onmouseout revert function", function () {
                ddtabmenu.snap2original[0] = false;
                ddtabmenu.definemenu("ddtabs1", 0);
                ddtabmenu.initMenu("ddtabs1", 0);
                expect(tabs[0].onmouseout).to.be.null;
            });
        });
    });

That seems to test the snap2original config setting quite well.

Testing the default menu code

The last part to test is this complex set of code:

            if (
                defaultSelected === "auto" &&
                defaultIsShown !== true &&
                isSelected(menuItem.href)
            ) {
                showSubmenu(tabId, menuItem);
                tabs[tabId].defaultSelected = menuItem;
                defaultIsShown = true;
            } else if (parseInt(defaultSelected) === menuIndex) {
                showSubmenu(tabId, menuItem);
                tabs[tabId].defaultSelected = menuItem;
            }

Fortunately when testing, I don’t necessarily have to understand what the code does. I only have to ensure that I keep track of the effect that the code has, so that later on when I do work with the code, the tests can ensure that the updated code still has the same effects.

The if condition though is going to be tricky to achieve. The isSelected function call only returns true when the tab link and the current page url match.

As a result, before doing any of these default menu tests, I’ll force the tab 0 link to be the current page url, and reset it back to what it was after the tests are done.

    describe("show default menu", function () {
        let tab0href;
        before(function () {
            tab0href = tabs[0].href;
            tabs[0].href = window.location;
        });
        after(function () {
            tabs[0].href = tab0href;
        });
        it("auto-shows the default", function () {
            ...
        });
    });

I’m now left with a puzzle about how I tell which of the if/else sections of code has been run. Both sections are identical all but for the defaultIsShown variable. As that defaultIsShown variable seems to be the only difference, I can update the initMenu function to return an object that contains that variable, for testing purposes. It’s not the ideal solution, but “needs must” when it comes to dealing with untestable code, and more appropriate techniques can be used later on.

We can now test that “auto” part with the following test:

    describe("show default menu", function () {
        let tab0href;
        beforeEach(function () {
            tab0href = tabs[0].href;
            tabs[0].className = "";
        });
        afterEach(function () {
            tabs[0].href = tab0href;
        });
        it("auto-shows the default", function () {
            const defaultSelected = "auto";
            ddtabmenu.definemenu("ddtabs1", defaultSelected);
            const result = ddtabmenu.initMenu("ddtabs1", defaultSelected);
            expect(tabs[0].className).to.contain("current");
            expect(result.defaultIsShown).to.be.true;
        });
    });

The expect test is checking that defaultIsShown is true, helping us to be sure that the first section of the if statement has been accessed.
A separate “current” classname is emptied before each test and checked. Strictly speaking it’s not needed for this particular test, but with the next tests it is needed so putting it in here too helps to make things consistent. That way only a minimal number of changes are made to each test to get the results that we need.

We can check that the first part of the if statement was not accessed, by changing only the “auto” part to 0, and checking that defaultIsShown is false.

        it("shows tab but not as default, when not auto", function () {
            tabs[0].href = window.location;
            const defaultSelected = 0;
            ddtabmenu.definemenu("ddtabs1", defaultSelected);
            const result = ddtabmenu.initMenu("ddtabs1", defaultSelected);
            expect(tabs[0].className).to.contain("current");
            expect(result.defaultIsShown).to.be.false;
        });

All in all the tests for the if/else code dealing with the default menu, are taken care of by the following tests:

    describe("show default menu", function () {
        let tab0href;
        beforeEach(function () {
            tab0href = tabs[0].href;
            tabs[0].className = "";
        });
        afterEach(function () {
            tabs[0].href = tab0href;
        });
        it("auto-shows the default", function () {
            tabs[0].href = window.location;
            const defaultSelected = "auto";
            ddtabmenu.definemenu("ddtabs1", defaultSelected);
            const result = ddtabmenu.initMenu("ddtabs1", defaultSelected);
            expect(result.defaultIsShown).to.be.true;
            expect(tabs[0].className).to.contain("current");
        });
        it("shows tab but not as default, when not auto", function () {
            tabs[0].href = window.location;
            const defaultSelected = 0;
            ddtabmenu.definemenu("ddtabs1", defaultSelected);
            const result = ddtabmenu.initMenu("ddtabs1", defaultSelected);
            expect(result.defaultIsShown).to.be.false;
            expect(tabs[0].className).to.contain("current");
        });
        it("doesn't show default when given tab index not found", function () {
            const defaultSelected = -1;
            ddtabmenu.definemenu("ddtabs1", defaultSelected);
            const result = ddtabmenu.initMenu("ddtabs1", defaultSelected);
            expect(result.defaultIsShown).to.be.false;
            expect(tabs[0].className).to.not.contain("current");
        });
    });

We should now have enough tests in place to give us good coverage when making improvements to the if/else structure of the default menu code, which I’ll start making improvements to in the next post.

2 Likes

Now that I have the tests in place, some refactoring can occur to this code.

The first thing that comes to mind is to reduce the scope of the config variable. It’s not used by anything below the first else, so we can push it down into the if structure.

        menuItems.forEach(function (menuItem, menuIndex) {
            // const config = tabs[tabId].config;
            if (menuItem.getAttribute("rel")) {
                const config = tabs[tabId].config;
                tabs[tabId].menuItems[menuIndex].hasSubContent = true;
                //...
            } else {

Next, the hasSubContent part isn’t needed. It’s only used when showing or hiding a menu section, so we can just directly check the id variable at the time instead.

            // if (menuItem.hasSubContent === true) {
            //     id = menuItem.getAttribute("rel");
            const id = menuItem.getAttribute("rel");
            if (id) {
                document.getElementById(id).style.display = "none";
            }
//...
        // if (targetItem.hasSubContent === true) {
        //     id = targetItem.getAttribute("rel");
        const id = targetItem.getAttribute("rel");
        if (id) {
            document.getElementById(id).style.display = "block";
        }
//...
        menuItems.forEach(function (menuItem, menuIndex) {
            if (menuItem.getAttribute("rel")) {
                const config = tabs[tabId].config;
                // tabs[tabId].menuItems[menuIndex].hasSubContent = true;
                //...
            } else {

We can also bring that assign id and check id idea into the if/else code too:

        menuItems.forEach(function (menuItem, menuIndex) {
            const id = menuItem.getAttribute("rel");
            // if (menuItem.getAttribute("rel")) {
            if (id) {
                const config = tabs[tabId].config;
                if (config.disableTabLinks) {
                    menuItem.onclick = disableClick;
                }
                if (config.snapToOriginal.snap === true) {
                    // id = menuItem.getAttribute("rel");
                    submenu = document.getElementById(id);
                    initWithSubmenu(tabId, menuItem, submenu);
                }
            } else {

Paranoia

Things are going well and despite all of the changes that I’m making the tests are all passing. That has me feeling paranoid. Years ago I was updating a different set of code than what was being tested, so I thought the code was passing the tests when instead problems were happening.

As a result I now check that the tests are connected to the code that I’m working on by deliberately mess up something in the code, usually by commenting out a line. It can be reassuring to see that the tests fail for some reason.

The tests suitably fail, giving me reassurance and I can restore that commented-out line feeling better about things.

Plans from here

We have simplified the code so that there there are now three main things that the code is doing. I want to move those out to separate functions, so that it’s easier to understand what’s going on.

What I want to have, but we don’t have yet, is something like the following simpler code:

        menuItems.forEach(function (menuItem, menuIndex) {
            const id = menuItem.getAttribute("rel");
            if (id) {
                initWithSubmenu(menuItem, id);
            } else {
                initWithoutSubmenu(menuItem);
            }
            showDefaultMenu(menuItem);
        });

Which I’ll work on in the next post.

3 Likes

Move code out to separate functions

Some of the if/else code is better placed in the initWithSubmenu, but the code in there doesn’t really fit together. It’s clear that different things are being done. The code in the initWithSubmenu function actually reverts things instead. As a result the initWithSubmenu code can be moved out to a separate revertToSubmenu function instead, making room to put code in the initWithSubmenu function.

        function revertToSubmenu(menuItem, id) {
            const submenu = document.getElementById(id);
            
            function revert() {
                revertToDefault(tabId);
            }
            function clearRevert() {
                clearRevertToDefault(tabId);
            }
            menuItem.onmouseout = revert;
            submenu.onmouseover = clearRevert;
            submenu.onmouseout = revert;
        }

        function initWithSubmenu(menuItem, id) {
            if (config.disableTabLinks) {
                menuItem.onclick = function disableClick() {
                    return false;
                };
            }
            if (config.snapToOriginal.snap === true) {
                revertToSubmenu(menuItem, id);
            }
        }
//...
        menuItems.forEach(function (menuItem, menuIndex) {
            const id = menuItem.getAttribute("rel");
            if (id) {
                initWithSubmenu(menuItem, id);
            } else {
                // add onMouseout effect
                initWithoutSubmenu(menuItem);
            }
            initSubmenu(menuItem);

That initSubmenu code just adds an onmouseover event.

        function initSubmenu(tab) {
            tab.onmouseover = function leaveTab() {
                showSubmenu(tabId, tab);
            };
        }

Instead of doing that, I’ll move the leaveTab function out of it, and do the tab.onmouseover command from both the initWithSubmenu and initWithoutSubmenu code. That way the event assignment is nice and consistent in both places.

I tried doing this all in one go and things went bad, so stepped back to here where we’ll do it more slowly.

First, the showSubmenu gets moved to the bottom of both the initWithSubmenu and initWithoutSubmenu functions.

        function initWithSubmenu(menuItem, id) {
            //...
            initSubmenu(menuItem);
        }
        function initWithoutSubmenu(menuItem) {
            //...
            initSubmenu(menuItem);
        }
//...
        menuItems.forEach(function (menuItem, menuIndex) {
            const id = menuItem.getAttribute("rel");
            if (id) {
                initWithSubmenu(menuItem, id);
            } else {
                // add onMouseout effect
                initWithoutSubmenu(menuItem);
            }
            // initSubmenu(menuItem);

Here is the initSubmenu code that we are wanting to remove the need for.

            function leaveTab(evt) {
                showSubmenu(tabId, menuItem);
            }
            menuItem.onmouseover = leaveTab;

We can now move the leaveTab function out of the initSubmenu code. Actually, no we can’t. The menuItem variable is a dependency.

Remove dependency on menuItem variable

The onmouseover event doesn’t make it all that easy for us to access the menuItem element. What can be done instead is to replace the onmouseover and onmouseout events with onmouseenter and onmouseleave instead. That way it doesn’t trigger on span and other elements within the item, it only triggers when appropriately entering or leaving.

            // menuItem.onmouseout = revert;
            menuItem.onmouseleave = revert;
            // submenu.onmouseover = clearRevert;
            submenu.onmouseenter = clearRevert;
            // submenu.onmouseout = revert;
            submenu.onmouseleave = revert;
//...
            // menuItem.onmouseover = leaveTab;
            menuItem.onmouseenter = leaveTab;
//...
            // menuItem.onmouseout = function revertWithoutSubmenu() {
            menuItem.onmouseleave = function revertWithoutSubmenu() {

Those changes break the tests so we step back and undo those changes, while we seek to better understand the problem.

We should be able to replace onmouseover for onmouseenter, and onmouseout for one at a time without anything going bad. Let’s do those one at a time and examine what occurs.

Replacing onmouseover in the revertToSubmenu function is successful.

        function revertToSubmenu(menuItem, id) {
            //...
            menuItem.onmouseout = revert;
            // submenu.onmouseover = clearRevert;
            submenu.onmouseenter = clearRevert;
            submenu.onmouseout = revert;
        }

The other onmouseover in the initSubmenu function is also successfully replaced.

        function initSubmenu(menuItem) {
            function leaveTab(evt) {
                showSubmenu(tabId, menuItem);
            }
            // menuItem.onmouseover = leaveTab;
            menuItem.onmouseenter = leaveTab;
        }

The onmouseleave is where trouble occurs:

            // menuItem.onmouseout = revert;
            menuItem.onmouseleave = revert;
            submenu.onmouseenter = clearRevert;
            // submenu.onmouseout = revert;
            submenu.onmouseleave = revert;

The problem is that the test code is checking for onmouseout. It would have been better if we didn’t use onmouseout and instead simulated the mouse movement in some way, but that gets a lot harder to do properly. As a result, we can just rename onmouseout to onmouseleave in the tests instead.

            it("has revert on tab 0", function () {
                // expect(tabs[0].onmouseout.name).to.equal("revert");
                expect(tabs[0].onmouseleave.name).to.equal("revert");
            });
            it("has revert on tab 1", function () {
                // expect(tabs[0].onmouseout.name).to.equal("revert");
                expect(tabs[0].onmouseleave.name).to.equal("revert");
            });

A different way to test that is to make the event handler function accessible so that we can replace that function with a spy instead, using an expansion to Chai called chai-spies. That could be something to investigate if we come across the same problem.

For now though, the updated code is all working properly as it should do, and the event handler now has much better access to the information that it needs.

Out of date comment

Of course, the comment about onmouseout is now misleading and out of date.

            if (id) {
                initWithSubmenu(menuItem, id);
            } else {
                // add onMouseout effect
                initWithoutSubmenu(menuItem);
            }

So that comment can be removed. Not only is there no onmouseout effect anymore, we plan to also have other events being setup in there too (both enter and leave). As the comment serves no beneficial purpose, it gets removed.

Next steps

Now that more suitable mouse events are being used, we can head back to separating the leaveTab function from the initSubmenu function - in the next post.

2 Likes

Improving the leaveTab code

Now that we are using onmouseenter for the event, we have a better solution for the following code:

        function initSubmenu(menuItem) {
            function leaveTab(evt) {
                showSubmenu(tabId, menuItem);
            }
            menuItem.onmouseenter = leaveTab;
        }

Instead of using menuItem, we can get the element from evt.target, letting us extract the leaveTab function.

        function leaveTab(evt) {
            showSubmenu(tabId, evt.target);
        }
        function initSubmenu(menuItem) {
            // function leaveTab(evt) {
            //     showSubmenu(tabId, menuItem);
            // }
            menuItem.onmouseenter = leaveTab;
        }

Because the initSubmenu function has so little to do now, we can replace the initSubmenu function calls with the onmouseenter assignment instead.

        // function initSubmenu(menuItem) {
        //     menuItem.onmouseenter = leaveTab;
        // }
//...
        function initWithSubmenu(menuItem, id) {
            //...
            if (config.snapToOriginal.snap === true) {
                revertToSubmenu(menuItem, id);
            }
            // initSubmenu(menuItem);
            menuItem.onmouseenter = leaveTab;
        }
//...
        function initWithoutSubmenu(menuItem) {
            menuItem.onmouseenter = leaveTab;
            menuItem.onmouseleave = function revertWithoutSubmenu() {
                //...
            };
            // initSubmenu(menuItem);
        }

Grouping onmouseenter with onmouseleave

For better clarity, it helps if related events are assigned in the same place.

Currently the revertToSubmenu function assigns onmouseleave and completely ignores onmouseenter.

        function revertToSubmenu(menuItem, id) {
            //...
            menuItem.onmouseleave = revert;
            submenu.onmouseenter = clearRevert;
            submenu.onmouseleave = revert;
        }

We should assign onmouseenter at the same time as assigning onmouseleave. That means using an if/else structure for snapToOriginal, so that the revertToSubmenu function can assign menuItem.onmouseenter, and have the else clause also assign onmouseenter too.

        function revertToSubmenu(menuItem, id) {
            //...
            menuItem.onmouseenter = leaveTab;
            menuItem.onmouseleave = revert;
            submenu.onmouseenter = clearRevert;
            submenu.onmouseleave = revert;
        }
            if (config.snapToOriginal.snap === true) {
                revertToSubmenu(menuItem, id);
            // }
            // menuItem.onmouseenter = leaveTab;
            } else {
                menuItem.onmouseenter = leaveTab;
            }

Clearing the onmouseleave event handler too

In the else clause, along with the onmouseenter method, it makes good sense to also assign a matching onmouseleave method.

We already have a test that checks that the onmouseleave event is null:

            it("doesn't set the onmouseleave revert function", function () {
                ddtabmenu.snap2original[0] = false;
                ddtabmenu.definemenu("ddtabs1", 0);
                ddtabmenu.initMenu("ddtabs1", 0);
                expect(tabs[0].onmouseleave).to.be.null;
            });

Adding in that event handler helps to make the code more explicit, with both the enter and leave methods being assign in the same place at the same time.

            } else {
                menuItem.onmouseenter = leaveTab;
                menuItem.onmouseleave = null;
            }

We even allowed the else statement to explicitly assign null to the event, to help inform everyone that we delibertely intend for no event to occur.

Use a consistent naming scheme

It becomes clear now that leaveTab is not a suitable name for that event handler. To gain a better understanding of what to name them, we can group all of the event handler functions together.

        function leaveTab(evt) {
            showSubmenu(tabId, evt.target);
        }
        function revert() {
            revertToDefault();
        }
        function clearRevert() {
            clearRevertToDefault();
        }
        function disableClick() {
            return false;
        }

It’s a good standard to have evt as the function parameter. Regardless of whether evt is used or not, it also helps to use the term Handler in the event handler function names to help avoid confusion about them. As such, we can rename leaveTab to be menuEnterHandler, and add Handler to the clearRevert and disableClick functions:

        // function leaveTab(evt) {
        function menuEnterHandler(evt) {
            showSubmenu(tabId, evt.target);
        }
        function revert() {
            revertToDefault();
        }
        // function clearRevertHandler() {
        function clearRevertHandler() {
            clearRevertToDefault(tabId);
        }
        // function disableClick() {
        function disableClickHandler() {
            return false;
        }
//...
            // menuItem.onmouseenter = leaveTab;
            menuItem.onmouseenter = menuEnterHandler;
            menuItem.onmouseleave = revert;
            const submenu = document.getElementById(id);
            // submenu.onmouseenter = clearRevert;
            submenu.onmouseenter = clearRevertHandler;
            submenu.onmouseleave = revert;
//...
            if (config.disableTabLinks) {
                // menuItem.onclick = disableClick;
                menuItem.onclick = disableClickHandler;
            }
//...
            } else {
                // menuItem.onmouseenter = leaveTab;
                menuItem.onmouseenter = menuEnterHandler;

Improving the click prevention

The menuEnterHandler function does cause a test to fail because it’s relying on the function name. That’s kind of brittle.

We could back off a bit, and just check that a function exists on the click handler instead, and click on the tab to check that nothing happens.

            it("disables tab links", function () {
                ddtabmenu.disabletablinks = true;
                ddtabmenu.definemenu("ddtabs1", 0);
                ddtabmenu.initMenu("ddtabs1", 0);
                // expect(tabs[0].onclick.name).to.equal("disableClick");
                expect(tabs[0].onclick).to.be.a("function");
                tabs[0].click();
            });

However, I want to do better than that, by passing pass a fake event object to the function, checking that a preventDefault method has been called.

Earlier I said that I would keep an eye out for when we want to spy on something else, and use the chai-spies library. This is that time, which will be the next post.

2 Likes

When it comes to testing the click event handler, we now need a way to check that the clicked-on link was prevented from being followed. The event object has a preventDefault method that achieves that, but we don’t yet have access to that.

What we can do is to store the event handlers in a handlers object, that is accessible both from the code and when testing too.

We can get started by testing for a click handler.

Testing the click handler

First we test that a click handler exists:

            it("has a click handler", function () {
                expect(ddtabmenu.handlers.click).to.be.a("function");
            });

That causes the test to fail, so we make the test pass by adding a handlers.click method.

    return {
        definemenu,
        initMenu,
        handlers: {
            click: undefined
        }
    };

The test is now happy about the click part, and expects a function instead of undefined.

We can move all of the event handlers to that handlers object. I don’t want all of the handler functions to be defined at the end of the code, so I’ll define them at the start of the code in a handlers object, and just assign that handlers object at the end.

    const handlers = {
    };
//...
    return {
        definemenu,
        initMenu,
        handlers
    };

The disable click handler can be assignedto the handlers object:

        function disableClickHandler(evt) {
            evt.preventDefault();
        }
        handlers.disableClick = disableClickHandler;
//...
            if (config.disableTabLinks) {
                // menuItem.onclick = disableClickHandler;
                menuItem.onclick = handlers.disableClick;
            }

That causes the test to pass, which indicates that we are on the right track.

Move other event handlers to handlers object

It doesn’t make much sense for only the click handler to be on the handlers object, so let’s put the rest of the event handlers on that handlers object too:

        handlers.menuEnter = menuEnterHandler;
        handlers.revert = revertHandler;
        handlers.clearRevert = clearRevertHandler;
        handlers.disableClick = disableClickHandler;
//...
            // menuItem.onmouseenter = menuEnterHandler;
            menuItem.onmouseenter = handlers.menuEnter;
            // menuItem.onmouseleave = revertHandler;
            menuItem.onmouseleave = handlers.revert;
            const submenu = document.getElementById(id);
            // submenu.onmouseenter = clearRevertHandler;
            submenu.onmouseenter = handlers.clearRevert;
            // submenu.onmouseleave = revertHandler;
            submenu.onmouseleave = handlers.revert;

Test the disableClick behaviour

Currently the disableClick just returns false to prevent the default behaviour of following a link. While that works, it’s not so easy to confirm that it’s achieving that desired behaviour. There is another way to prevent the default behaviour and that is to call the event method preventDefault()

We can have a test check that the preventDefault method is called, and we can use chai-spies to ensure that the click event ends up calling that function. We will do that in the next post.

2 Likes

Now that we have handlers that can be used by the code, and are easily accesible to testing.

Testing preventDefault

When an function that’s handing an event returns false, that prevents more than just the default action. It also prevents the event from bubbling up to parent nodes. These days instead of returning false, it’s preferred to more explicitly call the event method preventDefault(). Not only is that more explicit and easier to understand, it also helps to ensure that you don’t have a larger impact on the system than intended.

Without chai-spies, we would have a fucntion update a local variable so that we can check that the function was run.

            it("prevents the default action", function () {
                let spyWasCalled = false;
                const spy = function () {
                    spyWasCalled = true;
                };
                const evt = {
                    preventDefault: spy
                };
                ddtabmenu.handlers.disableClick(evt);
                expect(spyWasCalled).to.be.true;
            });

Right now that test appropriately fails. We can update the scripting code to use preventDefault instead of false.

        // function disableClickHandler() {
        function disableClickHandler(evt) {
            // return false;
            evt.preventDefault();
        }

The test now passes with good results.

This is a good time to install chai-spies, to replace the test code with something more designed for the job.

Installing chai-spies

Chai-spies prefers to be installed using Node, but I’m not using it in a Node environment so I must obtain it in some other way.

CDN’s are a good way to obtain JavaScript libraries, and I find chai-spies at https://www.jsdelivr.com/package/npm/chai-spies
I can then download it from there to my test/lib/ folder, and add chai-spies to the test page.

    <script src="lib/mocha.min.js"></script>
    <script src="lib/chai.min.js"></script>
    <script src="lib/chai-spies.min.js"></script>

Using chai.spy

I can now remove the temporary variable in the test and use chai.spy instead, with to.have.been.called() checking that the function was called.

            it("prevents the default action", function () {
                const spy = chai.spy(function () {});
                const evt = {
                    preventDefault: spy
                };
                ddtabmenu.handlers.disableClick(evt);
                expect(spy).to.have.been.called();
            });

The spy makes it easier to understand what the test is wanting to achieve.

We can now move on to using a spy with the click test too.

Using chai.spy elsewhere

The test after that which actually triggers the click event, can be updated to use a spy too.

            it("disables tab links", function () {
                ddtabmenu.disabletablinks = true;
                ddtabmenu.definemenu("ddtabs1", 0);
                const spy = chai.spy.on(ddtabmenu.handlers, "disableClick");
                ddtabmenu.handlers.disableClick = undefined;
                ddtabmenu.initMenu("ddtabs1", 0);
                tabs[0].click();
                expect(spy).to.have.been.called();
            });

That should work, but currently doesn’t.

Investigating the issue

Somehow the event handler is not being replaced by the spy, before it gets assigned to the click event.

I’ve scattered some console.log statements around to let me study the existing order in which things happen:

    function disableClickHandler(evt) {
        console.log("6. handler called");
        evt.preventDefault();
    }
//...
        console.log("1. define handler");
        handlers.disableClick = disableClickHandler;
//...
            if (config.disableTabLinks) {
                console.log("4. assign onclick");
                menuItem.onclick = ddtabmenu.handlers.disableClick;

With other log statements numbered to help me place things in a desired order.

            it("disables tab links", function () {
                ddtabmenu.disabletablinks = true;
                ddtabmenu.definemenu("ddtabs1", 0);
                console.log("2. create spy");
                const spy = chai.spy.on(ddtabmenu.handlers, "disableClick");
                console.log("3. replace handler");
                ddtabmenu.handlers.disableClick = undefined;
                ddtabmenu.initMenu("ddtabs1", 0);
                console.log("5. click element");
                tabs[0].click();
                console.log("7. expect spy");
                expect(spy).to.have.been.called();
            });

That gives a console log of:

2. create spy           tabmenu.test.js:90
3. replace handler      tabmenu.test.js:92
1. define handler       ddtabmenu.js:94
4. assign onclick       ddtabmenu.js:107
5. click element        tabmenu.test.js:95
6. handler called       ddtabmenu.js:69
7. expect spy           tabmenu.test.js:97

The define handler part is out of order. We want that to happen before everything else, or at least before replacing replacing the handler.

We’ll delve in to why that is happening in the next post.

2 Likes

The problem that we have is that the event handers are being defined and assigned both at the same time.

2. create spy           tabmenu.test.js:90
3. replace handler      tabmenu.test.js:92
1. define handler       ddtabmenu.js:94
4. assign onclick       ddtabmenu.js:107

We need to split that up a bit, so that the event handlers are first defined, letting us replace them with a spy, so that the updated event handler can then be assigned.

Init disableClick handler when first run

We want to assign disableClick to the handlers as soon as practical, before we even use the defineMenu and initMenu functions.

That means extracting the disableClickHandler function out of the initMenu function:

    function initMenu(tabId, defaultSelected) {
        //...
        handlers.menuEnter = menuEnterHandler;
        handlers.revert = revert;
        handlers.clearRevert = clearRevertHandler;
        // handlers.disableClick = disableClickHandler;
        //...
    }
    //...    
    handlers.disableClick = disableClickHandler;

    return {
        definemenu,
        initMenu,
        handlers
    };

and moving the disableClickHandler function so that it can be found from outside of the initMenu function.

    function disableClickHandler(evt) {
        evt.preventDefault();
    }
    function initMenu(tabId, defaultSelected) {
    //...
        // function disableClickHandler(evt) {
        //     evt.preventDefault();
        // }
    //...
    }

That all works now, and all of the console statements can be removed.

As this event handler restructure has been beneficial, we really should move the other event handlers to be in the same place too.

Move other event handlers to join the disableClickHandler

Currently we have several functions inside of the initMenu function. It will be easier to move them around once we move them to be outside of the initMenu function.

There are complicating factors though. Several of the event handler functions use closure to access the tabId variable. For example:

        function clearRevertHandler() {
            clearRevertToDefault(tabId);
        }

How are we to get that tabId in some other way? It must be via an event handler variable called evt.

        // function clearRevertHandler() {
        function clearRevertHandler(evt) {
            const tabId = ???
            clearRevertToDefault(tabId);
        }

We do have a way via the HTML elements. We have the tab menu using rel to refer to a submenu.

    <div id="ddtabs1" class="glowingtabs">
        <ul>
            <li><a href="http://www.dynamicdrive.com" rel="gc1"><span>Home</span></a></li>
            <!-- ... -->
        </ul>
    </div>
    <div class="tabcontainer">
        <div id="gc1" class="tabcontent">
            Return to the <a href="http://www.dynamicdrive.com">frontpage</a> of Dynamic Drive.
        </div>
            <!-- ... -->
    </div>

The evt.target reference will refer to one of the tabcontent sections. We can get gc1 from the id, find the element with rel=“gc1”, and walk up its parents to the UL element and further to the id=“ddtabs1” element.

        function clearRevertHandler(evt) {
            const submenuId = evt.target.id;
            const menuTab = document.querySelector("[rel=" + submenuId + "]");
            const ul = menuTab.parentNode.parentNode;
            const tabId = ul.parentNode.id;
            clearRevertToDefault(tabId);
        }

We can simplify that by moving most of the code into a separate function that does the search:

        function getTabmenuViaRel(el) {
            const submenuId = el.id;
            const menuTab = document.querySelector("[rel=" + submenuId + "]");
            const ul = menuTab.parentNode.parentNode;
            const tabmenu = ul.parentNode;
            return tabmenu;
        }
        function clearRevertHandler(evt) {
            const tabmenu = getTabmenuViaRel(evt.target);
            clearRevertToDefault(tabmenu.id);
        }

and those two functions can now be extracted up out of the initMenu function.

We can carry on by using getTabmenuViaRel() to get the tabId in the other functions, allowing us to extract them up out of the initMenu function too.

Extract the revert event handler

The revert event handler uses revertToDefault, which also accesses the tabId variable. We can get that tabId from tabmenu.id, and update reverToDefault to use a function parameter to get the tabId.

        // function revertToDefault() {
        function revertToDefault(tabId) {
            const delay = config.snapToOriginal.delay;
            tabs[tabId].timer = setTimeout(showDefaultTab, delay);
        }
        function revert(evt) {
            const tabmenu = getTabmenuViaRel(evt.target);
            // revertToDefault();
            revertToDefault(tabmenu.id);
        }
//...
                if (config.snapToOriginal.snap === true) {
                    // revertToDefault();
                    revertToDefault(tabId);
                }

The revertToDefault function also needs access to the config, so we need up update that too:

        // function revertToDefault() {
        function revertToDefault(tabId) {
            const config = tabs[tabId].config;
            const delay = config.snapToOriginal.delay;
            tabs[tabId].timer = setTimeout(showDefaultTab, delay);
        }

The showDefaultTab function also needs the tabId. As it’s only used by the timeout, we can move that function inside of revertToDefault so that it can access tabId by closure.

    function revertToDefault(tabId) {
        function showDefaultTab() {
            showSubmenu(tabId, tabs[tabId].defaultSelected);
        }
        const config = tabs[tabId].config;
        const delay = config.snapToOriginal.delay;
        tabs[tabId].timer = setTimeout(showDefaultTab, delay);
    }

We can now extract both the revertToDefault and the revert functions up out of the initMenu function.

Extract the menuEnterHandler function

We also need to use getTabmenuViaRel to get the tab id when the menuEnterHandler function is called:

        function menuEnterHandler(evt) {
            const tabmenu = getTabmenuViaRel(evt.target);
            showSubmenu(tabId, evt.target);
        }

This menu handler though has a different menu link, so we need to update getTabmenuViaRel so that it checks if there’s a rel attribute, and if there isn’t then it finds it using the id.

    function getTabmenuViaRel(el) {
        let menuTab = el;
        if (!el.rel) {
            const submenuId = el.id;
            menuTab = document.querySelector("[rel=" + submenuId + "]");
        }
        const ul = menuTab.parentNode.parentNode;
        const tabmenu = ul.parentNode;
        return tabmenu;
    }

We can now extract the menuEnterHandler function up out of the initMenu function.

Move the handler assignments to the end of the code

We now have the event handlers all nicely grouped together, outside of the initMenu function where they are more easily accessed.

    function getTabmenuViaRel(el) {
        let menuTab = el;
        if (!el.rel) {
            const submenuId = el.id;
            menuTab = document.querySelector("[rel=" + submenuId + "]");
        }
        const ul = menuTab.parentNode.parentNode;
        const tabmenu = ul.parentNode;
        return tabmenu;
    }
    function menuEnterHandler(evt) {
        const tabmenu = getTabmenuViaRel(evt.target);
        showSubmenu(tabmenu.id, evt.target);
    }
    function revert(evt) {
        const tabmenu = getTabmenuViaRel(evt.target);
        revertToDefault(tabmenu.id);
    }
    function clearRevertHandler(evt) {
        const tabmenu = getTabmenuViaRel(evt.target);
        clearRevertToDefault(tabmenu.id);
    }
    function disableClickHandler(evt) {
        evt.preventDefault();
    }

The code that assigns those handlers to the handlers object can now be moved down to the bottom of the code:

    handlers.menuEnter = menuEnterHandler;
    handlers.revert = revert;
    handlers.clearRevert = clearRevertHandler;
    handlers.disableClick = disableClickHandler;

    return {
        definemenu,
        initMenu,
        handlers
    };

Conclusion

All of the tests that we have in place still pass, major improvements have happened to the code while keeping everything still working.

A next step could be to investigate the coverage of the tests. Currently 80% of the code is handled by the tests, which means that 20% of the code can change without the tests picking up any problem at all. We could flesh out tests for the above event handlers, and use built-in browser tools to help us improve that coverage further, but that’s for another time.

The intention here was to use the tests to help us reduce the if/else nesting in some forEach code, which has been suitably achieved. I’ll finish up in my next post by returning back to that forEach code to tidy things up.

2 Likes

The forEach code that we are wanting to improve is as follows:

        menuItems.forEach(function (menuItem, menuIndex) {
            const id = menuItem.getAttribute("rel");
            if (id) {
                initWithSubmenu(menuItem, id);
            } else {
                initWithoutSubmenu(menuItem);
            }
            if (
                defaultSelected === "auto" &&
                defaultIsShown !== true &&
                isSelected(menuItem)
            ) {
                showSubmenu(tabId, menuItem);
                tabs[tabId].defaultSelected = menuItem;
                defaultIsShown = true;
            } else if (parseInt(defaultSelected) === menuIndex) {
                showSubmenu(tabId, menuItem);
                tabs[tabId].defaultSelected = menuItem;
            }
        });

The first if statement is a good example of what we should be aiming for. We want a nice simple condition, followed by a simple function call.

Remove test-specific code

We returned a defaultIsShown value so that a test can tell what’s going on.

        return {
            defaultIsShown
        };

I would rather not do that, and want to try to get that same information without that code.

In the tests we are already checking that the appropriate menu is showing, so we can remove that defaultIsShown code.

test/tabmenu.test.js

        it("auto-shows the default", function () {
            const defaultSelected = "auto";
            ddtabmenu.definemenu("ddtabs1", defaultSelected);
            const result = ddtabmenu.initMenu("ddtabs1", defaultSelected);
            // expect(result.defaultIsShown).to.be.true;
            expect(tabs[0].className).to.contain("current");
        });

ddtabmenu.js

        // return {
        //     defaultIsShown
        // };

With that gone, the rest should be quite easy to achieve.

Split up the forEach statement

Let’s simplify things by splitting up the forEach statement, so that we only have to deal with a smaller set of code.

        menuItems.forEach(function (menuItem) {
            const id = menuItem.getAttribute("rel");
            if (id) {
                initWithSubmenu(menuItem, id);
            } else {
                initWithoutSubmenu(menuItem);
            }
        });
        menuItems.forEach(function (menuItem, menuIndex) {
            if (
                defaultSelected === "auto" &&
                defaultIsShown !== true &&
                isSelected(menuItem)
            ) {
                showSubmenu(tabId, menuItem);
                tabs[tabId].defaultSelected = menuItem;
                defaultIsShown = true;
            } else if (parseInt(defaultSelected) === menuIndex) {
                showSubmenu(tabId, menuItem);
                tabs[tabId].defaultSelected = menuItem;
            }
        });

Move code out to functions

Simplifying things further, we can move the if conditions and body statements into separate functions. That way, the structure of the code is easier to understand and manipulate.

        function isAutoDefault(defaultSelected, menuItem) {
            return (
                defaultSelected === "auto" &&
                isSelected(menuItem)
            );
        }
        function isSpecifiedMenu(defaultSelected, menuIndex) {
            return parseInt(defaultSelected) === menuIndex;
        }
        function showDefaultMenu(menuItem) {
            showSubmenu(tabId, menuItem);
            tabs[tabId].defaultSelected = menuItem;
        }
//...
        menuItems.forEach(function (menuItem, menuIndex) {
            if (
                defaultIsShown !== true &&
                isAutoDefault(defaultSelected, menuItem)
            ) {
                showDefaultMenu(menuItem);
                defaultIsShown = true;
            } else if (isSpecifiedMenu(defaultSelected, menuIndex)) {
                showDefaultMenu(menuItem);
            }
        });

Be more explicit with defaultIsShown

The defaultIsShown boolean is designed to ensure that only one menu item is shown. We can make that more explicit by using separate if statements to return early, if the default is already shown.

        menuItems.forEach(function (menuItem, menuIndex) {
            if (defaultIsShown === true) {
                return;
            }
            if (isAutoDefault(defaultSelected, menuItem)) {
                showDefaultMenu(menuItem);
                defaultIsShown = true;
            }
            if (defaultIsShown === true) {
                return;
            }
            if (
                defaultIsShown !== true &&
                isSpecifiedMenu(defaultSelected, menuIndex)
            ) {
                showDefaultMenu(menuItem);
            }
        });

It’s now very clear that we only want to show the default menu once.

Use Array find method instead

A more suitable way to do something only once is to use the Array find method to find a suitable item.

To achieve that, we want menuItems to be a proper array.

        const menuItems = Array.from(container.querySelectorAll("a"));

We can now easily find the default menu, and show it.

        function isDefaultMenu(menuItem, menuIndex) {
            return (
                isAutoDefault(defaultSelected, menuItem) ||
                isSpecifiedMenu(defaultSelected, menuIndex)
            );
        }
        //...
        const defaultMenu = menuItems.find(isDefaultMenu);
        showDefaultMenu(defaultMenu);

The forEach code no longer serves ant benefit, and can now be removed.

Cleaning up

The showDefaultMenu doesn’t seem to be necessary now, so we can inline that function, and remove the remaining defaultIsShown code too.

        // function showDefaultMenu(menuItem) {
        //     showSubmenu(tabId, menuItem);
        //     tabs[tabId].defaultSelected = menuItem;
        // }
//...
        // const defaultIsShown = false;
//...
        const defaultMenu = menuItems.find(isDefaultMenu);
        // showDefaultMenu(defaultMenu);
        showSubmenu(tabId, defaultMenu);
        tabs[tabId].defaultSelected = defaultMenu;

The tests all still pass, and we are left with the following much simplified code that does everything that’s needed.

        const defaultMenu = menuItems.find(isDefaultMenu);
        showSubmenu(tabId, defaultMenu);
        tabs[tabId].defaultSelected = defaultMenu;

Conclusion

That brings this exploration to an end.

The tests have help to inform me whenever I start to diverge from what works. Most of those warnings I haven’t broadcast, ad instead have used undo to walk back to what does work, so that I can make another more successful attempt.

I hope that you found parts of this to provide some beneficial insight.

1 Like