Remove duplication to improve code

[Paul_Wilkins]

In a recent thread about validating form fields, I noticed a lot of duplication in the code, and that duplication has been making it tricky for people to examine the code. This thread is dedicated to exploring how to effectively expose the duplication and deal with it.

In this post we will get ready to use jsInspect:

• Copy the existing code
• Install jsInspect, which needs node
• Set things up so that we can use jsInspect to examine for duplication
• Use github so that the different stages of investigation can be easily retrieved

Copy the existing code

The latest version of the code seems to be from https://codepen.io/cfrank2000/pen/XWKdRwV. I have exported that code to a zip file, and in the extracted location plan to use jsinspect to investigate things.

Extracting the zip file gives a registration3 folder with both a src and dist folder. The src folder is the code in the codePen panels, and the dist folder is the ready to run page with the HTML CSS and JS all connected together.

That code I have put up on github as v0.0.0 in releases

Install Node and jsInspect

The jsinspect tool uses node, so if you don’t already have it, get and install Node.

The next step is to open a command prompt or a powershell window in the registration3 folder. With the shift key held down, right-click on the registration3 folder and you’ll find an option to open a command or a powershell window at that folder.

We can then init npm, which is a node package manager that comes with Node, and press enter on all of the asked for fields.

> npm init

The init information isn’t all that important at this stage so we can press Enter for each line to accept the default values. It is the package.json file from init that we need, so that we can then install jsinspect.

With jsinspect we want to use it when developing the code, but it’s of no use at any other stage, so we use --save-dev when installing it.

> npm install --save-dev jsinspect

Use jsInspect on the code

We can now use jsInspect to inspect the JavaScript code.

> npx jsinspect src

Couldn't parse ./src\script.js: Unexpected token (24:5)
    </script>
     ^

Looking at line 24 of the code, I see that you have script tags there. Those are not valid JavaScript code and should never be in the javsacript code, and need to be removed.

Split up the file

Here I’ll give the benefit of the doubt, and presume that the several sets of script tags tell us that the code is supposed to be split in to separate files. I’ve used those script tags to split the code in to separate files called modal.js, validate.js, change-password.js, and login.js

I’ve also copied those files over to the dist folder and removed the old script.js file from in there, so that I can set up index.html in there with update script tags.

<script src='https://ajax.googleapis.com/ajax/libs/jquery/3.3.1/jquery.min.js'></script>
<script src='https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/js/bootstrap.min.js'></script>
<script src="./setup.js"></script>
<script src="./validate.js"></script>
<script src="./change-password.js"></script>
<script src="./login.js"></script>

We no longer need the src folder so I’ve deleted the src folder. The dist folder causes troubles later on too, so better to rename it now, to registration3

Keeping track of things

Running jsinspect again I get a long list of duplicate code, and am told 18 matches found across 4 files. hat’s a lot of duplicates.

As the list is too long for me to reasonably scroll up and find the start, I’ll work on the last example of duplication that’s shown instead, which in this case is at line 1940 of the validate code.

Before we do that though, I’ll save this code to github to help keep track of things, and so that it can be downloaded as I progress through this work.

Adding code to github

This information about adding the code to github is less for your benefit, and more for mine as a later reference. I add the code to github so that you can gain the benefit of downloading it as I progress through this assistance.

Here I go to my account at github.com and create a new repository for [remove-duplication] and tell it to use a .gitignore for Node so that the node_modules folder doesn’t get pushed to the repository.

As the dist folder is normally not added to github, the dist folder gets renamed to instead be something else. Here I rename it to be the same name used at codePen, which is registration3

> ren dist registration3

We also don’t need the src folder anymore, as that contains the codePen code which we aren’t using any more. So the src folder gets removed.

> del src\*.*
> rmdir src

I then create a github repository on github.com and issue some git (download and install it) commands at the command prompt to add these files to the repository. I only mention this here as a record of what I’ve done. You shouldn’t use these commands yourself at all unless you are adding code to your own separate github account.

> git init
> git remote add origin git@github.com:pmw57/remove-duplication
> git pull
> git branch --set-upstream-to=origin/main main
> git checkout main
> git add .
> git commit -m "Initial commit"
> git push -u origin main

That’s a lot of detail there about setting up new content for a repository. It’s a little bit complex these days because we are transitioning away from the term master to using main instead and not all of the tooling is updated yet to do that automatically. Because of that there is some extra work in the process.

Retrieve code from the repository

Instead of you doing the above git stuff, all that you might want to do is to either, download and install git so that you can clone a repository at the command prompt:

> git clone https://github.com/pmw57/remove-duplication

or to retrieve the content from the github repository by downloading the source code as a zip file from the releases section.

• https://github.com/pmw57/remove-duplication/releases/tag/v0.0.1

After that you can use node (get and install Node to install the node_modules with:

> cd remove-duplication
> npm install

and can then follow along with the jsinspect:

> npx jsinspect registration3

I’ve added some info in the readme.md file about this as well.

Summary

We have setup Node and used jsInspect to start inspecting the code for duplication to remove. The code has also been added to a github respository so that you can easily retrieve the code at different stages of the process, to help you explore what’s being done at different stages of the process.

The code as it stands at the end of this post can be retrieved from https://github.com/pmw57/remove-duplication/releases/tag/v0.0.1

Next up we will look at one of the cases of duplication, and work on improving that section of code.

[Paul_Wilkins]

As we are set up to use jsInspect, we can now start looking at duplication in the code.

Today we will do the following:

• use jsInspect to find a case of duplication
• add some tests to keep track of what the code is supposed to do

That helps us to make sure that nothing breaks. After getting some tests in place, modifying the code can then happen afterwards.

Duplication in reset button code

Running jsInspect shows 24 sets of duplication throughout the code. Because it’s a very long list, I’m going to work through it starting from the very last item that it reports.

Match - 2 instances
> npx jsinspect registration3

...

./registration3\validate.js:1940,1942
if (value == "") {
   $(this).find(".error").html(name).removeClass("warning").addClass("ok");
   $(this).find(".feedback").removeClass("glyphicon glyphicon-ok").removeClass("glyphicon glyphicon-remove").removeClass("warning");

./registration3\validate.js:1946,1948
                 } else {
$(this).find(".error").html(name).removeClass("warning").addClass("ok");
                     $(this).find(".feedback").removeClass("glyphicon glyphicon-ok").removeClass("glyphicon glyphicon-ok").removeClass("ok");

24 matches found across 4 files

This section of duplication is in the btn2 section of code for the reset button, where the value is being checked if it’s empty.

It is irresponsible to change the code without having some way to ensure that the code still continues to work properly. I’m going to use Mocha to make tests that automically work to help ensure that the code works without error.

Install Mocha

The main options for test libraries are:

• Jasmine (standalone)
• Mocha (Node or standalone)
• Jest (Node)

We will have to use a standalone test library because the code we’re testing is not structured as node modules.
As we are already using Node with jsInpect, it makes good sense to use Mocha, as that has improved capabilities, while supporting standalone testing without needing Node.

Mocha works together with Chai, where Mocha runs the tests and Chai provides an improved way of asserting what to expect. Mocha (and Chai) is easily installed using Node.

> npm install --save-dev mocha chai

We can also add a tests folder to help keep our tests separate from the code that we’re testing.

> mkdir tests

Standalone tester

Typically Mocha is run from Node and shows the testing results at the command prompt. The code we’re working with though isn’t organised as node modules for that to easily occur. Because we don’t have the code as modules, we can use standalone testing in the HTML page itself.

The standalone testing typically shows the results in the web page. We won’t be doing that here. Instead of messing with the page, I’m setting the reporter so that the results appear in the browser console instead.

<!-- tests -->
<script src="../node_modules/mocha/mocha.js"></script>
<script src="../node_modules/chai/chai.js"></script>
<script>
    mocha.setup({
        ui: "bdd",
        reporter: "spec"
        // reporter: "dot"
    });
</script>
<script src="../tests/validate.test.js"></script>
<script>
    mocha.run();
</script>

• bdd interface lets us use describe, it, and expect for the tests
• spec reporter shows us the tests in the browser console
• dot matrix reporter is an alternative minimal view

Create test

Here is the reset form code that we are focused on right now:

    if (value == "") {
        $(this).find(".error").html(name).removeClass("warning").addClass("ok");
        $(this).find(".feedback").removeClass("glyphicon glyphicon-ok").removeClass("glyphicon glyphicon-remove").removeClass("warning");
        $(this).find(".starrq").removeClass("warning").addClass("ok");
        $("#termcheck").removeClass('ok').addClass('warning');
        $("#termsRequired").removeClass('ok').addClass('warning');
    } else {
        $(this).find(".error").html(name).removeClass("warning").addClass("ok");
        $(this).find(".feedback").removeClass("glyphicon glyphicon-ok").removeClass("glyphicon glyphicon-ok").removeClass("ok");
        $(this).find(".starrq").removeClass("warning").addClass("ok");
        $("#termcheck").addClass('ok').removeClass('warning');
        $("#termsRequired").addClass('ok').removeClass('warning');
    }

The duplicate code is doing two different things. It’s doing something when the value is empty, and it’s also doing something else when the value is not empty. It all happens though when the form is reset, so we can use describe as a container for our tests.

validate.test.js

describe("When form is reset, reset input classes", function () {
    
});

When it comes to the tests, we want to have the simplest way to trigger the behaviour. About the only way that we currently have of checking that the code works, is to trigger the reset field button. We can then check if certain things get done.

describe("When form is reset, reset input classes", function () {
    it("removes warning from error", function () {
        const formGroup = $(".form-group")[0];
        const field = $(formGroup).find(".check")[0];
        const $error = $(formGroup).find(".error");
        field.value = "";
        $error.addClass("warning");
        $(".btn2").trigger("click");
        expect($error.attr("class")).to.not.contain("warning");
    });
});

Fixing a few problems

The click doesn’t work though, because when the test runs the click trigger doesn’t yet exist. The document.ready part of the jQuery code delays the setup of the click handler resulting in the code not being ready when the test runs. We must delay the test until after the code has run. We can use setTimeout as a good way to achieve that.

<script>
    /// mocha.run();
    const delayms = 0;
    setTimeout(mocha.run, delayms);
</script>

A new problem now occurs, saying TypeError: Cannot read property 'preventDefault' of undefined

That happens because the global event object is being used in the code, which has trouble when jQuery triggers an event.

    event.preventDefault();

Instead of using the global event object, we should instead give an event parameter to the function handler, and get the event from there instead.

Many coders prefer to use evt isntead of event too, to help make it clear that the global event object is not being used.

        // $(".btn2").click(function () {
        $(".btn2").click(function (evt) {
            ...
            evt.preventDefault();

and the test now works. When we run the html page we are told in the browser console that the test passes.

Verifying that the test works

But - how do we know that the test will report a fail when the code fails? We can temporarily comment out a line of code to try that out:

        // $(this).find(".error").html(name).removeClass("warning").addClass("ok");

and the test now shows us what went wrong and where:

AssertionError: expected 'error warning' to not include 'warning' at Context.<anonymous> (file:///C:/.../remove-duplication/tests/validate.test.js:13:47)

We can now put the code back

        $(this).find(".error").html(name).removeClass("warning").addClass("ok");

and add more tests for this section of code that we are going to be updating.

The .feedback, .starrq #termcheck and #termsRequired elements are also affected in the duplicate code, so we should add tests to ensure that those behave properly too when we later on make changes to the code.

Recording existing behaviour

When writing tests, it’s important to test one thing at a time. That way when something goes wrong, we have better information about the trouble.

Normally a test is written before adding the code to make that test pass. What’s happening here is quite the reverse, but it’s still vital. Because we have no tests, we have no reliable way of ensuring that everything still works after making changes. The purpose of these tests is to records details of what actually occurs, so that I have assurance when making changes to the code that the code still works in the same way that it did before.

I’m not doing a full suite of tests here. That would involve ensuring that all form fields behave in the same way. Right now I’m only checking the behaviour of the first form field because it looks like all of the other form fields behave in the same way, and checking the terms and conditions section because that it different from all of the rest.

That is an assumption that I’m openly making here. It is though enough tests to catch unplanned mistakes when modifying the code. If full coverage testing is desired later on, we can certainly move forward to do that, for which there are coverage tools to help us more fully achieve that.

Here is a full set of tests for the reset form code:

describe("When form is reset, update classes", function () {
    const formGroup = $(".form-group")[0];
    const field = $(formGroup).find(".check")[0];
    const $error = $(formGroup).find(".error");
    const $feedback = $(formGroup).find(".feedback");
    const $starrq = $(formGroup).find(".starrq");
    const $termcheck = $("#termcheck");
    const $termsRequired = $("#termsRequired");
    
    function resetValues() {
        $(".form-group .check").each(function () {
            this.value = this.defaultValue;
        });
    }
    beforeEach(function () {
        resetValues();
    });
    it("removes warning from error", function () {
        $error.addClass("warning");
        $(".btn2").trigger("click");
        expect($error.attr("class")).to.not.contain("warning");
    });
    it("adds ok to error", function () {
        $error.removeClass("ok");
        $(".btn2").trigger("click");
        expect($error.attr("class")).to.contain("ok");
    });
    it("removes glyphicon from feedback", function () {
        $feedback.addClass("glyphicon");
        $(".btn2").trigger("click");
        expect($feedback.attr("class")).to.not.contain("glyphicon");
    });
    it("removes glyphicon-ok from feedback", function () {
        $feedback.addClass("glyphicon-ok");
        $(".btn2").trigger("click");
        expect($feedback.attr("class")).to.not.contain("glyphicon-ok");
    });
    it("removes warning from feedback", function () {
        $feedback.addClass("warning");
        $(".btn2").trigger("click");
        expect($feedback.attr("class")).to.not.contain("warning");
    });
    it("removes warning from starrq", function () {
        $starrq.addClass("warning");
        $(".btn2").trigger("click");
        expect($starrq.attr("class")).to.not.contain("warning");
    });
    it("adds ok to starrq", function () {
        $starrq.removeClass("ok");
        $(".btn2").trigger("click");
        expect($starrq.attr("class")).to.contain("ok");
    });
    it("adds checkbox ok to termcheck", function () {
        $termcheck.removeClass("ok");
        $(".btn2").trigger("click");
        expect($termcheck.attr("class")).to.contain("ok");
    });
    it("removes checkbox warning from termcheck", function () {
        $termcheck.addClass("warning");
        $(".btn2").trigger("click");
        expect($termcheck.attr("class")).to.not.contain("warning");
    });
    it("adds required ok to termsRequired", function () {
        $termsRequired.removeClass("ok");
        $(".btn2").trigger("click");
        expect($termsRequired.attr("class")).to.contain("ok");
    });
    it("removes required warning from termsRequired", function () {
        $termsRequired.addClass("warning");
        $(".btn2").trigger("click");
        expect($termsRequired.attr("class")).to.not.contain("warning");
    });
});

And as a reminder, here is the reset form code:

    if (value == "") {
        $(this).find(".error").html(name).removeClass("warning").addClass("ok");
        $(this).find(".feedback").removeClass("glyphicon glyphicon-ok").removeClass("glyphicon glyphicon-remove").removeClass("warning");
        $(this).find(".starrq").removeClass("warning").addClass("ok");
        $("#termcheck").removeClass('ok').addClass('warning');
        $("#termsRequired").removeClass('ok').addClass('warning');
    } else {
        $(this).find(".error").html(name).removeClass("warning").addClass("ok");
        $(this).find(".feedback").removeClass("glyphicon glyphicon-ok").removeClass("glyphicon glyphicon-ok").removeClass("ok");
        $(this).find(".starrq").removeClass("warning").addClass("ok");
        $("#termcheck").addClass('ok').removeClass('warning');
        $("#termsRequired").addClass('ok').removeClass('warning');
    }

There’s a lot of details in tests, but I’ve grouped them by each item that’s affected (error, feedback, starq, terms). Sometimes it’s tempting to test multiple things at once, but tests should only test one thing at a time.

Creating the tests also taught me some things about the code. For example, the if (value == "") condition really shouldn’t be there. In this reset handler all fields are already empty. The only thing that the condition is doing is to provide a separation for the terms and conditions checkbox. There are better structures that help us to understand what is happening there instead, but that’s for later.

Running the index.html file, the browser console shows that all of the tests are passing.

Summary

When any line of the reset form code is commented out, the tests tell us what’s gone wrong and what’s expected. That bodes well for when we make changes to the reset form code, to improve it. The objective is to remove duplication, and leave the code in a better state than how it began.

We are now well set up to remove the duplication and ensure that the code still continues to work in exactly the same way that it does now, which we’ll do in the next post.

[Paul_Wilkins]

Today we will do the following:

• update the code to remove duplication found by jsInspect
• use the tests to check that the updated code still works

Update the duplicated code

Both sections of the duplicated code are very similar. We can use separate functions to help remove that duplication.

Here is the full section of code that needs updating:

    if (value == "") {
        $(this).find(".error").html(name).removeClass("warning").addClass("ok");
        $(this).find(".feedback").removeClass("glyphicon glyphicon-ok").removeClass("glyphicon glyphicon-remove").removeClass("warning");
        $(this).find(".starrq").removeClass("warning").addClass("ok");
        $("#termcheck").removeClass('ok').addClass('warning');
        $("#termsRequired").removeClass('ok').addClass('warning');
    } else {
        $(this).find(".error").html(name).removeClass("warning").addClass("ok");
        $(this).find(".feedback").removeClass("glyphicon glyphicon-ok").removeClass("glyphicon glyphicon-ok").removeClass("ok");
        $(this).find(".starrq").removeClass("warning").addClass("ok");
        $("#termcheck").addClass('ok').removeClass('warning');
        $("#termsRequired").addClass('ok').removeClass('warning');
    }

Now that we have tests in place to tell us when something goes wrong, there is now no fear in making changes to the code.

Use a removeError function

First, we can move that duplicated error update into a removeError function:

    function removeError(el) {
       $(el).find(".error").html(name).removeClass("warning").addClass("ok");
    }
...
    if (value == "") {
        // $(this).find(".error").html(name).removeClass("warning").addClass("ok");
        removeError(this);
        ...
    } else {
        removeError(this);
        // $(this).find(".error").html(name).removeClass("warning").addClass("ok");
        ...
    }

And as the removeError is duplicated in both parts of the if statement, we can move it up above the if.

    removeError(this);
    if (value == "") {
        // removeError(this);
        ...
    } else {
        // removeError(this);
        ...
    }

Use removeTermWarning and addTermWarning functions

The rest of the code in each section of the if statement, either rely on the this variable, or refer to the terms and conditions. Those different parts can be extracted out to different functions.

    function resetWarnings(el) {
        $(el).find(".feedback").removeClass("glyphicon glyphicon-ok").removeClass("glyphicon glyphicon-remove").removeClass("warning");
        $(el).find(".starrq").removeClass("warning").addClass("ok");
    }
    function resetFieldOk(el) {
        $(el).find(".feedback").removeClass("glyphicon glyphicon-ok").removeClass("glyphicon glyphicon-ok").removeClass("ok");
        $(el).find(".starrq").removeClass("warning").addClass("ok");
    }
    function addTermWarning() {
        $("#termcheck").removeClass('ok').addClass('warning');
        $("#termsRequired").removeClass('ok').addClass('warning');
    }
    function removeTermWarning() {
        $("#termcheck").addClass('ok').removeClass('warning');
        $("#termsRequired").addClass('ok').removeClass('warning');
    }
...
                if (value == "") {
                    resetWarnings(this);
                    addTermWarning();
                } else {
                    resetFieldOk(this);
                    removeTermWarning();
                }

Some of that code is never used. The addTermWarning() function will always be replaced by what removeTermWarning() function does, because the terms and conditions at the end of the function is the only part that causes the else clause to run. The addTermWarning() function can be completely removed without changing anything about what happens.

//     function addTermWarning() {
//         $("#termcheck").removeClass('ok').addClass('warning');
//         $("#termsRequired").removeClass('ok').addClass('warning');
//     }
...
                if (value == "") {
                    resetWarnings(this);
                    // addTermWarning();
                } else {

The commented-out code is no longer needed, and can be entirely deleted.

The tests that we made all still pass, which is a pleasing sign.

Restructure the code

The if statement really is the wrong tool for the job. What we want to do instead is to run the removeError and resetWarnings on each form group, and after that to do something different with the terms and conditions.

After refactoring the code into several functions, while staying true to the requirements in the tests, I end up with the following simplification for the reset click handler.

        $(".btn2").click(function resetClickHandler(evt) {
            $(".form-group").each(resetMessages);
            removeTermWarning();
        });

The resetMessages() and removeTermWarning() functions contain the bulk of the code:

        function resetMessages(i, formGroup) {
            const $error = $(formGroup).find(".error");
            const name = $(formGroup).find(".check").attr("name");
            $error.html(name);
            resetWarning($error);
            resetFeedback($(formGroup).find(".feedback"));
            resetWarning($(formGroup).find(".starrq"));
        }
        function removeTermWarning() {
            const $termsGroup = $("#terms").closest(".form-group");
            resetFeedback($termsGroup.find(".feedback"));
            resetWarning($("#termcheck"));
            resetWarning($("#termsRequired"));
        }

Those functions had several examples of excessive duplication, such as the resetWarning ones. I’ve moved those out to separate functions.

The resetWarning and resetFeedback functions help to remove other duplication.

       function replaceClass(el, oldClass, newClass) {
            $(el).removeClass(oldClass).addClass(newClass);
        }
        function resetFeedback($el) {
            $el.removeClass("glyphicon glyphicon-ok glyphicon-remove warning ok");
        }
        function resetWarning($el) {
            replaceClass($el, "warning", "ok");
        }

The named functions don’t only improve the ability to understand what the code does, but the smaller functions become useful too when other code needs to do the same thing. The replaceClass function I can tell is going to be used a lot by other code.

The final code that we’re left with is easier to understand, and easier in which to spot potential issues.

        function replaceClass(el, oldClass, newClass) {
            $(el).removeClass(oldClass).addClass(newClass);
        }
        function resetWarning($el) {
            replaceClass($el, "warning", "ok");
        }
        function resetFeedback($el) {
            $el.removeClass("glyphicon glyphicon-ok glyphicon-remove warning ok");
        }
        function resetMessages(i, formGroup) {
            const $error = $(formGroup).find(".error");
            const name = $(formGroup).find(".check").attr("name");
            $error.html(name);
            resetWarning($error);
            resetFeedback($(formGroup).find(".feedback"));
            resetWarning($(formGroup).find(".starrq"));
        }
        function removeTermWarning() {
            const $termsGroup = $("#terms").closest(".form-group");
            resetFeedback($termsGroup.find(".feedback"));
            resetWarning($("#termcheck"));
            resetWarning($("#termsRequired"));
        }
        $(".btn2").click(function (evt) {
            $(".form-group").each(resetMessages);
            removeTermWarning();
        });

And, as the replaceClass and resetWarning functions are going to be used by other parts of the validate code, I’ve moved them to the top of the validate.js file.

Cleaning up

While working with code it’s a good policy to leave things better than it was, so I’ve tidied up the indenting of the code in the btn2 click function, and removed unwanted commented out code.

The document ready end comment above the btn2 click function is also removed because that’s clearly in the wrong place, leaving another copy of it below the code. That comment is a clear sign that the code is too large, but I’ll leave it there for now so-as to not get too distracted by things.

The HTML code for btn2 also has an unused onclick attribute, using a myFunction() function that doesn’t exist. I’ve removed that unused onclick attribute too.

Also, as I’m been doing some of this work offline while travelling, I’ve moved some internet files into a lib folder so that the test page still works when I’m offline.

Summary

We have successfully dealt with first set of duplication, and even though the updated code looks dramatically different from what was there before, the tests for that code all still pass. That gives us reassurance that everything still works exactly the same now as it did before, and things in that area are cleaned up better than they were.

The code as it stands at the end of this post can be retrieved from https://github.com/pmw57/remove-duplication/releases/tag/v0.0.2

There is a lot more duplication to be dealt with though, so we’ll move on with more of it in the next post.

[Paul_Wilkins]

Here’s what we’ll do today:

• ignore library files
• examine the duplication area
• use tests to detail the code behaviour
• remove the duplication
• simplify the tests

Ignore library files

When we run inspect.bat to use jsInspect, we are given confusing duplication from the bootstrap and jquery files that were downloaded into the library folder lib in the previous post. As they are in a separate folder, we can easily tell jsInspect to ignore them.

> npx jsinspect registration3 --ignore lib

To save us the trouble from typing that in each time, we can save that command into a batch file

inspect.bat

@echo off
npx jsinspect registration3 --ignore lib

We can now run that from the command prompt with:

> inspect

and are shown 23 sets of duplication that exist in the code.

Duplication in change-password.js at line 189

The expanded view of the code makes it quite difficult to get your head around the code. As a result I am restoring the code back to one line per statement, so that it’s easier to tell what’s going on.

For example, turning this expanded set of code:

      $(this)
        //.next()
        .find(".error")
        .html("Your " + st);
      $(this)
        .find(".error")
        .css("color", "green");

      $(this)
        //.next()
        .find(".feedback")
        //.html("Your " + st + " is OK ")
        //.removeClass("glyphicon glyphicon-remove warning")
          .removeClass("glyphicon glyphicon-ok ok");
      $(this)
        .next()
        .find(".error")
        .removeClass("warning")
        .addClass("ok");

into the following:

        $(this).find(".error").html("Your " + st);
        $(this).find(".error").css("color", "green");                      
        $(this).find(".feedback").removeClass("glyphicon glyphicon-ok ok");
        $(this).next().find(".error").removeClass("warning").addClass("ok");

Here’s what we have after cleaning up the code in the reported section:

    $(".button2color").click(function() {
        $(".inputboxmodal2").each(function() {
            var st = $(this).find(".input-check").attr("name");
            var st2 = $(this).find(".input-check").val().trim();
            if ($(this).find(".input-check").val().trim() != "") {
                $(this).find(".error").html("Your " + st);
                $(this).find(".error").css("color", "green");                      
                $(this).find(".feedback").removeClass("glyphicon glyphicon-ok ok");
                $(this).next().find(".error").removeClass("warning").addClass("ok");
            } else {
                $(this).find(".error").html("Your " + st);
                $(this).find(".error").css("color", "green");
                $(this).find(".feedback").removeClass("glyphicon glyphicon-remove");
                $(this).find(".error").addClass("ok");
            }
        });
    });

The duplication is easily seen in both sections of the if statement.

I want to start cleaning this up by improving the condition in the if statement, but before doing that I need to get this code into the right place. The .button2color is for the change-password reset button. That code is currently in login.js file, as is the rest of the inputboxmodel2 code. I have the filenames switched around for login.js and change-password.js. Switching those names around results in the code being in the correctly named files.

After updating the files, we should also update .button2color so that it more clearly tells us what it’s for.

    // $(".button2color").click(function () {
    $("#changepw [type=reset]").click(function passwordResetHandler() {
        // $(".inputboxmodal2").each(function () {
        $(".inputboxmodal2").each(function resetInputMessages() {

Before updating the rest of the code more tests are needed, to ensure that our changes to improve the code don’t change the behaviour of what the code is supposed to do.

Tests

The first set of code to test is from the if statement:

    if ($(this).find(".input-check").val().trim() != "") {
        $(this).find(".error").html("Your " + st);
        $(this).find(".error").css("color", "green");                      
        $(this).find(".feedback").removeClass("glyphicon glyphicon-ok ok");
        $(this).next().find(".error").removeClass("warning").addClass("ok");

As the $(".inputboxmodal2").each method is used to refer to different inputboxmodal2 sections, I’ll just use the first section with which to test.

The != "" part is odd though, because when resetting the form everything seems to already be set to empty. I can check if we ever get there in a test by seeing if a console.log statement ever appears.

    if ($(this).find(".input-check").val().trim() != "") {
        console.log("a filled input field");
        ...

And yes, when selecting login and change password, with something in one of the form fields, clicking Reset causes the console.log to appear in the browser console, after which the browser empties the form fields. Testing both filled and empty fields will need to be tested, at least initially, and the console.log line is no longer needed.

Testing the error content

For now, the change-password test needs to put a value in one of the fields, of which the email field is suitable. While it’s possible to use the email’s id attribute to access it, it’s prefereable instead to use the same technique that the code uses to get the field as that is a more reliable way to access what the code accesses.

When structuring a test, it’s always helpful to use a given/when/then structure for each test.

1. given that the email value has some content
2. when the change-password form Reset button is clicked
3. then the email error should change to be “Your E-mail”

./tests/change-password.test.js

describe("Change-password form is reset", function () {
    const inputGroup = $(".inputboxmodal2:first");
    const $emailInput = $inputGroup.find(".input-check");
    const $emailError = $inputGroup.find(".error");
    it("Resets the error message when value has content", function () {
        $emailInput.value = "test@example.com";
        $emailError.html("test content");
        $("#changepw [type=reset]").trigger("click");
        expect($emailError.html()).to.equal("Your E-mail");
    });
});

I might as well keep tests for the same thing grouped together. The other html thing that happens is in the else clause, when the value is empty.

describe("When reset password form is reset, reset input classes", function () {
    const $inputGroup = $(".inputboxmodal2");
    const $emailInput = $inputGroup.find(".input-check");
    const $emailError = $inputGroup.find(".error");
    it("Resets the error message when value has content", function () {
        $emailInput.value = "test@example.com";
        $emailError.html("test content");
        $("#changepw [type=reset]").trigger("click");
        expect($emailError.html()).to.equal("Your E-mail");
    });
    it("Resets the error message when value is empty", function () {
        $emailInput.value = "";
        $emailError.html("test content");
        $("#changepw [type=reset]").trigger("click");
        expect($emailError.html()).to.equal("Your E-mail");
    });
});

It’s quite clear now that that both behave exactly the same regardless of the value, so we can combine the two together and ignore the value when testing the html value.

describe("When reset password form is reset, reset input classes", function () {
    const $inputGroup = $(".inputboxmodal2");
    const $emailInput = $inputGroup.find(".input-check");
    const $emailError = $inputGroup.find(".error");
    it("Resets the error message", function () {
        $emailError.html("test content");
        $("#changepw [type=reset]").trigger("click");
        expect($emailError.html()).to.equal("Your E-mail");
    });
});

Test the error color

The error color is tested in a similar way:

describe("When reset password form is reset, reset input classes", function () {
    const $inputGroup = $(".inputboxmodal2");
    const $emailInput = $inputGroup.find(".input-check");
    const $emailError = $inputGroup.find(".error");
    ...
    it("Resets the error color", function () {
        $emailError.removeClass("green");
        $("#changepw [type=reset]").trigger("click");
        expect($emailError.attr("class")).to.contain("green");
    });
});

Test the feedback classes

The feedback behaviour differs based on whether the value has content or not, so separate tests will be needed for some of it.

    const $emailFeedback = $inputGroup.find(".feedback");
    ...
    it("Removes the feedback glyphicon class", function () {
        $emailFeedback.addClass("glyphicon");
        $("#changepw [type=reset]").trigger("click");
        expect($emailFeedback.attr("class")).to.not.contain("glyphicon");
    });
    it("Removes the feedback glyphicon-ok class when value has content", function () {
        $emailInput.val("test content");
        $emailFeedback.addClass("glyphicon-ok");
        $("#changepw [type=reset]").trigger("click");
        expect($emailFeedback.attr("class")).to.not.contain("glyphicon-ok");
    });
    it("Removes the feedback ok class when value has content", function () {
        $emailInput.val("test content");
        $emailFeedback.addClass("ok");
        $("#changepw [type=reset]").trigger("click");
        expect($emailFeedback.attr("class")).to.not.contain("ok");
    });
    it("Removes the feedback glyphicon-remove class when value is empty", function () {
        $emailInput.val("");
        $emailFeedback.addClass("glyphicon-remove");
        $("#changepw [type=reset]").trigger("click");
        expect($emailFeedback.attr("class")).to.not.contain("glyphicon-remove");
    });

Test the error classes

When testing the error classes, I see that the JS code seems to have an error in it.

    $(this).next().find(".error").removeClass("warning").addClass("ok");

That code results in the next error section being found, resulting in the first error section not being affected. There doesn’t seem to be a good reason for that type of behaviour, so I’m making an executive decision to remove next() from that line.

The tests then end up being simpler, resulting in the following test code:

    it("Removes the error warning class when value has content", function () {
        $emailInput.val("test content");
        $emailError.addClass("warning");
        $("#changepw [type=reset]").trigger("click");
        expect($emailError.attr("class")).to.not.contain("warning");
    });
    it("Adds the error ok class", function () {
        $emailError.removeClass("ok");
        $("#changepw [type=reset]").trigger("click");
        expect($emailError.attr("class")).to.contain("ok");
    });

We are now ready to update the change-password reset code.

Improve the condition

The if condition is the first thing that I’ll improve. There are several different things in there to understand before you know what it’s supposed to do:

• this
• find
• input-check
• val
• trim
• not empty

It’s much easier to understand when there are less concepts to take in, for example with the following comparison:

        if (trimmedValue > "") {

We only need the inputValue to help out here, for which we can rename that st2 variable.

        var $inputField = $(this).find(".input-check");
        var trimmedValue = $inputField.val().trim();
        var st = $inputField.attr("name");
        if (trimmedValue > "") {

That st variable can be renamed to inputName, making it easier to understand.

        var trimmedValue = $inputField.val().trim();
        // var st = $inputField.attr("name");
        var inputName = $inputField.attr("name");
        if (trimmedValue > "") {
            // $(this).find(".error").html("Your " + st);
            $(this).find(".error").html("Your " + inputName);

On further investigation, we don’t even need the if condition. Most of the code in the if and else sections are a duplicate of each other, and the rest works regardless of the condition. When the condition is removed, the tests all still work, helping to comfirm that the if/else split doesn’t serve any beneficial purpose.

    $("#changepw [type=reset]").click(function passwordResetHandler() {
        $(".inputboxmodal2").each(function resetInputMessages() {
            var $inputField = $(this).find(".input-check");
            var inputName = $inputField.attr("name");
            $(this).find(".error").html("Your " + inputName);
            $(this).find(".error").css("color", "green");                      
            $(this).find(".error").removeClass("warning").addClass("ok");
            $(this).find(".feedback").removeClass("glyphicon glyphicon-remove glyphicon-ok ok");
        });
    });

That is a much better simplification, and the tests still work telling us that everything is all good.

Simplify the tests

Now that we don’t need the if statement for the value being filled or empty, we can simplify the tests too by removing its requirement on the value.

    // it("Removes the error warning class when value has content", function () {
    it("Removes the error warning class", function () {
        // $emailInput.val("test content");
        $emailError.addClass("warning");
        $("#changepw [type=reset]").trigger("click");
        expect($emailError.attr("class")).to.not.contain("warning");
    });
...
    // it("Removes the feedback glyphicon-ok class when value has content", function () {
    it("Removes the feedback glyphicon-ok class", function () {
        // $emailInput.val("test content");
        $emailFeedback.addClass("glyphicon-ok");
        $("#changepw [type=reset]").trigger("click");
        expect($emailFeedback.attr("class")).to.not.contain("glyphicon-ok");
    });
    // it("Removes the feedback ok class when value has content", function () {
    it("Removes the feedback ok class", function () {
        // $emailInput.val("test content");
        $emailFeedback.addClass("ok");
        $("#changepw [type=reset]").trigger("click");
        expect($emailFeedback.attr("class")).to.not.contain("ok");
    });
    // it("Removes the feedback glyphicon-remove class when value is empty", function () {
    it("Removes the feedback glyphicon-remove class", function () {
        // $emailInput.val("");
        $emailFeedback.addClass("glyphicon-remove");
        $("#changepw [type=reset]").trigger("click");
        expect($emailFeedback.attr("class")).to.not.contain("glyphicon-remove");
    });

Summary

We have used jsInspect to find another case of duplication, used tests to ensure we don’t break the code, and refactored the code to remove that duplication.

I have also fixed what looks to be an error when the change-password form is reset, by removing next() from the error statement.

The code as it stands now can be obtained from the v0.0.3 release

Running inspect.bat again shows that there are only 21 sets of duplication left. We are starting to make progress.

[Paul_Wilkins]

In todays post we will:

• try to figure out what is button1color1
• make form submit code easier to test
• group tests by form inputs, and behaviour
• use functions to simplify the code

Where is the duplication?

The jsInspect program run with inspect.bat tells us that there is duplication in login.js at 124. That is the login submit code.

$(".button1color1").click(function() {
    $(".inputboxmodal1").each(function() {
        var st = $(this).find(".input-check").attr("name");
        var st2 = $(this).find(".input-check").val().trim();
        if ($(this).find(".input-check").val().trim() != "") {
            $(this).find(".error").html("Your " + st + " is OK ");
            $(this).find(".error").css("color", "green");
            $(this).find(".feedback").removeClass("glyphicon glyphicon-remove").addClass("glyphicon glyphicon-ok ok");
            $(this).find(".error").removeClass("warning").addClass("ok");
        } else {
            $(this).find(".error").html("Your " + st + " is empty ");
            $(this).find(".error").css("color", "red");
            $(this).find(".feedback").removeClass("glyphicon glyphicon-ok ok").addClass("glyphicon glyphicon-remove warning");
            $(this).find(".error").removeClass("ok").addClass("warning");
            event.preventDefault();
        }
    });
});

What is .button1color1 ?

The code in this sections has a cryptic beginning:

$(".button1color1").click(function() {

I don’t know what this part of the code is supposed to do yet. The line that assigns the click handler doesn’t help much either.

Looking in the HTML code for .button1color1 I find that it’s a submit button.

    <button type="submit" class="btn btn-default button1 button1color1">Submit</button>

But what does it submit? Scanning up through the HTML code for a <form> element, I eventually find one that tells us that it’s the modal login form.

    <form id="login" class="form-horizontal container-fluid modallogin modalform">

So the initial .button1color1 selector is for the login form submit button.

It would have been a great help if the code was able to tell us that without needing all of the Sherlock Holmes investigation. A lazy solution is to add a comment to tell us that it’s the submit section from the login form. A better solution is to update the code so that it gives us that same information.

// $(".button1color1").click(function() {
$("#login [type=submit]").click(function() {

I haven’t removed button1color1 from the HTML code yet as it is still used by some things. I do though have an aim when working with the JS code to replace poorly named things with easier to understand names.

I’ve also fixed up the event issue from other code, using evt instead from the event function.

Here is the updated code that needs some tests, before we start fixing the duplication.

/* modal check*/
$("#login [type=submit]").click(function(evt) {
    $(".inputboxmodal1").each(function() {
        var st = $(this).find(".input-check").attr("name");
        var st2 = $(this).find(".input-check").val().trim();
        if ($(this).find(".input-check").val().trim() != "") {
            $(this).find(".error").html("Your " + st + " is OK ");
            $(this).find(".error").css("color", "green");
            $(this).find(".feedback").removeClass("glyphicon glyphicon-remove").addClass("glyphicon glyphicon-ok ok");
            $(this).find(".error").removeClass("warning").addClass("ok");
        } else {
            $(this).find(".error").html("Your " + st + " is empty ");
            $(this).find(".error").css("color", "red");
            $(this).find(".feedback").removeClass("glyphicon glyphicon-ok ok").addClass("glyphicon glyphicon-remove warning");
            $(this).find(".error").removeClass("ok").addClass("warning");
            evt.preventDefault();
        }
    });
});

Making things testable

We add the login tests to the testing section of the index.html file:

<script src="../tests/validate.test.js"></script>
<script src="../tests/login.test.js"></script>
<script src="../tests/change-password.test.js"></script>

Examining the code we see a similar structure as with the other sets of code that we’ve dealt with before.

Use evt instead event

The event part of the code we rename to evt. I think that it’s jQuery that causes the trouble there, but it’s easily taken care of by renaming event to evt and adding it as a parameter to the event handler function. This is also a good time to name those functions too:

// $("#login [type=submit]").click(function() {
$("#login [type=submit]").click(function loginSubmitHandler(evt) {
    // $(".inputboxmodal1").each(function() {
    $(".inputboxmodal1").each(function validateInputField() {
    ...
            // event.preventDefault();
            evt.preventDefault();

A problem with form submit

Here is a first test:

describe("When login form is submitted", function () {
    const $inputGroup = $(".inputboxmodal1");
    const $emailInput = $inputGroup.find(".input-check");
    const $emailError = $inputGroup.find(".error");
    it("Resets the error message when value has content", function () {
        $emailInput.val("test content");
        $emailError.html("test content");
        $("#login [type=submit]").trigger("click");
        expect($emailError.html()).to.equal("Your E-mail");
    });
});

The test works, but the web page keeps reloading because of the form submit event. We need to separate the loginSubmitHandler from the assignment of it to the submit button, so that the loginSubmitHandler function can be more easily tested. That is easily achieved by moving the loginSubmitHandler function up in the code to be a separate function.

// before
// $("#login [type=submit]").click(function loginSubmitHandler(evt) {
//     ...
// });

// after
function loginSubmitHandler(evt) {
    ...
}
$("#login [type=submit]").click(loginSubmitHandler);

The click event is somewhat limited for form events, such as reset or submit.

Pressing Enter on a form normally triggers the submit event. If you assign a click event to the submit button, pressing the Enter key somewhere else in the form still attempts to submit submit the form, but it bypasses any planned click handler that you have on the submit button.

The best way to deal with that is to stop using the click event on the submit or reset button, and instead bind the submit event handler to the submit or reset event of the whole form itself.

// $("#login [type=submit]").click(loginSubmitHandler);
$("#login").on("submit", loginSubmitHandler);

The loginSubmitHandler function can now be used to test in isolation, without triggering the form submit event.

describe("When login form is submitted", function () {
    const fakeEvt = {
        preventDefault: function fakeFunc() {
            return;
        }
    };
    const $inputGroup = $(".inputboxmodal1");
    const $emailInput = $inputGroup.find(".input-check");
    const $emailError = $inputGroup.find(".error");
    it("Resets the error message when value has content", function () {
        $emailInput.val("test content");
        $emailError.html("test content");
        loginSubmitHandler(fakeEvt);
        expect($emailError.html()).to.equal("Your E-mail is OK ");
    });
});

The test now works, but I’m not happy about the extra space in the string after the word OK. I’ll come back to that after all of the other tests are in place.

Login submit tests

The code that we’re testing does the same thing for each login form input, so I’m only going to put together tests for the first login form input field, the email address.

I also found that I was ending the name of each test with “when email has value” and “when email is empty” so I’ve separate those with and without parts into different sections in the test.

describe("When login form is submitted", function () {
    const $inputGroup = $(".inputboxmodal1");
    const $emailInput = $inputGroup.find(".input-check").first();
    const $emailError = $inputGroup.find(".error");
    const $emailFeedback = $inputGroup.find(".feedback");
    const CSSred = "rgb(255, 0, 0)";
    const CSSgreen = "rgb(0, 128, 0)";
    const fakeEvt = {
        preventDefault: function fakeFunc() {}
    };
    describe("error message", function () {
        describe("email has value", function () {
            beforeEach(function () {
                $emailInput.val("test value");
            });
            it("Resets the error message", function () {
                $emailError.html("test content");
                loginSubmitHandler(fakeEvt);
                expect($emailError.html()).to.equal("Your E-mail is OK ");
            });
            it("Resets the error color", function () {
                $emailError.css("color", "");
                loginSubmitHandler(fakeEvt);
                expect($emailError.css("color")).to.equal(CSSgreen);
            });
            it("Removes the error warning class", function () {
                $emailError.addClass("warning");
                loginSubmitHandler(fakeEvt);
                expect($emailError.attr("class")).to.not.contain("warning");
            });
            it("Adds the error ok class", function () {
                $emailError.removeClass("ok");
                loginSubmitHandler(fakeEvt);
                expect($emailError.attr("class")).to.contain("ok");
            });
        });
        describe("email is empty", function () {
            beforeEach(function () {
                $emailInput.val("");
            });
            it("Gives an error message", function () {
                $emailError.html("test content");
                loginSubmitHandler(fakeEvt);
                expect($emailError.html()).to.equal("Your E-mail is empty ");
            });
            it("Gives error color", function () {
                $emailError.css("color", "");
                loginSubmitHandler(fakeEvt);
                expect($emailError.css("color")).to.equal(CSSred);
            });
            it("Adds the error warning class", function () {
                $emailError.removeClass("warning");
                loginSubmitHandler(fakeEvt);
                expect($emailError.attr("class")).to.contain("warning");
            });
            it("Removes the error ok class", function () {
                $emailError.addClass("ok");
                loginSubmitHandler(fakeEvt);
                expect($emailError.attr("class")).to.not.contain("ok");
            });
        });
    });
    describe("feedback message", function () {
        describe("email has value", function () {
            beforeEach(function () {
                $emailInput.val("test value");
            });
            it("Removes the feedback glyphicon-remove class", function () {
                $emailFeedback.addClass("glyphicon-remove");
                loginSubmitHandler(fakeEvt);
                expect($emailFeedback.attr("class")).to.not.contain("glyphicon-remove");
            });
            it("Adds the feedback glyphicon class", function () {
                $emailFeedback.removeClass("glyphicon");
                loginSubmitHandler(fakeEvt);
                expect($emailFeedback.attr("class")).to.contain("glyphicon");
            });
            it("Adds the feedback glyphicon-ok class", function () {
                $emailFeedback.removeClass("glyphicon-ok");
                loginSubmitHandler(fakeEvt);
                expect($emailFeedback.attr("class")).to.contain("glyphicon-ok");
            });
            it("Adds the feedback ok class", function () {
                $emailFeedback.removeClass("ok");
                loginSubmitHandler(fakeEvt);
                expect($emailFeedback.attr("class")).to.contain("ok");
            });
        });
        describe("email is empty", function () {
            beforeEach(function () {
                $emailInput.val("");
            });
            it("Removes the feedback glyphicon-ok class", function () {
                $emailFeedback.addClass("glyphicon-ok");
                loginSubmitHandler(fakeEvt);
                expect($emailFeedback.attr("class")).to.not.contain("glyphicon-ok");
            });
            it("Removes the feedback ok class", function () {
                $emailFeedback.addClass("ok");
                loginSubmitHandler(fakeEvt);
                expect($emailFeedback.attr("class")).to.not.contain("ok");
            });
            it("Adds the feedback glyphicon class", function () {
                $emailFeedback.removeClass("glyphicon");
                loginSubmitHandler(fakeEvt);
                expect($emailFeedback.attr("class")).to.contain("glyphicon");
            });
            it("Adds the feedback glyphicon-remove class", function () {
                $emailFeedback.removeClass("glyphicon-remove");
                loginSubmitHandler(fakeEvt);
                expect($emailFeedback.attr("class")).to.contain("glyphicon-remove");
            });
        });
    });
});

Improving the code

Now that we have tests in place we can work on improving the code. Here is how it starts:

Tidy up variable names

function loginSubmitHandler(evt) {
    $(".inputboxmodal1").each(function validateInputField() {
        var st = $(this).find(".input-check").attr("name");
        var st2 = $(this).find(".input-check").val().trim();
        if ($(this).find(".input-check").val().trim() != "") {
            $(this).find(".error").html("Your " + st + " is OK ");
            $(this).find(".error").css("color", "green");
            $(this).find(".feedback").removeClass("glyphicon glyphicon-remove").addClass("glyphicon glyphicon-ok ok");
            $(this).find(".error").removeClass("warning").addClass("ok");
        } else {
            $(this).find(".error").html("Your " + st + " is empty ");
            $(this).find(".error").css("color", "red");
            $(this).find(".feedback").removeClass("glyphicon glyphicon-ok ok").addClass("glyphicon glyphicon-remove warning");
            $(this).find(".error").removeClass("ok").addClass("warning");
            evt.preventDefault();
        }
    });
}

Starting from the top of the code, the st and st2 variables need renaming to better names. Instead of st I’ll used inputName, and for st2 I’ll use trimmedValue.

        // var st = $(this).find(".input-check").attr("name");
        var inputName = $(this).find(".input-check").attr("name");
        // var st2 = $(this).find(".input-check").val().trim();
        var trimmedValue = $(this).find(".input-check").val().trim();
        ...
            // $(this).find(".error").html("Your " + st + " is OK ");
            $(this).find(".error").html("Your " + inputName + " is OK ");
        ...
            // $(this).find(".error").html("Your " + st + " is empty ");
            $(this).find(".error").html("Your " + inputName + " is empty ");

The trimmedValue variable isn’t used anywhere, but I suspect that it was intended to be used in the if condition.

        var inputName = $(this).find(".input-check").attr("name");
        var trimmedValue = $(this).find(".input-check").val().trim();
        // if ($(this).find(".input-check").val().trim() != "") {
        if (trimmedValue !== "") {

Remove trailing spaces in strings

The trailing space I was concerned about when doing the tests, can now be dealt with.

As this means changing the behaviour of the code itself, the test is updated first. After updated test fails, we change the code so that the test then passes.

With the test, we don’t want the trailing space to be at the end of the string anymore.

    it("Resets the error message", function () {
        $emailError.html("test content");
        loginSubmitHandler(fakeEvt);
        // expect($emailError.html()).to.equal("Your E-mail is OK ");
        expect($emailError.html()).to.equal("Your E-mail is OK");
    });
...
        it("Gives an error message", function () {
            $emailError.html("test content");
            loginSubmitHandler(fakeEvt);
            // expect($emailError.html()).to.equal("Your E-mail is empty ");
            expect($emailError.html()).to.equal("Your E-mail is empty");
        });

The tests now report a problem, and we can update the code so that the tests passes.

        // $(this).find(".error").html("Your " + inputName + " is OK ");
        $(this).find(".error").html("Your " + inputName + " is OK");
        ...
        // $(this).find(".error").html("Your " + inputName + " is empty ");
        $(this).find(".error").html("Your " + inputName + " is empty");

Simplify if statement

Most of the time with if statements, they should use a function call to direct the flow of events. Each part of the existing if statement should be moved in to separate functions.

function removeError(inputGroup, message) {
    $(inputGroup).find(".error").html(message);
    $(inputGroup).find(".error").css("color", "green");
    $(inputGroup).find(".error").removeClass("warning").addClass("ok");
    $(inputGroup).find(".feedback").removeClass("glyphicon glyphicon-remove").addClass("glyphicon glyphicon-ok ok");
}
function addError(inputGroup, message) {
    $(inputGroup).find(".error").html(message);
    $(inputGroup).find(".error").css("color", "red");
    $(inputGroup).find(".error").removeClass("ok").addClass("warning");
    $(inputGroup).find(".feedback").removeClass("glyphicon glyphicon-ok ok").addClass("glyphicon glyphicon-remove warning");
}
...
        if (trimmedValue !== "") {
            removeError(this, "Your " + inputName + " is OK");
        } else {
            addError(this, "Your " + inputName + " is empty");
            evt.preventDefault();
        }

Those addError and removeError functions can be used elsewhere in the code too, to help simplify the way that things are done.

Summary

The login submit code has been updated to remove duplication and make it easier to test, and to understand what it does.

function loginSubmitHandler(evt) {
    $(".inputboxmodal1").each(function validateInputField() {
        var inputName = $(this).find(".input-check").attr("name");
        var trimmedValue = $(this).find(".input-check").val().trim();
        if (trimmedValue !== "") {
            removeError(this, "Your " + inputName + " is OK");
        } else {
            addError(this, "Your " + inputName + " is empty");
            evt.preventDefault();
        }
    });
}
$("#login").on("submit", loginSubmitHandler);

The tests that we used before updating the code, still work, helping to confirm that this updated code still does the same thing that it did before. We’ve also learned the benefits of separating the event assignment from the function that handles the event.

The code as it is right now is avaiable as v0.0.4 from releases.

As several things have been learned over the course of these updates, in the next post we will go back over previous code applying those learnings, bringing that code up to date.

[Paul_Wilkins]

Today we will be doing the following:

• update the validate and change-password code based on what we have learned
• simplify the functions so that there is commonality between them
• group together similar tests

And afterwards, we will find that several functions belong together

Separate event assignment from event handler

Benefits were found in updating and separating the event assignment from the event handler function. We should do that to all of the event code that we come across.

Here is the event assignment and handler for the validate reset code:

    $(".btn2").click(function (evt) {
        //...
    });

We will extract out the registrationResetHandler function.

// $(".btn2").click(function (evt) {
//     ...
// });
function registrationResetHandler(evt) {
    //...
}
$(".btn2").click(registrationResetHandler);

We can now easily use registrationResetHandler from the tests, by accessing the event handler without slowing things down.

And also, a possible beneficial future is for all of event handler statements to be grouped together in the same place, making it easier to have a large-scale understanding of what the code does.

The .btn2 name doesn’t tell us much at all. That can be improved to help inform us about what the click event is being assigned to.

With forms, it’s best if an id attribute is used to refer to the form, so we’ll give the form an id of registration

    <form id="registration" ...>

We can now easily refer to the reset button of that form:

// $(".btn2").click(registrationResetHandler);
$("#registration [type=reset]").click(registrationResetHandler);

Even though the click event is a shortcut to assigning a click event, benefits are gained by using jQuery’s on method instead.

// $("#registration [type=reset]").click(registrationResetHandler);
$("#registration [type=reset]").on("click", registrationResetHandler);

The click event is somewhat limited for form events such as reset or submit.

For example, pressing Enter on a form normally triggers the submit event. If you assign a click event to the submit button, the Enter key somewhere else in the form still attempts to submit submit the form, bypassing any planned click handler that you have on the submit button.

The best way to deal with that is to stop using the click event on the submit or reset button, and instead bind the event handler to the submit or reset event of the whole form itself.

// $("#registration [type=reset]").on("click", registrationResetHandler);
$("#registration").on("reset", registrationResetHandler);

We can do the same thing to the change-password code to get similar improvements there too:

function passwordResetHandler() {
    ...
}
$("#changepw").on("reset", passwordResetHandler);

Exposing the event handler for testing

The only trouble we have now is that the registrationResetHandler function is not available to us right now, because it’s hidden inside of the document ready structure of the code. Fortunately we don’t need that document ready part because we already have the scripts loading from the end of the <body> tag.

But removing document ready would then let everything inside of it pollute the global namespace. To prevent that, we can replace document.ready with a module structure instead, which is as simple as replacing document.ready with an IIFE (immediately invoked function expression).

// document.ready(function () {
const validate = (function () {
    //...
// });
}());

We now have a single variable called validate, inside of which is all of the validate code.

We can now return an object from validate, that contains information we don’t mind passing to the outside world. In this case that is the information about the event handlers.

const validate = (function () {
    //...
    return {
        eventHandler: {
            registrationReset: registrationResetHandler
        }
    };
}());

That puts the validate code in a module structure, and gives us the benefit of making the event handler easily accessible to the tests.

We can easily do that too with the change-password code, and the login code too.

The change-password code is not inside of a document ready structure, so we can easily add a changePassword module structure around it, and expose only the handler code to the outside world.

const changePassword = (function () {
    //...
    return {
        eventHandler: {
            passwordReset: passwordResetHandler
        }
    };
}());

And, the login code is not in a document ready structure either, so we can easily contain that in a module structure, and expose only the event handler.

const login = (function () {
    //...
    return {
        eventHandler: {
            loginSubmit: loginSubmitHandler
        }
    };
}());

Speed up the tests

Currently the tests take quite a long time to run, being 130ms to run the validation reset tests. That will improve when we update the tests to only use the registrationResetHandler function instead.

Here is an example of the type of change being made to the tests. We remove the click trigger, and instead we use the handler function. To avoid the preventDefault code in the function from causing trouble, we give it a fake event object to use instead.

    const registrationResetHandler = validate.eventHandler.registrationReset;
    const fakeEvt = {
        preventDefault: function fakeFunc() {}
    };
//...
    it("updates the error name", function () {
        $error.html("");
        // $(".btn2").trigger("click");
        registrationResetHandler(fakeEvt);
        expect($error.html()).to.equal(field.name);
    });

After making that kind of update to all of the validate tests, they now run at a much faster 60ms speed, which is more than twice the speed as before. That’s nice. The faster that tests run, the more likely that we are to use them.

Clean up the code

This is a good time to clean up several formatting problems that I’ve noticed in the code.

Condense code

I seen many lines were thought of as being too complex, resulting in several lines of dot methods being there instead. For example:

    $(this)
        .next()
        .find(".feedback")
        .removeClass("glyphicon glyphicon-ok")
        .addClass("glyphicon glyphicon-remove")
        .removeClass("ok")
        .addClass("warning");

Dropping them down to separate lines is not a good way of dealing with that complexity.

I’ve used a find/replace regular expression of \n\s+\. to find all of those dropped lines, and have pulled them back up to the one line of code.

    $(this).next().find(".feedback").removeClass("glyphicon glyphicon-ok").addClass("glyphicon glyphicon-remove").removeClass("ok").addClass("warning");

That way the complexity is easier to see, and more code can be seen on the screen helping us to understand the structure and intention of the code.

When we come across these more complex lines, appropriate steps and solutions can be applied at the time to deal with that complexity in better ways.

Remove dead code

I’ve also removed commented-out code that has been littering all over the place. Dead code like that only confuses, and hardly ever helps.

Improve indentation

While updating the code I’ve noticed some really bad indentation of the code, making it difficult to understand how things are structured without close examination. We can use beautifier.io to help clean up many of those presentation problems in the code.

We are then left with code that is easier to understand and make improvements to. It doesn’t seem to have affected jsInspect either, for it still reports that there are 21 duplication issues. We will get to those later on.

Improve test variables

While working with the validate tests, I saw that the test variables are a bit too broad in scope. We should make sure that it’s clear that they only refer to the first name area. That way when we come back to look at these tests, we won’t be under any false assumptions about the other other fields being untested for now.

    // const formGroup = $(".form-group")[0];
    const $firstnameGroup = $(".form-group").first();
    // const field = $(firstnameGroup).find(".check")[0];
    const firstnameInput = $firstnameGroup.find(".check");
    // const $error = $(firstnameGroup).find(".error");
    const $firstnameError = $firstnameGroup.find(".error");
    // const $feedback = $(firstnameGroup).find(".feedback");
    const $firstnameFeedback = $firstnameGroup.find(".feedback");
    // const $starrq = $(firstnameGroup).find(".starrq");
    const $firstnameRequired = $firstnameGroup.find(".starrq");

Group tests together

The tests make more sense when they are grouped together by theme, instead of being in a single larger group. Here, I’ve grouped the tests by error, feedback, and required.

    const $firstnameGroup = $(".form-group").first();
    const registrationResetHandler = validate.eventHandler.registrationReset;
    const fakeEvt = {
        preventDefault: function fakeFunc() {}
    };
    describe("firstname error", function () {
        const $firstnameInput = $firstnameGroup.find(".check");
        const $firstnameError = $firstnameGroup.find(".error");
        it("updates the error name", function () {
            //...
        });
        it("removes warning from error", function () {
            //...
        });
        it("adds ok to error", function () {
            //...
        });
    });
    describe("firstname feedback", function () {
        const $firstnameFeedback = $firstnameGroup.find(".feedback");
        it("removes glyphicon from feedback", function () {
            //...
        });
        it("removes glyphicon-ok from feedback", function () {
            //...
        });
        it("removes warning from feedback", function () {
            //...
        });
    });
    describe("firstname required", function () {
        const $firstnameRequired = $firstnameGroup.find(".starrq");
        it("removes warning from starrq", function () {
            //...
        });
        it("adds ok to starrq", function () {
            //...
        });
    });

Summary

We have improved the global namespace by putting the code into a module structure, used that structure to help us make the event handlers easier and faster to test, and cleaned up the tests by grouping similar things together.

You can download the code as it is currently from the v0.0.5 release

Next steps

There are several similarities between the code we have updated, as it all controls with the error and feedback parts showing the status of different inputs.

Most of the functions we now have in separate functions, all relate to the input status. When they are grouped together it’s easier for us to ensure that everything behaves in a consistent and reliable manner.

Next time we clean up the tests, bring together several similar functions, and organise them all into a single inputStatus module.

[Paul_Wilkins]

Today we do the following:

• break up tests into meaningful chunks
• extract simple functions that can be used in multiple places
• and move functions into a separate inputStatus module

Updating the tests

In the last post we updating the code. Now it’s time for the tests to be revisited and improved.

Use describe blocks to break up the tests into meaningful chunks

Breaking up the tests into meaningful chunks makes it easier to understand what they are doing.

login.test.js

describe("When change-password form is reset, update input messages", function () {
    //...
    describe("email error", function () {
        //...
        it("Resets the error message", function () {
            //...
        });
        it("Resets the error color", function () {
            //...
        });
        it("Removes the error warning class", function () {
            //...
        });
        it("Adds the error ok class", function () {
            //...
        });
    });
    describe("email feedback", function () {
            //...
        it("Removes the feedback glyphicon class", function () {
            //...
        });
        it("Removes the feedback glyphicon-ok class", function () {
            //...
        });
        it("Removes the feedback ok class", function () {
            //...
        });
        it("Removes the feedback glyphicon-remove class", function () {
            //...
        });
    });
});

Use a single form-group element as the container in which to find other needed parts

Having a single point of entry helps to make it easier to understand where the information is coming from.

    const $termsFormgroup = $("#terms").closest(".form-group");
    //...
    const $termsFeedback = $termsFormgroup.find(".feedback");
    //...
    const $termcheck = $termsFormgroup.find("#termcheck");
    //...
    const $termsRequired = $termsFormgroup.find("#termsRequired");

With those last couple, I could have just done $("#termcheck) and gained access just as easily, but that doesn’t tell me anything about where it is on the page. With this updated way I now know that all of them are somewhere inside of the terms form-group.

We can now move on to bringing functions together into an inputStatus module.

inputStatus module

There are three sections that are being updated in most of the code we’ve already looked at here. They are:

• error
• feedback
• required

Starting with error

The error section has its text updated, and is set to either warning or ok with a suitable red or green colour.

The login code uses both types, so that’s a good place to put together separate functions for them.

    function errorOk(inputGroup, message) {
        const $error = $(inputGroup).find(".error");
        $error.html(message);
        $error.css("color", "green");
        $error.removeClass("warning");
        $error.addClass("ok");
    }
    function removeError(inputGroup, message) {
        // $(inputGroup).find(".error").html(message);
        // $(inputGroup).find(".error").css("color", "green");
        // $(inputGroup).find(".error").removeClass("warning").addClass("ok");
        errorOk(inputGroup, message);
        $(inputGroup).find(".feedback").removeClass("glyphicon glyphicon-remove").addClass("glyphicon glyphicon-ok ok");
    }

We can also have an errorWarning function that we use in the addError function.

    function errorWarning(inputGroup, message) {
        const $error = $(inputGroup).find(".error");
        $error.html(message);
        $error.css("color", "red");
        $error.removeClass("ok");
        $error.addClass("warning");
    }
    function addError(inputGroup, message) {
        // $(inputGroup).find(".error").html(message);
        // $(inputGroup).find(".error").css("color", "red");
        // $(inputGroup).find(".error").removeClass("ok").addClass("warning");
        errorWarning(inputGroup, message);
        $(inputGroup).find(".feedback").removeClass("glyphicon glyphicon-ok ok").addClass("glyphicon glyphicon-remove warning");
    }

Create inputStatus module

The errorOk and errorWarning functions can now be moved out to a separate set of code, that I’ll call inputStatus. I intend that the inputStatus module of code is responsible for handling everything that gets done in regard to the input status, which includes error, feedback, and required sections.

input-status.js

const inputStatus = (function () {
    function errorOk(inputGroup, message) {
        const $error = $(inputGroup).find(".error");
        $error.html(message);
        $error.css("color", "green");
        $error.removeClass("warning");
        $error.addClass("ok");
    }
    function errorWarning(inputGroup, message) {
        const $error = $(inputGroup).find(".error");
        $error.html(message);
        $error.css("color", "red");
        $error.removeClass("ok");
        $error.addClass("warning");
    }
    return {
        errorOk,
        errorWarning
    };
}());

We can add that input-status.js file to the index.html file:

<script src="./setup.js"></script>
<script src="./input-status.js"></script>
<script src="./validate.js"></script>

and we can now use it from the other code.

Using inputStatus in login.js code

The errorOk and errorWarning references in the login code can now be replaced with ones to inputStatus instead.

    function removeError(inputGroup, message) {
        // errorOk(inputGroup, message);
        inputStatus.errorOk(inputGroup, message);
        $(inputGroup).find(".feedback").removeClass("glyphicon glyphicon-remove").addClass("glyphicon glyphicon-ok ok");
    }
    function addError(inputGroup, message) {
        // errorWarning(inputGroup, message);
        inputStatus.errorWarning(inputGroup, message);
        $(inputGroup).find(".feedback").removeClass("glyphicon glyphicon-ok ok").addClass("glyphicon glyphicon-remove warning");
    }

which lets us remove the errorOk and errorWarning functions from the login.js file.

More importantly, we can use inputStatus.errorOk and inputStatus.errorWarning from other scripting files too.

Using inputStatus in the change-password code

The change-password code is easy to update, so that the same inputStatus.errorOk function is used

    function passwordResetHandler() {
        $(".inputboxmodal2").each(function resetInputMessages() {
            var inputName = $(this).find(".input-check").attr("name");
//            $(this).find(".error").html("Your " + inputName);
//            $(this).find(".error").css("color", "green");
//            $(this).find(".error").removeClass("warning").addClass("ok");
            inputStatus.errorOk(this, "Your " + inputName);
            $(this).find(".feedback").removeClass("glyphicon glyphicon-remove glyphicon-ok ok");
        });
    }

Using inputStatus in the validate.js code

The validate code is also easy to update so that inputStatus.errorOk is used.

    function resetMessages() {
        const $error = $(this).find(".error");
        const name = $(this).find(".check").attr("name");
//        $error.html(name);
//        resetWarning($error);
        inputStatus.errorOk(this, name);
        resetFeedback($(this).find(".feedback"));
        resetWarning($(this).find(".starrq"));
    }

Add feedback code to inputStatus

We can now also move the feedback code into the inputStatus code module.

    //...
    function feedbackOk(inputGroup, message) {
        const $feedback = $(inputGroup).find(".feedback");
        $feedback.removeClass("glyphicon glyphicon-remove");
        $feedback.addClass("glyphicon glyphicon-ok ok");
    }
    function feedbackWarning(inputGroup, message) {
        const $feedback = $(inputGroup).find(".feedback");
        $feedback.removeClass("glyphicon glyphicon-ok ok");
        $feedback.addClass("glyphicon glyphicon-remove");
    }
    return {
        errorOk,
        errorWarning,
        feedbackOk,
        feedbackWarning
    };
}());

Use feedbackOk and feedbackWarning in the login code

Here is the login code with using the updated inputStatus feedback code:

    function removeError(inputGroup, message) {
        inputStatus.errorOk(inputGroup, message);
        // $(inputGroup).find(".feedback").removeClass("glyphicon glyphicon-remove").addClass("glyphicon glyphicon-ok ok");
        inputStatus.feedbackOk(inputGroup);
    }
    function addError(inputGroup, message) {
        inputStatus.errorWarning(inputGroup, message);
        // $(inputGroup).find(".feedback").removeClass("glyphicon glyphicon-ok ok").addClass("glyphicon glyphicon-remove warning");
        inputStatus.feedbackWarning(inputGroup);
    }

Use feedbackNone in the change-password code

With the change-password and validate code though, we are removing all of the feedback entirely, so we could do with a separate feedbackNone function in the inputStatus code:

    function feedbackNone(inputGroup, message) {
        const $feedback = $(inputGroup).find(".feedback");
        $feedback.removeClass("glyphicon glyphicon-remove");
        $feedback.removeClass("glyphicon glyphicon-ok ok");
    }
    //...
    return {
        //...
        feedbackNone,
        //...
    };

which we can use in the change-password code:

        $("#changepw .form-group").each(function resetInputMessages() {
            var inputName = $(this).find(".input-check").attr("name");
            inputStatus.errorOk(this, "Your " + inputName);
            // $(this).find(".feedback").removeClass("glyphicon glyphicon-remove glyphicon-ok ok");
            inputStatus.feedbackNone(this);
        });

Use feedbackNone in the validate code

When we use feedbackNone in the validate code, there is a slight issue:

    function resetMessages() {
        const $error = $(this).find(".error");
        const name = $(this).find(".check").attr("name");
        inputStatus.errorOk(this, name);
        // resetFeedback($(this).find(".feedback"));
        inputStatus.feedbackNone(this);
        resetWarning($(this).find(".starrq"));
    }

A test fails, saying that warning is expected to be removed, but it wasn’t.

One of the feedback examples of code that we used to create feedbackNone was incomplete. Here is an update, ensuring that the warning class is removed too.

    function feedbackNone(inputGroup, message) {
        const $feedback = $(inputGroup).find(".feedback");
        // $feedback.removeClass("glyphicon glyphicon-remove");
        $feedback.removeClass("glyphicon glyphicon-remove warning");
        $feedback.removeClass("glyphicon glyphicon-ok ok");
    }

(this is the wrong approach, which we soon learn and use a different technique with)

We also need to add the removal of warning to the other feedback functions too.

    function feedbackOk(inputGroup, message) {
        const $feedback = $(inputGroup).find(".feedback");
        // $feedback.removeClass("glyphicon glyphicon-remove");
        $feedback.removeClass("glyphicon glyphicon-remove warning");
        $feedback.addClass("glyphicon glyphicon-ok ok");
    }
    function feedbackWarning(inputGroup, message) {
        const $feedback = $(inputGroup).find(".feedback");
        $feedback.removeClass("glyphicon glyphicon-ok ok");
        $feedback.addClass("glyphicon glyphicon-remove warning");
    }

When it comes to the feedback functions in the inputStatus module, I can’t help but think that there’s a better way. We needed to make changes to three sets of code in three different places, and that’s concerning. There should be a better way to do that without needing quite so many changes.

Improving the feedback none/ok/warning functions

From the feedbackOk and feedbackWarning functions, instead of removing classes I can call feedbackNone instead to remove all of the relevant classes, and then add only what is needed.

    function feedbackNone(inputGroup, message) {
        const $feedback = $(inputGroup).find(".feedback");
        $feedback.removeClass("glyphicon glyphicon-remove warning");
        $feedback.removeClass("glyphicon glyphicon-ok ok");
    }
    function feedbackOk(inputGroup, message) {
        const $feedback = $(inputGroup).find(".feedback");
        feedbackNone(inputGroup);
        $feedback.addClass("glyphicon glyphicon-ok ok");
    }
    function feedbackWarning(inputGroup, message) {
        const $feedback = $(inputGroup).find(".feedback");
        feedbackNone(inputGroup);
        $feedback.addClass("glyphicon glyphicon-remove warning");
    }

The benefit of doing things that way is that there is exactly one add and one remove for each type of thing. Each addClass has a matching removeClass. That code is a lot better now.

Improve the terms code

The last set of code to improve is the terms and conditions in the validate code:

    function removeTermWarning() {
        const $termsGroup = $("#terms").closest(".form-group");
        resetFeedback($termsGroup.find(".feedback"));
        resetWarning($("#termcheck"));
        resetWarning($("#termsRequired"));
    }

Instead of using resetFeedback, we can use inputStatus.feedbackNone

    function removeTermWarning() {
        const $termsGroup = $("#terms").closest(".form-group");
        inputStatus.feedbackNone($termsGroup);
        resetWarning($("#termcheck"));
        resetWarning($("#termsRequired"));
    }

That lets us delete the old resetFeedback function.

The resetWarning function would be handy to have in the inputStatus function. The errorOk and errorWarning functions can certainly use them, as setOk and setWarning functions.

Add setOk and setWarning to the inputStatus module

We can add setOk to go with the errorOk function

    function setOk($el) {
        $el.removeClass("warning");
        $el.addClass("ok");
    }
    function errorOk(inputGroup, message) {
        const $error = $(inputGroup).find(".error");
        $error.html(message);
        $error.css("color", "green");
        // $error.removeClass("warning");
        // $error.addClass("ok");
        setOk($error);
    }

And, we can do similar to add setWarning to go with the errorOk function:

    function errorWarning(inputGroup, message) {
        const $error = $(inputGroup).find(".error");
        $error.html(message);
        $error.css("color", "red");
        // $error.removeClass("ok");
        // $error.addClass("warning");
        setWarning($error);
    }

We can also use those setOk and setWarning functions with the feedback code, but we’ll also need a setNone function to keep feedbackNone happy.

    function setNone($el) {
        $el.removeClass("warning");
        $el.removeClass("ok");
    }
    //...
    function feedbackNone(inputGroup, message) {
        const $feedback = $(inputGroup).find(".feedback");
        $feedback.removeClass("glyphicon glyphicon-remove");
        $feedback.removeClass("glyphicon glyphicon-ok");
        setNone($feedback);
    }
    function feedbackOk(inputGroup, message) {
        const $feedback = $(inputGroup).find(".feedback");
        feedbackNone(inputGroup);
        $feedback.addClass("glyphicon glyphicon-ok");
        setOk($feedback);
    }
    function feedbackWarning(inputGroup, message) {
        const $feedback = $(inputGroup).find(".feedback");
        feedbackNone(inputGroup);
        $feedback.addClass("glyphicon glyphicon-remove");
        setWarning($feedback);
    }

We can now update the term code to use setOk as well.

    function removeTermWarning() {
        const $termsGroup = $("#terms").closest(".form-group");
        inputStatus.feedbackNone($termsGroup);
        // resetWarning($termsGroup.find("#termcheck"));
        inputStatus.setOk($termsGroup.find("#termcheck"));
        // resetWarning($termsGroup.find("#termsRequired"));
        inputStatus.setOk($termsGroup.find("#termsRequired"));
    }

Add .starrq for required to the inputStatus

With the aim to delete the resetWarning function, there’s only one thing more left to update, and that’s the starrq required part of the code.

In the validate code we can tell it to use the

    function resetMessages() {
        const $error = $(this).find(".error");
        const name = $(this).find(".check").attr("name");
        inputStatus.errorOk(this, name);
        inputStatus.feedbackNone(this);
        // resetWarning($(this).find(".starrq"));
        inputStatus.setWarning($(this).find(".starrq"));
    }

and we can now finally remove the removeWarning and replaceClass functions from the validate code.

Summary

The bulk of the code is now being done by the inputStatus function. Here’s what the inputStatus.js code looks like:

const inputStatus = (function () {
    function setNone($el) {
        $el.removeClass("warning");
        $el.removeClass("ok");
    }
    function setOk($el) {
        $el.removeClass("warning");
        $el.addClass("ok");
    }
    function setWarning($el) {
        $el.removeClass("ok");
        $el.addClass("warning");
    }
    function errorOk(inputGroup, message) {
        const $error = $(inputGroup).find(".error");
        $error.html(message);
        $error.css("color", "green");
        setOk($error);
    }
    function errorWarning(inputGroup, message) {
        const $error = $(inputGroup).find(".error");
        $error.html(message);
        $error.css("color", "red");
        setWarning($error);
    }
    function feedbackNone(inputGroup, message) {
        const $feedback = $(inputGroup).find(".feedback");
        $feedback.removeClass("glyphicon glyphicon-remove");
        $feedback.removeClass("glyphicon glyphicon-ok");
        setNone($feedback);
    }
    function feedbackOk(inputGroup, message) {
        const $feedback = $(inputGroup).find(".feedback");
        feedbackNone(inputGroup);
        $feedback.addClass("glyphicon glyphicon-ok");
        setOk($feedback);
    }
    function feedbackWarning(inputGroup, message) {
        const $feedback = $(inputGroup).find(".feedback");
        feedbackNone(inputGroup);
        $feedback.addClass("glyphicon glyphicon-remove");
        setWarning($feedback);
    }
    return {
        setNone,
        setOk,
        setWarning,
        errorOk,
        errorWarning,
        feedbackNone,
        feedbackOk,
        feedbackWarning
    };
}());

That causes even the most complex code to be rather simple. Here for example is the validate reset code:

    function resetMessages() {
        const $error = $(this).find(".error");
        const name = $(this).find(".check").attr("name");
        inputStatus.errorOk(this, name);
        inputStatus.feedbackNone(this);
        inputStatus.setOk($(this).find(".starrq"));
    }

    function removeTermWarning() {
        const $termsGroup = $("#terms").closest(".form-group");
        inputStatus.feedbackNone($termsGroup);
        inputStatus.setOk($termsGroup.find("#termcheck"));
        inputStatus.setOk($termsGroup.find("#termsRequired"));
    }

    function registrationResetHandler(evt) {
        $(".form-group").each(resetMessages);
        removeTermWarning();
    }
    $("#registration").on("reset", registrationResetHandler);

We should be able to make good use of the inputStatus functions as we work through the rest of the code.

The latest code is found at v0.0.6 in releases

As a reminder, we have only randomly looked at three different sections of code so far, drawn to them by jsInspect looking for cases of duplication. Those three sections being:

• validate reset
• login submit
• change password reset

There are a whole lot of other sections to the code, but ideally we will be able to group together similar behaviour into external libraries like inputStatus.

[Paul_Wilkins]

Today we will be doing the following:

• use jsInspect to find the next case of duplication
• use tests to accurately keep track of everything the code touches
• extract a useful function from the the terms click code

The next duplication

The next set of duplication that jsInspect tells us about is in the validate.js file at line 787

Match - 2 instances

./registration3\validate.js:787,791
if ($(this).is(":checked")) {
    console.log("Checkbox is checked.");
    $("#termcheck").addClass('ok').removeClass('warning');
    $("#termsRequired").addClass('ok').removeClass('warning');
} else if ($(this).is(":not(:checked)")) {

./registration3\validate.js:817,821
if ($("#terms").is(":checked")) {
    console.log("Checkbox is checked.");
    $("#termcheck").addClass('ok').removeClass('warning');
    $("#termsRequired").addClass('ok').removeClass('warning');
} else if ($("#terms").is(":not(:checked)")) {

Cleanup - remove console.log statements

I’ms seeing a lot of console.log statements, all of which aren’t needed anymore, so I’m removing all of the console.log statements from all of our code.

The duplicated code is in two different sections of the validate.js file.

One part is about the terms,

    $('#terms').on("click", function() {
        //...
    });

and the other part involved a button click.

    $(".btn1").click(function() {
        //...
    });

Quite which button we don’t know yet. The HTML code indicates that it’s the registration submit. But I won’t mess around with the code right now until we have tests in place to ensure that everything still continues to work in the same way that it does now.

Tests for clicking the terms

As we are adding test for terms when we click submit, I’m naming the other terms tests in there more appropriately so that we have a better way to know that those others are for when the terms are reset.

    describe("terms reset", function () {
        //...
    });

Another set of terms tests are now added above those, for when the terms checkbox is clicked.

    describe("terms submit", function () {
        const $termsGroup = $("#terms").closest(".form-group");
        const $terms = $termsGroup.find("#terms");
        const $termsError = $termsGroup.find("#termcheck");
        const $termsRequired = $termsGroup.find("#termsRequired");
        describe("terms are checked", function () {
            beforeEach(function () {
                $("#terms").prop("checked", false);
            });
            it("adds ok to the checkbox", function () {
                $termsError.removeClass("ok");
                $terms.trigger("click");
                expect($termsError.attr("class")).to.contain("ok");
            });
            it("removes warning from the checkbox", function () {
                $termsError.addClass("warning");
                $terms.trigger("click");
                expect($termsError.attr("class")).to.not.contain("warning");
            });
            it("add ok to the required star", function () {
                $termsRequired.removeClass("ok");
                $terms.trigger("click");
                expect($termsRequired.attr("class")).to.contain("ok");
            });
            it("removes warning from the required star", function () {
                $termsRequired.addClass("warning");
                $terms.trigger("click");
                expect($termsRequired.attr("class")).to.not.contain("warning");
            });
        });
        describe("terms are unchecked", function () {
            beforeEach(function () {
                $("#terms").prop("checked", true);
            });
            it("removes ok from the checkbox", function () {
                $termsError.addClass("ok");
                $terms.trigger("click");
                expect($termsError.attr("class")).to.not.contain("ok");
            });
            it("adds warning to the checkbox", function () {
                $termsError.removeClass("warning");
                $terms.trigger("click");
                expect($termsError.attr("class")).to.contain("warning");
            });
            it("removes ok from the required star", function () {
                $termsRequired.addClass("ok");
                $terms.trigger("click");
                expect($termsRequired.attr("class")).to.not.contain("ok");
            });
            it("adds warning to the required star", function () {
                $termsRequired.removeClass("warning");
                $terms.trigger("click");
                expect($termsRequired.attr("class")).to.contain("warning");
            });
        });
    });

Make the code easier to test

I don’t want the trigger click part to be there in the tests as that’s an unwanted complication. Now that I have the tests in place, I can update the terms click code so that it has a better structure, and appropriately exposes the terms click handler.

    function termsClickHandler() {
        //...
    }
    // $('#terms').click(function() {
    // });
    $('#terms').on("click", termsClickHandler);
//...
    return {
        eventHandler: {
            registrationReset: registrationResetHandler,
            termsClick: termsClickHandler
        }
    };

The tests can now eventHandler.termsClick to test the event handler. Because the termsClickHandler uses the this keyword, we need to call the handler using #terms as the context. And because we aren’t actually clicking anything, instead of setting the checkbox as it would be before we click it, we instead need to set the checkbox to the desired state that we want to test.

        const termsClickHandler = validate.eventHandler.termsClick;
        describe("terms are checked", function () {
            beforeEach(function () {
                // $("#terms").prop("checked", false);
                $("#terms").prop("checked", true);
            });
            it("adds ok to the checkbox", function () {
                $termsError.removeClass("ok");
                // $terms.trigger("click");
                termsClickHandler.call($terms);
                expect($termsError.attr("class")).to.contain("ok");
            });
            it("removes warning from the checkbox", function () {
                $termsError.addClass("warning");
                // $terms.trigger("click");
                termsClickHandler.call($terms);
                expect($termsError.attr("class")).to.not.contain("warning");
            });

The rest of the tests are updated in the same way.

Improving the code

Now that we have a fast set of tests that cover the code, we can look at making improvements to the code and use the tests as reassurance that everything still behaves the same as it did before.

Here is the code that we are looking at updating.

    function termsClickHandler() {
        if ($(this).is(":checked")) {
            $("#termcheck").addClass('ok').removeClass('warning');
            $("#termsRequired").addClass('ok').removeClass('warning');
        } else if ($(this).is(":not(:checked)")) {
            $("#termcheck").removeClass('ok').addClass('warning');
            $("#termsRequired").removeClass('ok').addClass('warning');
        }
    }

Remove the else clause

A checkbox has only two possible states. Testing for the other state results in confusion, because you’re trying to figure out what else there could be. Instead of that confusion, just leave it as an else clause.

    function termsClickHandler() {
        if ($(this).is(":checked")) {
            $("#termcheck").addClass('ok').removeClass('warning');
            $("#termsRequired").addClass('ok').removeClass('warning');
        // } else if ($(this).is(":not(:checked)")) {
        } else {
            $("#termcheck").removeClass('ok').addClass('warning');
            $("#termsRequired").removeClass('ok').addClass('warning');
        }
    }

Use existing setOk and setWarning code

Because of the code we already have in the inputStatus module, we can reuse that here too.

    function termsClickHandler() {
        if ($(this).is(":checked")) {
            // $("#termcheck").addClass('ok').removeClass('warning');
            inputStatus.setOk($("#termcheck"));
            // $("#termsRequired").addClass('ok').removeClass('warning');
            inputStatus.setOk($("#termsRequired"));
        } else {
            // $("#termcheck").removeClass('ok').addClass('warning');
            inputStatus.setWarning($("#termcheck"));
            // $("#termsRequired").removeClass('ok').addClass('warning');
            inputStatus.setWarning($("#termsRequired"));
        }
    }

And as I can tell that we’re going to need the same code in the other section further below, now is a good time to move that code out to a separate function.

    function updateTerms(terms) {
        if ($(terms).is(":checked")) {
            inputStatus.setOk($("#termcheck"));
            inputStatus.setOk($("#termsRequired"));
        } else {
            inputStatus.setWarning($("#termcheck"));
            inputStatus.setWarning($("#termsRequired"));
        }
    }
    function termsClickHandler() {
        updateTerms(this);
    }

Should we move this code into the inputStatus module? I don’t think that we should do that, as the inputStatus module shouldn’t need to know specifics of the different types of inputs that we have there. Those details we can manage outside of the inputStatus code.

The updateTermsStatus function can be used whenever we want to update the status of the terms. And who knows, when we’ve collected a fair sampling of other functions we might even consider moving them in to a separate validateStatus module.

For now though, the terms click code is all updated.

Update the readme

On taking a look at the readme.md file, I notice that ignoring the lib folder isn’t mentioned, or the use of the inspect.bat file.
The best time to take care of a small problem is immediately, so I’ve made a small update to it.

Summary

Test the code and improve the testability of the code. Improve the tests then update the code. That is the general process that we are using.

The code for how things are at this stage can be obtained from v0.0.7 from releases

In the next post we’ll update the other part of the duplication that was reported to us in the registration submit code, and do some more tidying up of the rest of the code in that section.

[Paul_Wilkins]

The second part of this duplication that was found is in the registration submit code.

Today we do the following:

• get an initial test in place
• make minor adjustment so the code is testable
• use chai-spies to spy on evt.preventDefault
• add tests for when field is empty
• add tests for when terms is check and unchecked
• divide up the tests into separate files

Initial tests

The code that we are going to test starts with the following:

    $(".btn1").click(function() {
        $('.form-group').each(function() {
            var $requiredField = $(this).find(".check");
            if ($requiredField.length === 0) {
                return;
            }
            if ($(".inputstatus .warning").length != 0) {
                event.preventDefault();
            }

Use evt instead of event

An initial test helps us learn if our test can access the code:

    describe("registration submit", function () {
        const submitButton = $(".btn1");
        it("", function () {
            submitButton.trigger("click");
        });
    });

The browser console reports the error: TypeError: Cannot read property 'preventDefault' of undefined

Here we apply the standard technique of getting evt from the event handler

    // $(".btn1").click(function() {
    $(".btn1").click(function(evt) {
        $('.form-group').each(function() {
            var $requiredField = $(this).find(".check");
            if ($requiredField.length === 0) {
                return;
            }
            if ($(".inputstatus .warning").length != 0) {
                // event.preventDefault();
                evt.preventDefault();
            }
            if (value === "") {
                //...
                // event.preventDefault();
                evt.preventDefault();
            }
            } else if ($("#terms").is(":not(:checked)")) {
                // event.preventDefault();
                evt.preventDefault();
                //...
            }
        });
    });

Test the required field length

How do we test the required field length? When we comment out the return statement, the test in the browser console gives us a type error:

            var $requiredField = $(this).find(".check");
            if ($requiredField.length === 0) {
                // return;
            }

TypeError: Cannot read property 'trim' of undefined

A basic test in that regard is that no error occurs:

        const submitButton = $(".btn1");
        it("doesn't throw an error", function () {
            expect(function () {
                submitButton.trigger("click");
            }).to.not.throw();
        });

We can get more specific with the error, for example including text from the error itself:

        const submitButton = $(".btn1");
        it("doesn't throw an error", function () {
            expect(function () {
                submitButton.trigger("click");
            }).to.not.throw("Cannot read property 'trim' of undefined");
        });

Can we avoid that trim error in some other way? I think that we can, but problem happen when testing this submit event handler such as endlessly looping page submits, so we need to do some other things first before messing around with the code. Those other things are:

• test the event handler separately
• test what the code does
• restructure the form event

and then we can come back to improving the code.

Test the event handler separately

As this is a submit event that we’re dealing with, we need to separate the event assignment from the event handler before much more useful testing can occur.

We give the event handler a suitable name:

    $(".btn1").click(function registrationSubmitHandler(evt) {
        //...
    });

And then we extract out the function:

    function registrationSubmitHandler(evt) {
        //...
    }
    $(".btn1").click(registrationClickHandler);

and provide easy access to it at the end of the code:

    //...
    return {
        eventHandler: {
            registrationSubmit: registrationSubmitHandler,
            //...
        }
    };

We can now update the test so that it uses registrationSubmitHandler instead of triggering the submit event.

Note: My first attempt at this failed due to an oversight. A good way to ensure that things go well is to have a failing test occur, so that you can ensure that it continues to properly fail as you update the test.

We can comment out the return statement as we did earlier, to give us a reliable test error helping us to confirm that testing remains consistent while changes are being made.

            var $requiredField = $(this).find(".check");
            if ($requiredField.length === 0) {
                // return;
            }

We can now update the test so that we are using that registrationSubmitHandler instead:

    describe("registration submit", function () {
        // const submitButton = $(".btn1");
        const registrationSubmitHandler = validate.eventHandler.registrationSubmit;
        let fakeEvt;
        beforeEach(function () {
            fakeEvt = {
                preventDefault: function () {}
            };
        });
        it("doesn't throw an error", function () {
            expect(function () {
                // submitButton.trigger("click");
                registrationSubmitHandler(fakeEvt);
            }).to.not.throw("Cannot read property 'trim' of undefined");
        });
    });

Now that the updated test is known to be working, we can restore the return statement back to normal.

            var $requiredField = $(this).find(".check");
            if ($requiredField.length === 0) {
                return;
            }

Test the preventDefault on warning

The next part that we should test is the preventDefault when a warning exists.’

            if ($(".inputstatus .warning").length != 0) {
                evt.preventDefault();
            }

We can use a spy, to figure out if that preventDefault method was called. There exist existing library code that provides chai spies.

I went ahead and used my own custom spy, but we might as well use the proper code for using spies.

Install chai-spies from the command prompt with:

> npm install save-dev chai-spies

and add it to index.html where there is the chai script:

<script src="../node_modules/chai/chai.js"></script>
<script src="../node_modules/chai-spies/chai-spies.js"></script>

We can now use it in tests to easily spy on something. In this case it is the fakeEvt.preventDefault method

We can ensure that preventDefault isn’t called by adding a value to all checked form fields, remove all warning notices, and ensure that the terms checkbox is checked.

        it("doesn't call preventDefault when fields have no error", function () {
            chai.spy.on(fakeEvt, "preventDefault");
            $(".form-group .check").val("test value");
            $(".inputstatus .warning").removeClass("warning");
            $("#terms").prop("checked", true);
            registrationSubmitHandler(fakeEvt);
            expect(fakeEvt.preventDefault).to.not.have.been.called();
        });

To ensure that the $(".inputstatus .warning").length != 0 condition is responsible for preventDefault, we can add a warning to the above test, and check if preventDefault was called.

        it("calls preventDefault when any field has an error", function () {
            chai.spy.on(fakeEvt, "preventDefault");
            $(".form-group .check").val("test value");
            $(".inputstatus .warning").removeClass("warning");
            $(".inputstatus .error").eq(0).addClass("warning");
            $("#terms").prop("checked", true);
            registrationSubmitHandler(fakeEvt);
            expect(fakeEvt.preventDefault).to.have.been.called();
        });

Those tests can all be put into an errors section.

    describe("avoiding errors", function () {
        it("doesn't throw an error", function () {
            //...
        });
        it("doesn't call preventDefault when no fields have a warning", function () {
            //...
        });
        it("calls preventDefault when a field has a warning", function () {
            //...
        });
    });

We can now move on to testing the two if statements in the submit event handler.

Testing when empty value

When an input field has an empty value, that causes several different things to occur. Here is the code that we are testing:

            if (value === "") {
                $(this).find(".error").html(name + " is empty !").removeClass("ok").addClass("warning");
                $(this).find(".feedback").removeClass("glyphicon glyphicon-ok").addClass("glyphicon glyphicon-remove").removeClass("ok").addClass("warning");
                $(this).find(".starrq").removeClass("ok").addClass("warning");
                $("#termcheck").removeClass('ok').addClass('warning');
                $("#termsRequired").removeClass('ok').addClass('warning');
                evt.preventDefault();
            }

And here are the tests for that code.

    describe("firstname is empty", function () {
        const $firstnameGroup = $("#registration .form-group").first();
        const $firstnameInput = $firstnameGroup.find("input");
        const firstnameName = $firstnameGroup.find("input").attr("name");
        beforeEach(function () {
            $firstnameInput.val("");
        });
        describe("error", function () {
            const $firstnameError = $firstnameGroup.find(".error");
            it("shows the error text", function () {
                $firstnameError.html("");
                registrationSubmitHandler(fakeEvt);
                expect($firstnameError.html()).to.equal("First Name is empty !");
            });
            it("removes ok", function () {
                $firstnameError.addClass("ok");
                registrationSubmitHandler(fakeEvt);
                expect($firstnameError.attr("class")).to.not.contain("ok");
            });
            it("adds warning", function () {
                $firstnameError.removeClass("warning");
                registrationSubmitHandler(fakeEvt);
                expect($firstnameError.attr("class")).to.contain("warning");
            });
        });
        describe("feedback", function () {
            const $firstnameFeedback = $firstnameGroup.find(".feedback");
            it("adds glyphicon", function () {
                $firstnameFeedback.removeClass("glyphicon");
                registrationSubmitHandler(fakeEvt);
                expect($firstnameFeedback.attr("class")).to.contain("glyphicon");
            });
            it("removes glyphicon-ok", function () {
                $firstnameFeedback.addClass("glyphicon-ok");
                registrationSubmitHandler(fakeEvt);
                expect($firstnameFeedback.attr("class")).to.not.contain("glyphicon-ok");
            });
            it("adds glyphicon-remove", function () {
                $firstnameFeedback.removeClass("glyphicon-remove");
                registrationSubmitHandler(fakeEvt);
                expect($firstnameFeedback.attr("class")).to.contain("glyphicon-remove");
            });
            it("removes ok", function () {
                $firstnameFeedback.addClass("ok");
                registrationSubmitHandler(fakeEvt);
                expect($firstnameFeedback.attr("class")).to.not.contain("ok");
            });
            it("adds warning", function () {
                $firstnameFeedback.removeClass("warning");
                registrationSubmitHandler(fakeEvt);
                expect($firstnameFeedback.attr("class")).to.contain("warning");
            });
        });
        describe("required star", function () {
            const $firstnameRequired = $firstnameGroup.find(".starrq");
            it("removes ok", function () {
                $firstnameRequired.addClass("ok");
                registrationSubmitHandler(fakeEvt);
                expect($firstnameRequired.attr("class")).to.not.contain("ok");
            });
            it("adds warning", function () {
                $firstnameRequired.removeClass("warning");
                registrationSubmitHandler(fakeEvt);
                expect($firstnameRequired.attr("class")).to.contain("warning");
            });
        });
        describe("terms", function () {
            const $termsGroup = $("#terms").closest(".form-group");
            const $termsError = $termsGroup.find(".error2");
            const $termsRequired = $termsGroup.find(".starrq");
            beforeEach(function () {
                $("#terms").prop("checked", false);
            })
            it("removes ok from error", function () {
                $termsError.addClass("ok");
                registrationSubmitHandler(fakeEvt);
                expect($termsError.attr("class")).to.not.contain("ok");
            });
            it("adds warning to error", function () {
                $termsError.removeClass("warning");
                registrationSubmitHandler(fakeEvt);
                expect($termsError.attr("class")).to.contain("warning");
            });
            it("removes ok from required", function () {
                $termsRequired.addClass("ok");
                registrationSubmitHandler(fakeEvt);
                expect($termsRequired.attr("class")).to.not.contain("ok");
            });
            it("adds warning to required", function () {
                $termsRequired.removeClass("warning");
                registrationSubmitHandler(fakeEvt);
                expect($termsRequired.attr("class")).to.contain("warning");
            });
        });
        it("prevents form submission", function () {
            $firstnameGroup.find(".check").val("test value");
            $firstnameGroup.find("input").val("");
            $("#terms").prop("checked", true);
            chai.spy.on(fakeEvt, "preventDefault");
            registrationSubmitHandler(fakeEvt);
            expect(fakeEvt.preventDefault).to.have.been.called();
        });
    });

Testing terms state

The last part of the code that does things is in regard to the terms and conditions.

            if ($("#terms").is(":checked")) {
                $("#termcheck").addClass('ok').removeClass('warning');
                $("#termsRequired").addClass('ok').removeClass('warning');
            } else if ($("#terms").is(":not(:checked)")) {
                evt.preventDefault();
                $("#termcheck").removeClass('ok').addClass('warning');
                $("#termsRequired").removeClass('ok').addClass('warning');
            }

We created the same tests in the previous post so this lot will be easy to test.

    describe("terms and conditions", function () {
        const $termsGroup = $("#terms").closest(".form-group");
        const $termsError = $termsGroup.find(".error2");
        const $termsRequired = $termsGroup.find(".starrq");
        describe("terms are checked", function () {
            beforeEach(function () {
                $("#terms").prop("checked", true);
            })
            it("adds ok to error", function () {
                $termsError.removeClass("ok");
                registrationSubmitHandler(fakeEvt);
                expect($termsError.attr("class")).to.contain("ok");
            });
            it("removes warning from error", function () {
                $termsError.addClass("warning");
                registrationSubmitHandler(fakeEvt);
                expect($termsError.attr("class")).to.not.contain("warning");
            });
            it("adds ok to required", function () {
                $termsRequired.removeClass("ok");
                registrationSubmitHandler(fakeEvt);
                expect($termsRequired.attr("class")).to.contain("ok");
            });
            it("removes warning from required", function () {
                $termsRequired.addClass("warning");
                registrationSubmitHandler(fakeEvt);
                expect($termsRequired.attr("class")).to.not.contain("warning");
            });
        });
        describe("terms are unchecked", function () {
            beforeEach(function () {
                $("#terms").prop("checked", false);
            })
            it("removes ok from error", function () {
                $termsError.addClass("ok");
                registrationSubmitHandler(fakeEvt);
                expect($termsError.attr("class")).to.not.contain("ok");
            });
            it("adds warning to error", function () {
                $termsError.removeClass("warning");
                registrationSubmitHandler(fakeEvt);
                expect($termsError.attr("class")).to.contain("warning");
            });
            it("removes ok from required", function () {
                $termsRequired.addClass("ok");
                registrationSubmitHandler(fakeEvt);
                expect($termsRequired.attr("class")).to.not.contain("ok");
            });
            it("adds warning to required", function () {
                $termsRequired.removeClass("warning");
                registrationSubmitHandler(fakeEvt);
                expect($termsRequired.attr("class")).to.contain("warning");
            });
        });
    });

Divide large files into smaller files

The validate tests file have grown to be quite large now, so dividing them into separate test files helps to make it easier to manage them.

<script src="../tests/validate-terms-click.test.js"></script>
<script src="../tests/validate-registration-submit.test.js"></script>
<script src="../tests/validate-registration-reset.test.js"></script>

Summary

The validate-submit section of code that we’re working with is quite large and has resulted in a lot of tests. That’s fine, for the tests are ensuring that everything still continues to work in the same way as desired, when we come to updating the code.

We have used chai-spies to check that the preventDefault method is called, tested what happens when the firstname field is empty (all other fields should behave in the same way), and tested what the validate-submit code does when the terms checkbox is checked or not checked.

The code as things are right now is found in v0.0.8 from releases

Now that those tests are in place, we are in a good position to work on several improvements to the code. But that’s for next time.

[Paul_Wilkins]

Removing duplication from the registration submit code is what we’re up to today. We will be doing the following:

• test the event assignment, and improve it
• refine some preventDefault tests
• remove terms duplication
• simplify the submit handler function
• further simplify using inputStatus methods

Now that we have a set of tests covering what happens when validate submit occurs, we can start to make some improvements.

Improve the event assignment

With the event assignment, a click on btn1 isn’t all that informative. It’s much better when we know it’s the registration submit event instead.

Before doing that though, I don’t think that this part of the code has any tests relating to it.

    $(".btn1").click(registrationSubmitHandler);

When commenting-out that line, I find that no it doesn’t, as the tests still pass with it commented out, and that isn’t good.

I was about to use a prevent-default part of the test to also ensure that the click assignment works, but that is using a test to check multiple things. Tests are more reliable when only one things is checked, so a new test is needed.

We can easily test it by ensuring a form field is empty and that no warnings are on the page before the form submit code runs.

        it("runs the submit handler when submit is clicked", function () {
            $firstnameGroup.find("input").val("");
            const $error = $firstnameGroup.find(".error");
            $error.removeClass("warning");
            $("#registration").trigger("submit");
            expect($error.attr("class")).to.contain("warning");
        });

The $("#registration").trigger("submit"); causes the test page to endlessly reload, because we don’t have an event handler set up yet for the form submit.

Updating the form submit assignment fixes that problem:

    $("#registration").on("submit", registrationSubmitHandler);

and help to confirm for us that the test is properly checking the right thing.

Tidying up preventDefault tests

While making the above update, I noticed that some other tests about avoiding errors were causing trouble. They were too excessive in the changes they were making.

    describe("avoiding errors", function () {
        // it("doesn't throw an error", function () {
        it("doesn't throw a trim error", function () {
            expect(function () {
                registrationSubmitHandler(fakeEvt);
            }).to.not.throw("Cannot read property 'trim' of undefined");
        });
        it("doesn't call preventDefault when no fields have a warning", function () {
            chai.spy.on(fakeEvt, "preventDefault");
            // $(".form-group .check").val("test value");
            $(".form-group input").val("test value");
            $(".form-group textarea").val("test value");
            // $(".inputstatus .warning").removeClass("warning");
            $(".form-group .warning").removeClass("warning");
            $("#terms").prop("checked", true);
            registrationSubmitHandler(fakeEvt);
            expect(fakeEvt.preventDefault).to.not.have.been.called();
        });
        it("calls preventDefault when a field has a warning", function () {
            chai.spy.on(fakeEvt, "preventDefault");
            // $(".form-group .check").val("test value");
            $(".form-group .check").eq(0).val("");
            // $(".inputstatus .warning").removeClass("warning");
            // $(".inputstatus .error").eq(0).val("");
            $(".inputstatus .error").removeClass("warning");
            $("#terms").prop("checked", true);
            registrationSubmitHandler(fakeEvt);
            expect(fakeEvt.preventDefault).to.have.been.called();
        });
    });

With those refinements in place, they still test what they should, without interfering with anything.

Remove duplication from the terms

The registration submit code has a clear case of duplication in regard to the terms and conditions.

Earlier we put code into an updateTerms function, so we can replace existing code with a function call instead.

The tests that we put in place in the previous post will tell us if anything goes wrong.

            // if ($("#terms").is(":checked")) {
            //     $("#termcheck").addClass('ok').removeClass('warning');
            //     $("#termsRequired").addClass('ok').removeClass('warning');
            // } else if ($("#terms").is(":not(:checked)")) {
            //     evt.preventDefault();
            //     $("#termcheck").removeClass('ok').addClass('warning');
            //     $("#termsRequired").removeClass('ok').addClass('warning');
            // }
            updateTerms($("#terms"));

It’s still surprising when everything just works after making a big change like that. But, the tests show that everything still works.

An even better metric is to remove the call to that updateTerms function, and the tests for terms and conditions all jump up complaining that something is wrong. Put back the call to the updateTerms function and all is good.

Move preventDefault

Currently some of the preventDefault code is inside of the form-group each-loop, and doesn’t need to be there.

When using preventDefault, it’s best if it occurs at the start of the event handler, or failing that at the end of the event handler. That way we have an easier way to understand potentially weird behaviour.

We can move the if statement checking the warnings to the end of that section of code, below the loop instead, and get rid of all other preventDefault parts of the code.

        $('.form-group').each(function() {
            //...
            // if ($(".inputstatus .warning").length != 0) {
            //     evt.preventDefault();
            // }
            //...
            if (value === "") {
                //...
                // evt.preventDefault();
            }
            updateTerms($("#terms"));
        });
        if ($(".inputstatus .warning").length !== 0) {
            evt.preventDefault();
        }

Remove the need for checking requiredField length

The code to check the requiredField length is acting like a guard statement. Without it, troubles happen.

Here is that code:

            if ($requiredField.length === 0) {
                return;
            }

A part of the reason why is that some of the fields are not required.

Instead of making that check inside of the function, we can filter the form groups for only the ones that have a check class somewhere within them.

        // $('.form-group').has(".check").each(function() {
        $('.form-group').has(".check").each(function validateGroup() {
            var $requiredField = $(this).find(".check");
            // if ($requiredField.length === 0) {
            //     console.log(this);  
            //     return;
            // }

We can now remove the .check from inside of the function, and use a more specific statement to find either an input or textarea field.

        $('.form-group').has(".check").each(function validateGroup() {
            // var $requiredField = $(this).find(".check");
            var $requiredField = $(this).find("input textarea");
            var value = $requiredField.val().trim();
            if (value === "") {

Use inputStatus methods

The code inside of the if statement can now be updated using the inputStatus module.

Here is the code before we start:

    if (value === "") {
        $(this).find(".error").html(name + " is empty !").removeClass("ok").addClass("warning");
        $(this).find(".feedback").removeClass("glyphicon glyphicon-ok").addClass("glyphicon glyphicon-remove").removeClass("ok").addClass("warning");
        $(this).find(".starrq").removeClass("ok").addClass("warning");
        $("#termcheck").removeClass('ok').addClass('warning');
        $("#termsRequired").removeClass('ok').addClass('warning');
    }
    updateTerms($("#terms"));

Replacing that code with calls to inputStatus, it all becomes much simpler. The terms part isn’t needed either, as that’s all dealt with by the updateTerms part of the code.

    if (value === "") {
        inputStatus.errorWarning(this, name + " is empty !");
        inputStatus.feedbackWarning(this);
        inputStatus.setWarning($(this).find(".starrq"));
    }
    updateTerms($("#terms"));

That starrq part of the code might deserve being moved into the inputStatus module too, as a part of handling required stars. When I see another example error+feedback+starrq, I’ll move the starrq code in with the inputStatus code.

Move updateTerms out of the loop

There is no need for that updateTerms to be run for each and every required input field in the loop. It can be moved out of the loop instead.

$('.form-group').has(".check").each(function validateGroup() {
    var $requiredField = $(this).find("input, textarea");
    var name = $requiredField.attr("name");
    var value = $requiredField.val().trim();
    if (value === "") {
        inputStatus.errorWarning(this, name + " is empty !");
        inputStatus.feedbackWarning(this);
        inputStatus.setWarning($(this).find(".starrq"));
    }
    // updateTerms($("#terms"));
});
updateTerms($("#terms"));

That’s a big simplification to the code.

Summary

We have improved the registration submit event assignment, removed terms duplication, simplified the submit handler function, and used inputStatus methods to simplify things further.

The code as it stands today is found at v0.0.9 in releases

We have made our way through 4 different sets of duplication, as reported by the JSInspect program. Progress is slow, but reliable and steady. It takes some time to clean things up, but we have jsInspect that tells us that there is an eventual end to things.

Next time we attack another example of duplication in the code.

[Paul_Wilkins]

Removing duplication from the city input, is what we’re up to today. We will be do the following:

• simplify some test filenames
• use a basic test to test via the event handler function
• add a full suite of tests for the code
• remove duplication from the code
• add requiredOk and requiredWarning methods to inputStatus
• simplify the terms and conditions functions

Duplication number 20 found by jsInspect

Duplication numbers 24-21 have already been dealt with. I’m working backwards through the list so that I don’t have to scroll up hundreds of lines through the found examples.

The next example of duplication is found in the validate code when an input event occurs, such as when typing a character in an input field.

./registration3\validate.js:368,371
if (value === "") {
    $(this).next().find(".error").html("Your " + name + " field is Empty !").removeClass("ok").addClass("warning");

    $(this).next().find(".feedback").removeClass("glyphicon glyphicon-ok").addClass("glyphicon glyphicon-remove").removeClass("ok").addClass("warning");

./registration3\validate.js:561,563
if (value === "") {
    $(this).next().find(".error").html("Your " + name + " field is Empty !").removeClass("ok").addClass("warning");
    $(this).next().find(".feedback").removeClass("glyphicon glyphicon-ok").addClass("glyphicon glyphicon-remove").removeClass("ok").addClass("warning");

The two places involved are when the first name is empty, and when the city is empty.

If I resolve both of them, that might leave another case of duplication unfound, so I’ll only resolve the last set of duplication for now.

Organise tests

Before moving on though, the test filenames are growing in size, so the word validate should bd removed from the start of the filenames.

<!--<script src="../tests/validate-terms-click.test.js"></script>-->
<script src="../tests/terms-click.test.js"></script>
<!--<script src="../tests/validate-registration-submit.test.js"></script>-->
<script src="../tests/registration-submit.test.js"></script>
<!--<script src="../tests/validate-registration-reset.test.js"></script>-->
<script src="../tests/registration-reset.test.js"></script>

That means that the new test can be called registration-input-city.test.js

Make the input event easier to test

Before making any changes to the code, we need some tests to track what is currently done by the code, and give us assurance when changing the code that everything still works perfectly the same now as it did before.

The code is inside of an input event handler:

    $('.input-group').on('focusin focusout input', function() {
        //...
        if (name === "Your City") {
            //...
        }
        //...
    });

A test that ensures we are interacting with the above section of code, is as follows, where we empty the city value, remove its warning, and trigger an input event.

registration-input-city.test.js

describe("registration input city", function () {
    it("sets an error when city is empty", function () {
        const $city = $(".check[name='Your City']");
        $city.val("");
        $city.find(".error").removeClass("warning");
        const $cityInput = $city.closest(".input-group");
        $cityInput.trigger("input");
        const $cityGroup = $city.closest(".form-group");
        expect($cityGroup.find(".error").attr("class")).to.contain("warning");
    });
});

We can now rearrange the code to separate the event handler from the event assignment:

    // $('.input-group').on('focusin focusout input', function() {
    function registrationInputHandler(evt) {
        //...
        if (name === "Your City") {
            //...
        }
        //...
    // });
    }
    $('.input-group').on('focusin focusout input', registrationInputHandler);

make the registrationInputHandler accessible:

    return {
        eventHandler: {
            registrationInput: registrationInputHandler,
            //...

and the test can be modified to use registrationInputHandler instead.

describe("registration input city", function () {
    const registrationInputHandler = validate.eventHandler.registrationInput;
    it("sets an error when city is empty", function () {
        //...
        const $cityInput = $city.closest(".input-group");
        registrationInputHandler.call($cityInput);
        // $cityInput.trigger("input");
        //...
    });
});

The test still works all the way through the update, and we can now modify the test so that it can test using the input handler function instead of triggering an event.

Add more tests

We can now add more tests for everything that the city section of code does. I even included a structure section

describe("registration input city", function () {
    /*
       Structure
       - .form-group
         - .starrq
         - .input-group
           - input
         - .inputstatus
           - .error
           - .feedback
    */
    function callRegistrationInputHandler() {
        const registrationInputHandler = validate.eventHandler.registrationInput;
        const $cityInputGroup = $cityGroup.find(".input-group");
        registrationInputHandler.call($cityInputGroup);
    }
    const $cityGroup = $(".form-group").has("[name='Your City']");
    const $cityInput = $cityGroup.find("input");
    describe("when value is empty", function () {
        beforeEach(function () {
            $cityInput.val("");
        });
        describe("error", function () {
            const $cityError = $cityGroup.find(".error");
            it("gives an error message", function () {
                $cityError.html("");
                callRegistrationInputHandler();
                expect($cityError.html()).to.equal("Your Your City field is Empty !");
            });
            it("removes ok from error", function () {
                $cityError.addClass("ok");
                callRegistrationInputHandler();
                expect($cityError.attr("class")).to.not.contain("ok");
            });
            it("adds warning to error", function () {
                $cityError.removeClass("warning");
                callRegistrationInputHandler();
                expect($cityGroup.find(".error").attr("class")).to.contain("warning");
            });
        });
        describe("feedback", function () {
            const $cityFeedback = $cityGroup.find(".feedback");
            it("adds glyphicon to feedback", function () {
                $cityFeedback.removeClass("glyphicon");
                callRegistrationInputHandler();
                expect($cityFeedback.attr("class")).to.contain("glyphicon");
            });
            it("removes glyphicon-ok from feedback", function () {
                $cityFeedback.addClass("glyphicon-ok");
                callRegistrationInputHandler();
                expect($cityFeedback.attr("class")).to.not.contain("glyphicon-ok");
            });
            it("adds glyphicon-remove to feedback", function () {
                $cityFeedback.removeClass("glyphicon-remove");
                callRegistrationInputHandler();
                expect($cityFeedback.attr("class")).to.contain("glyphicon-remove");
            });
            it("removes ok from feedback", function () {
                $cityFeedback.addClass("ok");
                callRegistrationInputHandler();
                expect($cityFeedback.attr("class")).to.not.contain("ok");
            });
            it("adds warning to feedback", function () {
                $cityFeedback.removeClass("warning");
                callRegistrationInputHandler();
                expect($cityFeedback.attr("class")).to.contain("warning");
            });
        });
        describe("required", function () {
            const $cityRequired = $cityGroup.find(".starrq");
            it("removes ok from required", function () {
                $cityRequired.addClass("ok");
                callRegistrationInputHandler();
                expect($cityRequired.attr("class")).to.not.contain("ok");
            });
            it("adds warning to feedback", function () {
                $cityRequired.removeClass("warning");
                callRegistrationInputHandler();
                expect($cityRequired.attr("class")).to.contain("warning");
            });
        });
    });
    describe("when value has content", function () {
        beforeEach(function () {
            $cityInput.val("test value");
        });
        describe("error", function () {
            const $cityError = $cityGroup.find(".error");
            it("gives an error message", function () {
                $cityError.html("");
                callRegistrationInputHandler();
                expect($cityError.html()).to.equal("Your Your City field is OK !");
            });
            it("removes warning from error", function () {
                $cityError.addClass("warning");
                callRegistrationInputHandler();
                expect($cityError.attr("class")).to.not.contain("warning");
            });
            it("adds ok to error", function () {
                $cityError.removeClass("ok");
                callRegistrationInputHandler();
                expect($cityGroup.find(".error").attr("class")).to.contain("ok");
            });
        });
        describe("feedback", function () {
            const $cityFeedback = $cityGroup.find(".feedback");
            it("adds glyphicon to feedback", function () {
                $cityFeedback.removeClass("glyphicon");
                callRegistrationInputHandler();
                expect($cityFeedback.attr("class")).to.contain("glyphicon");
            });
            it("removes glyphicon-remove from feedback", function () {
                $cityFeedback.addClass("glyphicon-remove");
                callRegistrationInputHandler();
                expect($cityFeedback.attr("class")).to.not.contain("glyphicon-remove");
            });
            it("adds glyphicon-ok to feedback", function () {
                $cityFeedback.removeClass("glyphicon-ok");
                callRegistrationInputHandler();
                expect($cityFeedback.attr("class")).to.contain("glyphicon-ok");
            });
            it("removes warning from feedback", function () {
                $cityFeedback.addClass("warning");
                callRegistrationInputHandler();
                expect($cityFeedback.attr("class")).to.not.contain("warning");
            });
            it("adds ok to feedback", function () {
                $cityFeedback.removeClass("ok");
                callRegistrationInputHandler();
                expect($cityFeedback.attr("class")).to.contain("ok");
            });
        });
        describe("required", function () {
            const $cityRequired = $cityGroup.find(".starrq");
            it("removes warning from required", function () {
                $cityRequired.addClass("warning");
                callRegistrationInputHandler();
                expect($cityRequired.attr("class")).to.not.contain("warning");
            });
            it("adds ok to feedback", function () {
                $cityRequired.removeClass("ok");
                callRegistrationInputHandler();
                expect($cityRequired.attr("class")).to.contain("ok");
            });
        });
    });
});

Using a separate function call for the callRegistrationInputHandler also makes good sense. I’ve also updated the other tests with the same function call modification.

Remove duplication from the code

The code that we’re updating is the following:

        if (name === "Your City") {
            if (value === "") {
                $(this).next().find(".error").html("Your " + name + " field is Empty !").removeClass("ok").addClass("warning");
                $(this).next().find(".feedback").removeClass("glyphicon glyphicon-ok").addClass("glyphicon glyphicon-remove").removeClass("ok").addClass("warning");
                $("#cityRequired").removeClass("ok").addClass("warning");
            } else {
                $(this).next().find(".error").html("Your " + name + " field is OK !").removeClass("warning").addClass("ok");
                $(this).next().find(".feedback").removeClass("glyphicon glyphicon-remove").addClass("glyphicon glyphicon-ok").removeClass("warning").addClass("ok");
                $("#cityRequired").removeClass("warning").addClass("ok");
            }
        }

We can use inputStatus methods to help simplify that code.

        if (name === "Your City") {
            const $formGroup = $(this).closest(".form-group");
            if (value === "") {
                inputStatus.errorWarning($formGroup, "Your " + name + " field is Empty !");
                inputStatus.feedbackWarning($formGroup);
                inputStatus.setWarning($formGroup.find(".starrq"));
            } else {
                inputStatus.errorOk($formGroup, "Your " + name + " field is OK !");
                inputStatus.feedbackOk($formGroup);
                inputStatus.setOk($formGroup.find(".starrq"));
            }
        }

requiredWarning and requiredOk methods

I said before that if we see that errorOk/FeedbackOk/setOk pattern again, that I’m adding requiredWarning and requiredOk methods to the inputStatus code. It’s now time to do that.

input-status.js

   function requiredOk(inputGroup, message) {
        const $required = $(inputGroup).find(".starrq");
        setOk($required);
    }
    function requiredWarning(inputGroup, message) {
        const $required = $(inputGroup).find(".starrq");
        setWarning($required);
    }
    return {
        //...
        requiredOk,
        requiredWarning
    };

We can now use requiredWarning and requireOk from the main set of code:

validate.js

        if (name === "Your City") {
            const $formGroup = $(this).closest(".form-group");
            if (value === "") {
                inputStatus.errorWarning($formGroup, "Your " + name + " field is Empty !");
                inputStatus.feedbackWarning($formGroup);
                // inputStatus.setWarning($formGroup.find(".starrq"));
                inputStatus.requiredWarning($formGroup);
            } else {
                inputStatus.errorOk($formGroup, "Your " + name + " field is OK !");
                inputStatus.feedbackOk($formGroup);
                // inputStatus.setOk($formGroup.find(".starrq"));
                inputStatus.requiredOk($formGroup);
            }
        }

I should also update the other code that used setWarning and setOk

validate.js

    function resetMessages() {
        const $error = $(this).find(".error");
        const name = $(this).find(".check").attr("name");
        inputStatus.errorOk(this, name);
        inputStatus.feedbackNone(this);
        // inputStatus.setOk($(this).find(".starrq"));
        inputStatus.requiredOk(this);
    }
//...
    function removeTermWarning() {
        const $termsGroup = $("#terms").closest(".form-group");
        inputStatus.feedbackNone($termsGroup);
        inputStatus.setOk($termsGroup.find("#termcheck"));
        // inputStatus.setOk($termsGroup.find("#termsRequired"));
        inputStatus.requiredOk($termsGroup);
    }
//...
    function updateTerms(terms) {
        const $termsGroup = $(".form-group").has($(terms));
        if ($(terms).is(":checked")) {
            inputStatus.setOk($("#termcheck"));
            // inputStatus.requiredOk($("#termsRequired"));
            inputStatus.requiredOk($termsGroup);
        } else {
            inputStatus.setWarning($("#termcheck"));
            // inputStatus.requiredWarning($("#termsRequired"));
            inputStatus.requiredWarning($termsGroup);
        }
    }

And this is where I make another declaration. If I see error/feedback/required grouped together again, I should move that grouped set in to the inputStatus module too.

Simplify updateTerms function

While I’m updating the updateTerms function, I should also remove the terms parameter as it’s not needed there.

    function updateTerms() {
        const $termsGroup = $(".form-group").has("#terms");
        const $terms = $termsGroup.find("#terms");
        if ($terms.is(":checked")) {
            inputStatus.setOk($termsGroup.find(".error2"));
            inputStatus.requiredOk($termsGroup);
        } else {
            inputStatus.setWarning($termsGroup.find(".error2"));
            inputStatus.requiredWarning($termsGroup);
        }
    }
//...
    function termsClickHandler() {
        // updateTerms(this);
        updateTerms();
    }
//...
    function registrationSubmitHandler(evt) {
        //...
        // updateTerms($("#terms"));
        updateTerms();
        //...
    }

Summary

We have improved the test filenames, tested the city input code, removed duplication that was found, added some useful requiredOk and requiredWarning methods to inputStatus, and simplified some of the terms and conditions code.

The code as it stands today is found at v0.0.10 in releases

Next time we will work on a more complicated set of duplication, to do with passwords.

[Paul_Wilkins]

Removing duplication from the retype password code, is what we’re up to today. We will be doing the following:

• investigating the duplicate retype-password code
• removing an unused section of code
• adding tests for when retyping the password
• add some TODO notes to the tests
• removing duplication by using inputStatus code

Duplication found in password code

./registration3\validate.js:640,654
        $(this).next().find(".errorm").addClass('warning').removeClass('ok');
        $(this).next().find(".feedback").removeClass("glyphicon glyphicon-ok").addClass("glyphicon glyphicon-remove").removeClass("ok").addClass("warning");
        $("#pwreRequired").removeClass("ok").addClass("warning");
    } else {
        $(this).next().find(".error").html(name + " is OK: Your data has been entered correctly " + inputstr);
        $(this).next().find(".error").addClass('ok').removeClass('warning');
        $(this).next().find(".feedback").removeClass("glyphicon glyphicon-remove").addClass("glyphicon glyphicon-ok").removeClass("warning").addClass("ok");
        $("#pwreRequired").removeClass("warning").addClass("ok");
    }
} else {
    $(this).next().find(".error").html(name + " is EMPTY: Please enter data into this input");
    $(this).next().find(".error").addClass('warning').removeClass('ok');
    $(this).next().find(".feedback").removeClass("glyphicon glyphicon-ok").addClass("glyphicon glyphicon-remove").removeClass("ok").addClass("warning");
    $("#pwreRequired").removeClass("warning").addClass("ok");
}

./registration3\validate.js:767,781
        $(this).next().find(".error").addClass('warning').removeClass('ok');
        $(this).next().find(".feedback").removeClass("glyphicon glyphicon-ok").addClass("glyphicon glyphicon-remove").removeClass("ok").addClass("warning");
        $("#pwreRequired").removeClass("ok").addClass("warning");
    } else {
        $(this).next().find(".error").html(name + " is OK: Your data has been entered correctly " + inputstr);
        $(this).next().find(".error").addClass('ok').removeClass('warning');
        $(this).next().find(".feedback").removeClass("glyphicon glyphicon-remove").addClass("glyphicon glyphicon-ok").removeClass("warning").addClass("ok");
        $("#pwreRequired").removeClass("warning").addClass("ok");
    }
} else {
    $(this).next().find(".error").html(name + " is EMPTY: Please enter data into this input");
    $(this).next().find(".error").addClass('warning').removeClass('ok');
    $(this).next().find(".feedback").removeClass("glyphicon glyphicon-ok").addClass("glyphicon glyphicon-remove").removeClass("ok").addClass("warning");
    $("#pwreRequired").removeClass("warning").addClass("ok");
}

Both are for the confirm password field. One of them is in the registration input handler at line 345, that were were dealing with in the previous post, and the other one is in a place for input-groupmodel elements at line 659, but that doesn’t seem to exist in any of the HTML code.

Is the input-groupmodel code needed?

On further investigation, the input-groupmodel code in the validate.js file contains three sections for email/password/change password.

The input-groupmodel code doesn’t get run at all, and looks to be left over from the changepassword code that already exists elsewhere in the change-password.js file. Deleting that code from line 659 through to 784 is the right choice to make.

    $('.input-group').on('focusin focusout input', registrationInputHandler);

    // $('.input-groupmodal').on('focusin focusout input', function() {
        // var namem = $(this).find(".check,textarea").attr("name");
        // var valuem = $(this).find(".check,textarea").val().trim();
        //...
    // });

We can now put together tests for the remaining code that we will be updating.

Tests

Here is the retype password code for which we are putting together tests.

        if (name === "Retype Password") {
            if (value.length > 0) {
                if (inputstr !== inputs.Password.value) {
                    $(this).next().find(".error").html(name + " is Incorrect: Password doesn't match retyped pwd ");
                    $(this).next().find(".errorm").html(name + " is Incorrect: Password doesn't match retyped pwd ");
                    $(this).next().find(".error").addClass('warning').removeClass('ok');
                    $(this).next().find(".errorm").addClass('warning').removeClass('ok');
                    $(this).next().find(".feedback").removeClass("glyphicon glyphicon-ok").addClass("glyphicon glyphicon-remove").removeClass("ok").addClass("warning");
                    $("#pwreRequired").removeClass("ok").addClass("warning");
                } else {
                    $(this).next().find(".error").html(name + " is OK: Your data has been entered correctly " + inputstr);
                    $(this).next().find(".error").addClass('ok').removeClass('warning');
                    $(this).next().find(".feedback").removeClass("glyphicon glyphicon-remove").addClass("glyphicon glyphicon-ok").removeClass("warning").addClass("ok");
                    $("#pwreRequired").removeClass("warning").addClass("ok");
                }
            } else {
                $(this).next().find(".error").html(name + " is EMPTY: Please enter data into this input");
                $(this).next().find(".error").addClass('warning').removeClass('ok');
                $(this).next().find(".feedback").removeClass("glyphicon glyphicon-ok").addClass("glyphicon glyphicon-remove").removeClass("ok").addClass("warning");
                $("#pwreRequired").removeClass("warning").addClass("ok");
            }
        }

That’s a lot of changes to the page, so it’s a lot of tests to ensure that it all is properly tested.

The tests for the above code are:

describe("registration-input retype password", function () {
    /*
       Structure
       - .form-group
         - .starrq
         - .input-group
           - input
         - .inputstatus
           - .error
           - .feedback
    */
    function callRegistrationInputHandler(thisArg) {
        const registrationInputHandler = validate.eventHandler.registrationInput;
        registrationInputHandler.call(thisArg);
    }
    const $retypeGroup = $(".form-group").has("[name='Retype Password']");
    const $retypeInputGroup = $retypeGroup.find(".input-group");
    const $retypeInput = $retypeGroup.find("input");
    const $retypeError = $retypeGroup.find(".error");
    const $retypeFeedback = $retypeGroup.find(".feedback");
    const $retypeRequired = $retypeGroup.find(".starrq");
    describe("value has content", function () {
        beforeEach(function () {
            $retypeInput.val("test value");
        });
        describe("doesn't match password", function () {
            beforeEach(function () {
                const $password = $("[name=Password]");
                $password.val($retypeInput.val() + "nomatch");
            });
            it("shows an error message", function () {
                $retypeError.html("");
                callRegistrationInputHandler($retypeInputGroup);
                expect($retypeError.html()).to.equal("Retype Password is Incorrect: Password doesn't match retyped pwd ");
            });
            it("adds warning to error", function () {
                $retypeError.removeClass("warning");
                callRegistrationInputHandler($retypeInputGroup);
                expect($retypeError.attr("class")).to.contain("warning");
            });
            it("removes ok from error", function () {
                $retypeError.addClass("ok");
                callRegistrationInputHandler($retypeInputGroup);
                expect($retypeError.attr("class")).to.not.contain("ok");
            });
            it("adds glyphicon to feedback", function () {
                $retypeFeedback.removeClass("glyphicon");
                callRegistrationInputHandler($retypeInputGroup);
                expect($retypeFeedback.attr("class")).to.contain("glyphicon");
            });
            it("removes glyphicon-ok from feedback", function () {
                $retypeFeedback.addClass("glyphicon-ok");
                callRegistrationInputHandler($retypeInputGroup);
                expect($retypeFeedback.attr("class")).to.not.contain("glyphicon-ok");
            });
            it("adds glyphicon-remove to feedback", function () {
                $retypeFeedback.removeClass("glyphicon-remove");
                callRegistrationInputHandler($retypeInputGroup);
                expect($retypeFeedback.attr("class")).to.contain("glyphicon-remove");
            });
            it("removes ok from feedback", function () {
                $retypeFeedback.addClass("ok");
                callRegistrationInputHandler($retypeInputGroup);
                expect($retypeFeedback.attr("class")).to.not.contain("ok");
            });
            it("adds warning to feedback", function () {
                $retypeFeedback.removeClass("warning");
                callRegistrationInputHandler($retypeInputGroup);
                expect($retypeFeedback.attr("class")).to.contain("warning");
            });
            it("removes ok from required", function () {
                $retypeRequired.addClass("ok");
                callRegistrationInputHandler($retypeInputGroup);
                expect($retypeRequired.attr("class")).to.not.contain("ok");
            });
            it("adds warning to required", function () {
                $retypeRequired.removeClass("warning");
                callRegistrationInputHandler($retypeInputGroup);
                expect($retypeRequired.attr("class")).to.contain("warning");
            });
        });
        describe("does match password", function () {
            beforeEach(function () {
                const $password = $("[name=Password]");
                $password.val($retypeInput.val());
            });
            const $retypeError = $retypeGroup.find(".error");
            const $retypeFeedback = $retypeGroup.find(".feedback");
            const $retypeRequired = $retypeGroup.find(".starrq");
            it("shows an error message", function () {
                $retypeError.html("");
                callRegistrationInputHandler($retypeInputGroup);
                expect($retypeError.html()).to.equal("Retype Password is OK: Your data has been entered correctly test value");
                // TODO - it's really bad security to show the password on the screen
            });
            it("adds ok to error", function () {
                $retypeError.removeClass("ok");
                callRegistrationInputHandler($retypeInputGroup);
                expect($retypeError.attr("class")).to.contain("ok");
            });
            it("removes warning from error", function () {
                $retypeError.addClass("warning");
                callRegistrationInputHandler($retypeInputGroup);
                expect($retypeError.attr("class")).to.not.contain("warning");
            });
            it("adds glyphicon to feedback", function () {
                $retypeFeedback.removeClass("glyphicon");
                callRegistrationInputHandler($retypeInputGroup);
                expect($retypeFeedback.attr("class")).to.contain("glyphicon");
            });
            it("removes glyphicon-remove from feedback", function () {
                $retypeFeedback.addClass("glyphicon-remove");
                callRegistrationInputHandler($retypeInputGroup);
                expect($retypeFeedback.attr("class")).to.not.contain("glyphicon-remove");
            });
            it("adds glyphicon-ok to feedback", function () {
                $retypeFeedback.removeClass("glyphicon-ok");
                callRegistrationInputHandler($retypeInputGroup);
                expect($retypeFeedback.attr("class")).to.contain("glyphicon-ok");
            });
            it("removes warning from feedback", function () {
                $retypeFeedback.addClass("warning");
                callRegistrationInputHandler($retypeInputGroup);
                expect($retypeFeedback.attr("class")).to.not.contain("warning");
            });
            it("adds ok to feedback", function () {
                $retypeFeedback.removeClass("ok");
                callRegistrationInputHandler($retypeInputGroup);
                expect($retypeFeedback.attr("class")).to.contain("ok");
            });
            it("removes warning from required", function () {
                $retypeRequired.addClass("warning");
                callRegistrationInputHandler($retypeInputGroup);
                expect($retypeRequired.attr("class")).to.not.contain("warning");
            });
            it("adds ok to required", function () {
                $retypeRequired.removeClass("ok");
                callRegistrationInputHandler($retypeInputGroup);
                expect($retypeRequired.attr("class")).to.contain("ok");
            });
        });
    });
    describe("value is empty", function () {
        beforeEach(function () {
            $retypeInput.val("");
        });
        const $retypeError = $retypeGroup.find(".error");
        const $retypeFeedback = $retypeGroup.find(".feedback");
        const $retypeRequired = $retypeGroup.find(".starrq");
        it("shows an error message", function () {
            $retypeError.html("");
            callRegistrationInputHandler($retypeInputGroup);
            expect($retypeError.html()).to.equal("Retype Password is EMPTY: Please enter data into this input");
        });
        it("adds warning to error", function () {
            $retypeError.removeClass("warning");
            callRegistrationInputHandler($retypeInputGroup);
            expect($retypeError.attr("class")).to.contain("warning");
        });
        it("removes ok from error", function () {
            $retypeError.addClass("ok");
            callRegistrationInputHandler($retypeInputGroup);
            expect($retypeError.attr("class")).to.not.contain("ok");
        });
        it("adds glyphicon to feedback", function () {
            $retypeFeedback.removeClass("glyphicon");
            callRegistrationInputHandler($retypeInputGroup);
            expect($retypeFeedback.attr("class")).to.contain("glyphicon");
        });
        it("removes glyphicon-ok from feedback", function () {
            $retypeFeedback.addClass("glyphicon-ok");
            callRegistrationInputHandler($retypeInputGroup);
            expect($retypeFeedback.attr("class")).to.not.contain("glyphicon-ok");
        });
        it("adds glyphicon-remove to feedback", function () {
            $retypeFeedback.removeClass("glyphicon-remove");
            callRegistrationInputHandler($retypeInputGroup);
            expect($retypeFeedback.attr("class")).to.contain("glyphicon-remove");
        });
        it("removes ok from feedback", function () {
            $retypeFeedback.addClass("ok");
            callRegistrationInputHandler($retypeInputGroup);
            expect($retypeFeedback.attr("class")).to.not.contain("ok");
        });
        it("adds warning to feedback", function () {
            $retypeFeedback.removeClass("warning");
            callRegistrationInputHandler($retypeInputGroup);
            expect($retypeFeedback.attr("class")).to.contain("warning");
        });
        it("removes warning from required", function () {
            $retypeRequired.addClass("warning");
            callRegistrationInputHandler($retypeInputGroup);
            expect($retypeRequired.attr("class")).to.not.contain("warning");
        });
        it("adds ok to required", function () {
            $retypeRequired.removeClass("ok");
            callRegistrationInputHandler($retypeInputGroup);
            expect($retypeRequired.attr("class")).to.contain("ok");
        });
    });
});

I’ve been noticing some issues while putting together the tests, which I’ve indicated as TODO sections. I won’t put TODO comments into production code, but using them locally is okay for me, so long as I don’t commit those todo notes.

Remove duplicate code

The duplicated code in this retype-password section can be replaced by the inputStatus code that we have already organised.

The updated retype password code is the following:

        if (name === "Retype Password") {
            const $formGroup = $(this).closest(".form-group");
            if (value.length > 0) {
                if (inputstr !== inputs.Password.value) {
                    inputStatus.errorWarning($formGroup, name + " is Incorrect: Password doesn't match retyped pwd ");
                    inputStatus.feedbackWarning($formGroup);
                    inputStatus.requiredWarning($formGroup);
                } else {
                    inputStatus.errorOk($formGroup, name + " is OK: Your data has been entered correctly " + inputstr);
                    inputStatus.feedbackOk($formGroup);
                    inputStatus.requiredOk($formGroup);
                }
            } else {
                inputStatus.errorWarning($formGroup, name + " is EMPTY: Please enter data into this input");
                inputStatus.feedbackWarning($formGroup);
                inputStatus.requiredOk($formGroup);
            }
        }

And look! We have more of that error/feedback/required duplication that was noticed in the previous post. We will deal with that in the next post, along with a series of TODO issues that have been building up.

Summary

We have removed an entire section of code that’s already done by the change-password section of code, added tests for when retyping passwords on the main registration page, and improved the code by using the inputStatus code.

The code as it stands today is found at v0.0.11 in releases

Next time we look into some TODO issues that have been building up.

[Paul_Wilkins]

Today we’ll be clearing up some TODO issues that I’ve been noting down. We will be doing the following:

• grouping error/warning/required code into inputStatus
• preventing the password from being shown on the screen
• cleaning up the formatting of some text messages
• investigated inconsistent messages for the same thing

Add error/warning/required group to inputStatus

Frequently we’ve been noticing that error/feedback/required are used in the same way, in many parts of the code.
Earlier I said that when I come across that happening again, I would add it as a separate function in the inputStatus module, and I saw it in the previous post.

Here is the extra code that I’ve added to inputStatus for the ok and warning functions.

    function ok(inputGroup, message) {
        errorOk($formGroup, message);
        feedbackOk($formGroup);
        requiredOk($formGroup);
    }
    function warning(inputGroup, message) {
        errorWarning($formGroup, message);
        feedbackWarning($formGroup);
        requiredWarning($formGroup);
    }
    return {
        //...
        ok,
        warning
    };

The retype code can now be updated to use those new inputStatus methods:

        if (name === "Retype Password") {
            const $formGroup = $(this).closest(".form-group");
            if (value.length > 0) {
                if (inputstr !== inputs.Password.value) {
                    inputStatus.warning($formGroup, name + " is Incorrect: Password doesn't match retyped pwd ");
                } else {
                    inputStatus.ok($formGroup, name + " is OK: Your data has been entered correctly " + inputstr);
                }
            } else {
                inputStatus.warning($formGroup, name + " is EMPTY: Please enter data into this input");
                inputStatus.requiredOk($formGroup);
            }
        }

That + inputstr part really shouldn’t be there. It’s terrible security to let the password be shown on the screen. I’ve left a TODO note to take care of that shortly.

Use inputStatus ok and warning, elsewhere in the code

The city input is also where the new inputStatus methods can be used.

        if (name === "Your City") {
            const $formGroup = $(this).closest(".form-group");
            if (value === "") {
                // inputStatus.errorWarning($formGroup, "Your " + name + " field is Empty !");
                // inputStatus.feedbackWarning($formGroup);
                // inputStatus.requiredWarning($formGroup);
                inputStatus.warning($formGroup, "Your " + name + " field is Empty !");
            } else {
                // inputStatus.errorOk($formGroup, "Your " + name + " field is OK !");
                // inputStatus.feedbackOk($formGroup);
                // inputStatus.requiredOk($formGroup);
                inputStatus.ok($formGroup, "Your " + name + " field is OK !");
            }
        }

And another place where this improvement can occur is in the registrationSubmitHandler

            if (value === "") {
                // inputStatus.errorWarning(this, name + " is empty !");
                // inputStatus.feedbackWarning(this);
                // inputStatus.setWarning($(this).find(".starrq"));
                inputStatus.warning(this, name + " is empty!");
            }

Improve the retype-password structure

The first condition of retyping the password should be about if the two match, so we can somewhat simplify the code by reducing the amount of nested if/else code.

        if (name === "Retype Password") {
            const $formGroup = $(this).closest(".form-group");
            if (inputstr === inputs.Password.value) {
                inputStatus.ok($formGroup, name + " is OK: Your data has been entered correctly " + inputstr);
            } else if (value.length > 0) {
                inputStatus.warning($formGroup, name + " is Incorrect: Password doesn't match retyped pwd ");
            } else {
                inputStatus.warning($formGroup, name + " is EMPTY: Please enter data into this input");
                inputStatus.requiredOk($formGroup);
            }
        }

Take care of TODO issues

I’ve been using TODO’s to note a series of small issues. Now is a good time to take care of them.

Prevent the password from being shown on the screen

One of my TODO notes is about preventing the password from being shown on the screen. This is an easy fix, that just means removing the + inputstr part from the test, and from the code.

Let’s update the test to represent what we want the code to do instead, which is to not show the test value.

registration-input-retypepassword.test.js

            it("shows an error message", function () {
                $retypeError.html("");
                callRegistrationInputHandler($retypeInputGroup);
                // expect($retypeError.html()).to.equal("Retype Password is OK: Your data has been entered correctly test value");
                expect($retypeError.html()).to.equal("Retype Password is OK: Your data has been entered correctly");
            });

We can now update the code to match.

validate.js

            if (inputstr === inputs.Password.value) {
                // inputStatus.ok($formGroup, name + " is OK: Your data has been entered correctly " + inputstr);
                inputStatus.ok($formGroup, name + " is OK: Your data has been entered correctly");

Tidy up the formatting of several messages

Several of the messages have spaces in weird places, such as directly before the exclamation mark, or they leave an empty space at the end of a string. Now it a good time to clean up these issues in the text.

We update the test to indicate what we want to occur:

registration-input-retypepassword.test.js

            it("shows an error message", function () {
                $retypeError.html("");
                callRegistrationInputHandler($retypeInputGroup);
                // expect($retypeError.html()).to.equal("Retype Password is Incorrect: Password doesn't match retyped pwd ");
                expect($retypeError.html()).to.equal("Retype Password is Incorrect: Password doesn't match retyped pwd");
            });

And we update the code so that the test passes.

validate.js

            } else if (value.length > 0) {
                // inputStatus.warning($formGroup, name + " is Incorrect: Password doesn't match retyped pwd ");
                inputStatus.warning($formGroup, name + " is Incorrect: Password doesn't match retyped pwd");

This is a very simple test and update process that almost seems unnecessary, but it’s like indicating when exiting a roundabout. You don’t really need it until it’s too late, and then it’s too late.

Compare two possible ways to handle tests:

Way one - update the code first and then afterwards make the tests match. You end up feeling resistant to doing more tests because they just slow you down and get in your way.

Way two - update the test first and then afterwards make the code pass. You feel bouyant about writing a simple test that fails, because you can then write the code to make that test pass. Then while the test continues to pass you can improve the code with no fear that you’re going to break anything, because tests always have your back.

I much prefer the second way to handle things. Here is another simple update to the test and the code to improve the text:

registration-input-city.test.js

            it("gives an error message", function () {
                $cityError.html("");
                callRegistrationInputHandler($cityInputGroup);
                // expect($cityError.html()).to.equal("Your Your City field is Empty !");
                expect($cityError.html()).to.equal("Your Your City field is Empty!");
            });
//...
            it("gives an error message", function () {
                $cityError.html("");
                callRegistrationInputHandler($cityInputGroup);
                // expect($cityError.html()).to.equal("Your Your City field is OK !");
                expect($cityError.html()).to.equal("Your Your City field is OK!");
            });

        if (name === "Your City") {
            const $formGroup = $(this).closest(".form-group");
            if (value === "") {
                // inputStatus.warning($formGroup, "Your " + name + " field is Empty !");
                inputStatus.warning($formGroup, "Your " + name + " field is Empty!");
            } else {
                // inputStatus.ok($formGroup, "Your " + name + " field is OK !");
                inputStatus.ok($formGroup, "Your " + name + " field is OK!");
            }
        }

There is still an issue about the message saying “Your Your City” because the field is named “Your City”. I strongly recommend renaming that field to be just “City” instead. I will not be making that change right here right now though, because it’s vital that changes to field names be done in tandem with server-side code changes to match up with the renamed field.

registration-submit.test.js

            it("shows the error text", function () {
                $firstnameError.html("");
                registrationSubmitHandler(fakeEvt);
                // expect($firstnameError.html()).to.equal("First Name is empty !");
                expect($firstnameError.html()).to.equal("First Name is empty!");
            });

validate.js

            if (value === "") {
                // inputStatus.errorWarning(this, name + " is empty !");
                inputStatus.warning(this, name + " is empty!");
            }

These issues of presentation are all minor annoyances, but it’s important to be consistant.

Investigate “is empty” issues

Speaking of consistent, I’m noticing just now that sometimes “is empty” is used, and other times “is Empty” is used.

Let’s take a look at how “is empty” is presented throughout the code.

• Your Your City field is Empty !
• Your First Name field is Empty !
• Phone Number is EMPTY: Please enter data into this input
• E-mail is empty
• Postal Address is EMPTY: Please enter data into this input
• zip code is EMPTY: Please enter data into this input
• Your Your City field is Empty!
• Password is EMPTY: Please enter data into this input
• Retype Password is EMPTY: Please enter data into this input
• (on submit) Field Name is empty!
• (login) Password is empty
• (login) Your Field Name is empty
• (retype) Password Retype is empty
• (retype) Your Field Name is empty

Those are a lot of different ways to say the same thing. I strongly recommend coming up with a consistent way to say this in the same way, for every different form field. Its not unique to the fields about being empty either.

It can be very easy to make things consistent. All we would need to do is to have the inputStatus code check if the field is empty, and use the same message for every single time that occurs. That is an idea that we might make use of later.

Summary

We have grouped error/warning/required code into inputStatus, prevented the password from being shown on the screen, cleaned up the formatting of some text messages, and investigated some inconsistent “is empty” messages.

The code as it stands today is found at v0.0.12 in releases

Next time we gaze in wonder at going from 19 cases of duplication down to only 12.

[Paul_Wilkins]

Today we will be removing duplication from code about removing repetition, which is somewhat apt.

We will be doing the following:

• tidying up an annoying trailing spaces issue
• adding tests for code that we are going to change
• replace duplicated code using inputStatus.warning()

Remove trailing space from text

The trailing spaces in the text messages have become so when I come across them, that it’s better to take care of them now sooner than later.

        // $(this).next().find(".error").html(name + " is Fake text: Please remove repetition ");
        $(this).next().find(".error").html(name + " is Fake text: Please remove repetition");

All of the code has now been updated to remove this particular burr from my mind.

Duplication #12, remove repetition

For some strange reason we went from 19 sets of duplication down now to only 12. We have been highly effective at removing duplication from the code, when dealing with the duplication in the retype password section.

This set of duplication occurs in the code that checks for repetition, at lines 387, 425, 485, and 572.

        } else {
            if (fakeReg.test(value)) {
                $(this).next().find(".error").html(name + " is Fake text: Please remove repetition");
                $(this).next().find(".feedback").removeClass("glyphicon glyphicon-ok").addClass("glyphicon glyphicon-remove").removeClass("ok").addClass("warning");
                $(this).next().find(".error").addClass('warning').removeClass('ok');
                $("#fnameRequired").removeClass("ok").addClass("warning");

This should be an easy case of replacing it with a single line of inputStatus.warning() code instead, but the protocols must still be observed.

Have tests for the code

Before making any changes to this code, we need a way to easily tell that it still carries on working in the same way that it does now. We are using Mocha+Chai to help us to easily do that.

The four sections of repetition code are in first name, last name, email, and password.

Create some tests

I’m putting the first name tests into a file called registration-input-firstname.test.js

<script src="../tests/registration-input-firstname.test.js"></script>
<script src="../tests/registration-input-city.test.js"></script>

From other tests in this registration-input area we already have an easy way to access the code we want to test, using the registration input handler, so it’s just a matter of detailing all that the code is supposed to do.

registration-input-firstname.test.js

describe("registration-input first name", function () {
    /*
       Structure
       - .form-group
         - .starrq
         - .input-group
           - input
         - .inputstatus
           - .error
           - .feedback
    */
    function callRegistrationInputHandler(thisArg) {
        const registrationInputHandler = validate.eventHandler.registrationInput;
        registrationInputHandler.call(thisArg);
    }
    const $firstnameGroup = $(".form-group").has("[name='First Name']");
    const $firstnameInputGroup = $firstnameGroup.find(".input-group");
    const $firstnameInput = $firstnameGroup.find("input");
    const $firstnameError = $firstnameGroup.find(".error");
    const $firstnameFeedback = $firstnameGroup.find(".feedback");
    const $firstnameRequired = $firstnameGroup.find(".starrq");
    describe("first name has repetition", function () {
        beforeEach(function () {
            $firstnameInput.val("abbbc");
        });
        it("shows an error message", function () {
            $firstnameError.html("");
            callRegistrationInputHandler($firstnameInputGroup);
            expect($firstnameError.html()).to.equal("First Name is Fake text: Please remove repetition");
        });
        it("adds warning to error", function () {
            $firstnameError.removeClass("warning");
            callRegistrationInputHandler($firstnameInputGroup);
            expect($firstnameError.attr("class")).to.contain("warning");
        });
        it("removes ok from error", function () {
            $firstnameError.addClass("ok");
            callRegistrationInputHandler($firstnameInputGroup);
            expect($firstnameError.attr("class")).to.not.contain("ok");
        });
        it("adds glyphicon to feedback", function () {
            $firstnameFeedback.removeClass("glyphicon");
            callRegistrationInputHandler($firstnameInputGroup);
            expect($firstnameFeedback.attr("class")).to.contain("glyphicon");
        });
        it("removes glyphicon-ok from feedback", function () {
            $firstnameFeedback.addClass("glyphicon-ok");
            callRegistrationInputHandler($firstnameInputGroup);
            expect($firstnameFeedback.attr("class")).to.not.contain("glyphicon-ok");
        });
        it("adds glyphicon-remove to feedback", function () {
            $firstnameFeedback.removeClass("glyphicon-remove");
            callRegistrationInputHandler($firstnameInputGroup);
            expect($firstnameFeedback.attr("class")).to.contain("glyphicon-remove");
        });
        it("removes ok from feedback", function () {
            $firstnameFeedback.addClass("ok");
            callRegistrationInputHandler($firstnameInputGroup);
            expect($firstnameFeedback.attr("class")).to.not.contain("ok");
        });
        it("adds warning to feedback", function () {
            $firstnameFeedback.removeClass("warning");
            callRegistrationInputHandler($firstnameInputGroup);
            expect($firstnameFeedback.attr("class")).to.contain("warning");
        });
        it("removes ok from required", function () {
            $firstnameRequired.addClass("ok");
            callRegistrationInputHandler($firstnameInputGroup);
            expect($firstnameRequired.attr("class")).to.not.contain("ok");
        });
        it("adds warning to required", function () {
            $firstnameRequired.removeClass("warning");
            callRegistrationInputHandler($firstnameInputGroup);
            expect($firstnameRequired.attr("class")).to.contain("warning");
        });
    });
});

Those are the tests for the code from line 387. We only have other tests for the code at 425, 485, and 572 to go.

Create more tests

The lastname, email, and password tests get added to the HTML test page in the same way.

<script src="../tests/registration-input-firstname.test.js"></script>
<script src="../tests/registration-input-lastname.test.js"></script>
<script src="../tests/registration-input-email.test.js"></script>
<script src="../tests/registration-input-city.test.js"></script>
<script src="../tests/registration-input-password.test.js"></script>

To help keep things organised, I’ve ordered the filenames by the same order that they appear in the form.

The lastname repetition tests are similar to the firstname repetition tests, and tempting though it is to ignore them, they can’t be ignored as it’s interacting with a different set of HTML code in another area of the page.

registration-input-lastname.test.js

describe("registration-input last name", function () {
    /*
       Structure
       - .form-group
         - .starrq
         - .input-group
           - input
         - .inputstatus
           - .error
           - .feedback
    */
    function callRegistrationInputHandler(thisArg) {
        const registrationInputHandler = validate.eventHandler.registrationInput;
        registrationInputHandler.call(thisArg);
    }
    const $lastnameGroup = $(".form-group").has("[name='Last Name']");
    const $lastnameInputGroup = $lastnameGroup.find(".input-group");
    const $lastnameInput = $lastnameGroup.find("input");
    const $lastnameError = $lastnameGroup.find(".error");
    const $lastnameFeedback = $lastnameGroup.find(".feedback");
    const $lastnameRequired = $lastnameGroup.find(".starrq");
    describe("first name has repetition", function () {
        beforeEach(function () {
            $lastnameInput.val("abbbc");
        });
        it("shows an error message", function () {
            $lastnameError.html("");
            callRegistrationInputHandler($lastnameInputGroup);
            expect($lastnameError.html()).to.equal("Last Name is Fake text: Please remove repetition");
        });
        it("adds warning to error", function () {
            $lastnameError.removeClass("warning");
            callRegistrationInputHandler($lastnameInputGroup);
            expect($lastnameError.attr("class")).to.contain("warning");
        });
        it("removes ok from error", function () {
            $lastnameError.addClass("ok");
            callRegistrationInputHandler($lastnameInputGroup);
            expect($lastnameError.attr("class")).to.not.contain("ok");
        });
        it("adds glyphicon to feedback", function () {
            $lastnameFeedback.removeClass("glyphicon");
            callRegistrationInputHandler($lastnameInputGroup);
            expect($lastnameFeedback.attr("class")).to.contain("glyphicon");
        });
        it("removes glyphicon-ok from feedback", function () {
            $lastnameFeedback.addClass("glyphicon-ok");
            callRegistrationInputHandler($lastnameInputGroup);
            expect($lastnameFeedback.attr("class")).to.not.contain("glyphicon-ok");
        });
        it("adds glyphicon-remove to feedback", function () {
            $lastnameFeedback.removeClass("glyphicon-remove");
            callRegistrationInputHandler($lastnameInputGroup);
            expect($lastnameFeedback.attr("class")).to.contain("glyphicon-remove");
        });
        it("removes ok from feedback", function () {
            $lastnameFeedback.addClass("ok");
            callRegistrationInputHandler($lastnameInputGroup);
            expect($lastnameFeedback.attr("class")).to.not.contain("ok");
        });
        it("adds warning to feedback", function () {
            $lastnameFeedback.removeClass("warning");
            callRegistrationInputHandler($lastnameInputGroup);
            expect($lastnameFeedback.attr("class")).to.contain("warning");
        });
        it("removes ok from required", function () {
            $lastnameRequired.addClass("ok");
            callRegistrationInputHandler($lastnameInputGroup);
            expect($lastnameRequired.attr("class")).to.not.contain("ok");
        });
        it("adds warning to required", function () {
            $lastnameRequired.removeClass("warning");
            callRegistrationInputHandler($lastnameInputGroup);
            expect($lastnameRequired.attr("class")).to.contain("warning");
        });
    });
});

The email repetition tests are similar too:

describe("registration-input email", function () {
    /*
       Structure
       - .form-group
         - .starrq
         - .input-group
           - input
         - .inputstatus
           - .error
           - .feedback
    */
    function callRegistrationInputHandler(thisArg) {
        const registrationInputHandler = validate.eventHandler.registrationInput;
        registrationInputHandler.call(thisArg);
    }
    const $emailGroup = $(".form-group").has("[name='E-mail']");
    const $emailInputGroup = $emailGroup.find(".input-group");
    const $emailInput = $emailGroup.find("input");
    const $emailError = $emailGroup.find(".error");
    const $emailFeedback = $emailGroup.find(".feedback");
    const $emailRequired = $emailGroup.find(".starrq");
    describe("first name has repetition", function () {
        beforeEach(function () {
            $emailInput.val("abbbc");
        });
        it("shows an error message", function () {
            $emailError.html("");
            callRegistrationInputHandler($emailInputGroup);
            expect($emailError.html()).to.equal("E-mail is Fake text: Please remove repetition");
        });
        it("adds warning to error", function () {
            $emailError.removeClass("warning");
            callRegistrationInputHandler($emailInputGroup);
            expect($emailError.attr("class")).to.contain("warning");
        });
        it("removes ok from error", function () {
            $emailError.addClass("ok");
            callRegistrationInputHandler($emailInputGroup);
            expect($emailError.attr("class")).to.not.contain("ok");
        });
        it("adds glyphicon to feedback", function () {
            $emailFeedback.removeClass("glyphicon");
            callRegistrationInputHandler($emailInputGroup);
            expect($emailFeedback.attr("class")).to.contain("glyphicon");
        });
        it("removes glyphicon-ok from feedback", function () {
            $emailFeedback.addClass("glyphicon-ok");
            callRegistrationInputHandler($emailInputGroup);
            expect($emailFeedback.attr("class")).to.not.contain("glyphicon-ok");
        });
        it("adds glyphicon-remove to feedback", function () {
            $emailFeedback.removeClass("glyphicon-remove");
            callRegistrationInputHandler($emailInputGroup);
            expect($emailFeedback.attr("class")).to.contain("glyphicon-remove");
        });
        it("removes ok from feedback", function () {
            $emailFeedback.addClass("ok");
            callRegistrationInputHandler($emailInputGroup);
            expect($emailFeedback.attr("class")).to.not.contain("ok");
        });
        it("adds warning to feedback", function () {
            $emailFeedback.removeClass("warning");
            callRegistrationInputHandler($emailInputGroup);
            expect($emailFeedback.attr("class")).to.contain("warning");
        });
        it("removes ok from required", function () {
            $emailRequired.addClass("ok");
            callRegistrationInputHandler($emailInputGroup);
            expect($emailRequired.attr("class")).to.not.contain("ok");
        });
        it("adds warning to required", function () {
            $emailRequired.removeClass("warning");
            callRegistrationInputHandler($emailInputGroup);
            expect($emailRequired.attr("class")).to.contain("warning");
        });
    });
});

and the password repetition tests are no-less vital.

registration-input-password.test.js

describe("registration-input password", function () {
    /*
       Structure
       - .form-group
         - .starrq
         - .input-group
           - input
         - .inputstatus
           - .error
           - .feedback
    */
    function callRegistrationInputHandler(thisArg) {
        const registrationInputHandler = validate.eventHandler.registrationInput;
        registrationInputHandler.call(thisArg);
    }
    const $passwordGroup = $(".form-group").has("[name='Password']");
    const $passwordInputGroup = $passwordGroup.find(".input-group");
    const $passwordInput = $passwordGroup.find("input");
    const $passwordError = $passwordGroup.find(".error");
    const $passwordFeedback = $passwordGroup.find(".feedback");
    const $passwordRequired = $passwordGroup.find(".starrq");
    describe("password has repetition", function () {
        beforeEach(function () {
            $passwordInput.val("abbbc");
        });
        it("shows an error message", function () {
            $passwordError.html("");
            callRegistrationInputHandler($passwordInputGroup);
            expect($passwordError.html()).to.equal("Password is Fake text: Please remove repetition");
        });
        it("adds warning to error", function () {
            $passwordError.removeClass("warning");
            callRegistrationInputHandler($passwordInputGroup);
            expect($passwordError.attr("class")).to.contain("warning");
        });
        it("removes ok from error", function () {
            $passwordError.addClass("ok");
            callRegistrationInputHandler($passwordInputGroup);
            expect($passwordError.attr("class")).to.not.contain("ok");
        });
        it("adds glyphicon to feedback", function () {
            $passwordFeedback.removeClass("glyphicon");
            callRegistrationInputHandler($passwordInputGroup);
            expect($passwordFeedback.attr("class")).to.contain("glyphicon");
        });
        it("removes glyphicon-ok from feedback", function () {
            $passwordFeedback.addClass("glyphicon-ok");
            callRegistrationInputHandler($passwordInputGroup);
            expect($passwordFeedback.attr("class")).to.not.contain("glyphicon-ok");
        });
        it("adds glyphicon-remove to feedback", function () {
            $passwordFeedback.removeClass("glyphicon-remove");
            callRegistrationInputHandler($passwordInputGroup);
            expect($passwordFeedback.attr("class")).to.contain("glyphicon-remove");
        });
        it("removes ok from feedback", function () {
            $passwordFeedback.addClass("ok");
            callRegistrationInputHandler($passwordInputGroup);
            expect($passwordFeedback.attr("class")).to.not.contain("ok");
        });
        it("adds warning to feedback", function () {
            $passwordFeedback.removeClass("warning");
            callRegistrationInputHandler($passwordInputGroup);
            expect($passwordFeedback.attr("class")).to.contain("warning");
        });
        it("removes ok from required", function () {
            $passwordRequired.addClass("ok");
            callRegistrationInputHandler($passwordInputGroup);
            expect($passwordRequired.attr("class")).to.not.contain("ok");
        });
        it("adds warning to required", function () {
            $passwordRequired.removeClass("warning");
            callRegistrationInputHandler($passwordInputGroup);
            expect($passwordRequired.attr("class")).to.contain("warning");
        });
    });
});

Restructure the code?

I can see an easy restructure of the nested if/else code that will help out a lot, but I must resist that urge until all of the related code is under test. Only then is it safe to make such changes under the assurance that the tests will tell me when something goes wrong.

Remove duplication from the code

Removing duplication from the repetition code is as easy as using inputStatus.warning()

        } else {
            if (fakeReg.test(value)) {
                // $(this).next().find(".error").html(name + " is Fake text: Please remove repetition");
                // $(this).next().find(".feedback").removeClass("glyphicon glyphicon-ok").addClass("glyphicon glyphicon-remove").removeClass("ok").addClass("warning");
                // $(this).next().find(".error").addClass('warning').removeClass('ok');
                // $("#fnameRequired").removeClass("ok").addClass("warning");
                inputStatus.warning($(this).closest(".form-group"), name + " is Fake text: Please remove repetition");

We can make things even simpler by defining a $formGroup variable near the top of the function.

        var $form = $("form.register");
        var inputs = $form[0].elements;
        const $formGroup = $(this).closest(".form-group");

That way the repetition code becomes even simpler still.

        } else {
            if (fakeReg.test(value)) {
                inputStatus.warning($formGroup, name + " is Fake text: Please remove repetition");

The other sections about repetition are updated in the same way, and the tests are our assurance that those sections of code continue to work just as they did before.

Summary

We tidied up an annoying problem with trailing spaces, added tests for code that was going to be changed, and replaced duplicate code with inputStatus.warning().

The code as it stands today is found at v0.0.13 in releases

Next time we take a deeper look at how tests and the code work hand-in-hand to improve each other, with the login reset code.

[Paul_Wilkins]

Tests for removing duplication from the login and change-password sections is what we’re up to today. We will be doing the following:

• renaming some test files for improved flexibility
• taking an in-depth look at initial testing
• providing a more complete testing suite

The duplication found by jsInspect

Here is example #12 of duplication found by jsInspect in the code.

./registration3\change-password.js:78,82
$(".inputboxmodal2").each(function() {
    var st = $(this).find(".input-check").attr("name");
    var st2 = $(this).find(".input-check").val().trim();
    if ($(this).find(".input-check").val().trim() != "") {
        $(this).find(".error").html("Your " + st + " is OK");

./registration3\login.js:88,92
$(".inputboxmodal1").each(function() {
    var st = $(this).find(".input-check").attr("name");
    var st2 = $(this).find(".input-check").val().trim();
    if ($(this).find(".input-check").val().trim() != "") {
        $(this).find(".error").html("Your " + st);

One is in the login reset code, and the other is in the change-password reset code.

Rename some test files to improve our ability to add more tests

I don’t want the size of the test files to become very large, mainly because it’s a struggle for me to scroll up and down finding things in there.

Because we already have reset tests for the login and change-password forms, it makes sense to rename their test files to be submit ones. That way we can also have related reset test files there too.

<!--<script src="../tests/login.test.js"></script>-->
<script src="../tests/login-submit.test.js"></script>
<script src="../tests/login-reset.test.js"></script>
<!--<script src="../tests/change-password.test.js"></script>-->
<script src="../tests/change-password-submit.test.js"></script>
<script src="../tests/change-password-reset.test.js"></script>

The initial test for login reset

The first initial test just needs to check that some of the login reset code is running.

describe("login reset", function () {
    /*
       Structure
       - .form-group
         - input
         - .inputstatusmodal
           - .error
           - .feedback
    */
    function loginResetHandler() {
        const resetHandler = login.eventHandler.loginReset;
        $(".button1color").trigger("click");
    }
    const $emailGroup = $("#login .form-group").has(".input-check");
    const $emailInput = $emailGroup.find("input");
    const $emailError = $emailGroup.find(".error");
    describe("when email has value", function () {
        beforeEach(function () {
            $emailInput.val("test value");
        });
        it("Shows a message", function () {
            $emailError.html("");
            loginResetHandler();
            expect($emailError.html()).to.contain("Your E-mail");
        });
    });
});

Now that we have a reliable test for something in the login reset code, we can modify the structure of the reset handler to make it easier to test.

Extract the reset handler

My motivation here is to extract the reset handler out of the event assignment, so that we can more easily test that reset handler.

login.js

    function loginResetHandler() {
        $(".inputboxmodal1").each(function() {
            //...
        });
    }
    $(".button1color").click(loginResetHandler);

We now want the test to use that loginResetHandler function. Let’s access it via the login eventHandler object that we return from the login code:

    function loginResetHandler() {
        // $("#login").trigger("reset");
        const resetHandler = login.eventHandler.loginReset;
        resetHandler();
    }

That causes the test to fail because loginReset doesn’t exist. A failing test for a new capability is a good thing. We can now update the code to add that new thing:

login.js

    return {
        eventHandler: {
            loginSubmit: loginSubmitHandler,
            loginReset: loginResetHandler
        }

and the test goes back to passing.

That does though leave the click assignment untested. If we remove that line . . .

    // $(".button1color").click(loginResetHandler);

. . . the test still passes. That is bad news as we want a test to inform us if the reset event handler stops working.

We need another test to ensure that the event assignment works properly.

    it("resets the login form", function () {
        $emailError.html("");
        $(".button1color").trigger("click");
        expect($emailError.html()).to.contain("Your");
    });

That’s better. The test helps to ensure that the event assignment is done.

Improve the event assignment

My motivation for this next part is to make the form reset much move obvious in the code.

Here is the existing event assignment for resetting the login form.

    $(".button1color").click(loginResetHandler);

That .button1color doesn’t really tell us much, and a click event for resetting the form isn’t all that informative either.

We learn in the HTML code that .button1color is the reset button.

        <button type="reset" class="btn btn-default button1 button1color">Reset</button>

Instead of referring to .button1color, it’s much more informative to start from the form itself and refer to the reset event itself.

login.js

    // $(".button1color").click(loginResetHandler);
    $("#login [type=reset]").click(loginResetHandler);

Referring to the reset event is much more expressive way to help us understand what it does, than to instead click on a button.

An even more improved way to code that is to not worry about clicking on a button, and instead to refer to the reset event itself.

login.js

    // $("#login [type=reset]").click(loginResetHandler);
    $("#login").on("reset", loginResetHandler);

The tests still pass, letting us know that things still work properly there.

We can now improve the test so that it too also uses the form reset event.

    it("resets the login form", function () {
        $emailError.html("");
        // $(".button1color").trigger("click");
        $("#login").trigger("reset");
        expect($emailError.html()).to.contain("Your E-mail");
    });

This staggered process of improving the test to improve the code to impove the test to improve the code, is how significant improvements are made. Each step building on the other until the end result is much better than how it began.

Add more email tests

Now that we can easily test the login reset code, the rest of the tests for the email can be added.

describe("login reset", function () {
    /*
       Structure
       - .form-group
         - input
         - .inputstatusmodal
           - .error
           - .feedback
    */
    function loginResetHandler() {
        const resetHandler = login.eventHandler.loginReset;
        resetHandler();
    }
    const $emailGroup = $("#login .form-group").has("[name='E-mail']");
    const $emailInput = $emailGroup.find("input");
    const $emailError = $emailGroup.find(".error");
    const $emailFeedback = $emailGroup.find(".feedback");
    it("resets the login form", function () {
        $emailError.html("");
        $("#login").trigger("reset");
        expect($emailError.html()).to.contain("Your");
    });
    describe("when email has value", function () {
        beforeEach(function () {
            $emailInput.val("test value");
        });
        it("shows a message", function () {
            $emailError.html("");
            loginResetHandler();
            expect($emailError.html()).to.equal("Your E-mail");
        });
        it("sets the message color", function () {
            const CSSgreen = "rgb(0, 128, 0)";
            $emailError.css("color", "red");
            loginResetHandler();
            expect($emailError.css("color")).to.equal(CSSgreen);
        });
        it("removes warning from error", function () {
            $emailError.addClass("warning");
            loginResetHandler();
            expect($emailError.attr("class")).to.not.contain("warning");
        });
        it("adds ok to error", function () {
            $emailError.removeClass("ok");
            loginResetHandler();
            expect($emailError.attr("class")).to.contain("ok");
        });
        it("removes glyphicon from feedback", function () {
            $emailFeedback.addClass("glyphicon");
            loginResetHandler();
            expect($emailFeedback.attr("class")).to.not.contain("glyphicon");
        });
        it("removes glyphicon-ok from feedback", function () {
            $emailFeedback.addClass("glyphicon-ok");
            loginResetHandler();
            expect($emailFeedback.attr("class")).to.not.contain("glyphicon-ok");
        });
        it("removes ok from feedback", function () {
            $emailFeedback.addClass("ok");
            loginResetHandler();
            expect($emailFeedback.attr("class")).to.not.contain("ok");
        });
    });
    describe("when email is empty", function () {
        beforeEach(function () {
            $emailInput.val("");
        });
        it("shows a message", function () {
            $emailError.html("");
            loginResetHandler();
            expect($emailError.html()).to.equal("Your E-mail");
        });
        it("sets the message color", function () {
            const CSSgreen = "rgb(0, 128, 0)";
            $emailError.css("color", "red");
            loginResetHandler();
            expect($emailError.css("color")).to.equal(CSSgreen);
        });
        it("removes glyphicon from feedback", function () {
            $emailFeedback.addClass("glyphicon");
            loginResetHandler();
            expect($emailFeedback.attr("class")).to.not.contain("glyphicon");
        });
        it("removes glyphicon-remove from feedback", function () {
            $emailFeedback.addClass("glyphicon-remove");
            loginResetHandler();
            expect($emailFeedback.attr("class")).to.not.contain("glyphicon-remove");
        });
    });
});

Do I leave the tests with only the email and not the password section? No, that doesn’t make sense. At least one test for each of the other form fields should be done too.

Add password tests

As we now have both email and passwords tests to deal with, that means putting the email tests in their own describe block and having a separate describe block for the password test.

    describe("email", function () {
        const $emailGroup = $("#login .form-group").has("[name='E-mail']");
        //...
    });
    describe("password", function () {
        const $passwordGroup = $("#login .form-group").has("[name=Password");
        const $passwordInput = $passwordGroup.find("input");
        const $passwordError = $passwordGroup.find(".error");
        describe("when password has value", function () {
            beforeEach(function () {
                $passwordInput.val("test value");
            });
            it("Shows a message", function () {
                $passwordError.html("");
                loginResetHandler();
                expect($passwordError.html()).to.equal("Your Password");
            });
        });
    });

Do I add the rest of the password tests? It feels bad to leave them out, so password tests are included too.

    describe("password", function () {
        const $passwordGroup = $("#login .form-group").has("[name=Password]");
        const $passwordInput = $passwordGroup.find("input");
        const $passwordError = $passwordGroup.find(".error");
        const $passwordFeedback = $passwordGroup.find(".feedback");
        describe("when password has value", function () {
            beforeEach(function () {
                $passwordInput.val("test value");
            });
            it("shows a message", function () {
                $passwordError.html("");
                loginResetHandler();
                expect($passwordError.html()).to.equal("Your Password");
            });
            it("sets the message color", function () {
                const CSSgreen = "rgb(0, 128, 0)";
                $passwordError.css("color", "red");
                loginResetHandler();
                expect($passwordError.css("color")).to.equal(CSSgreen);
            });
            it("removes warning from error", function () {
                $passwordError.addClass("warning");
                loginResetHandler();
                expect($passwordError.attr("class")).to.not.contain("warning");
            });
            it("adds ok to error", function () {
                $passwordError.removeClass("ok");
                loginResetHandler();
                expect($passwordError.attr("class")).to.contain("ok");
            });
            it("removes glyphicon from feedback", function () {
                $passwordFeedback.addClass("glyphicon");
                loginResetHandler();
                expect($passwordFeedback.attr("class")).to.not.contain("glyphicon");
            });
            it("removes glyphicon-ok from feedback", function () {
                $passwordFeedback.addClass("glyphicon-ok");
                loginResetHandler();
                expect($passwordFeedback.attr("class")).to.not.contain("glyphicon-ok");
            });
            it("removes ok from feedback", function () {
                $passwordFeedback.addClass("ok");
                loginResetHandler();
                expect($passwordFeedback.attr("class")).to.not.contain("ok");
            });
        });
        describe("when password is empty", function () {
            beforeEach(function () {
                $passwordInput.val("");
            });
            it("shows a message", function () {
                $passwordError.html("");
                loginResetHandler();
                expect($passwordError.html()).to.equal("Your Password");
            });
            it("sets the message color", function () {
                const CSSgreen = "rgb(0, 128, 0)";
                $passwordError.css("color", "red");
                loginResetHandler();
                expect($passwordError.css("color")).to.equal(CSSgreen);
            });
            it("removes glyphicon from feedback", function () {
                $passwordFeedback.addClass("glyphicon");
                loginResetHandler();
                expect($passwordFeedback.attr("class")).to.not.contain("glyphicon");
            });
            it("removes glyphicon-remove from feedback", function () {
                $passwordFeedback.addClass("glyphicon-remove");
                loginResetHandler();
                expect($passwordFeedback.attr("class")).to.not.contain("glyphicon-remove");
            });
        });
    });

We now have a full suite of tests for the login reset behaviour, and can move on to removing duplication in the code. But that can be for next time.

Summary

We have renamed some test files to improve their flexibility, taken a more in-depth look at initial testing, and provided complete suite of tests for the login reset.

The code as it stands today is found at v0.0.14 in releases

Next time we will remove the duplication from the login reset code.

[Paul_Wilkins]

Removing duplication from … is what we’re up to today. We will be doing the following:

• getting live test results
• removing login reset duplication
• creating tests for reset-password submit code
• removing reset-password submit duplication

Auto-running tests

Now that we have tests covering everything that the login reset code is supposed to do, we can run them while making changes to the code.

I use live-server so that whenever a file changes, it reloads the webpage so that I get live updates on the changes that have been made.
While I’m working on the login reset section of code, I’m not all that interested in the other tests that we have for other sections. Mocha has a url grep command, that we can use to only run certain tests. In this case it’s those relating to the login reset.

http://127.0.0.1:8080/registration3/?grep=login%20reset

and we can easily use the browser console to do that, with:

> location.search="grep=login reset"

That causes all 23 login reset tests to run whenever I save a file. If I want to run all of the tests just in case something has an unwanted side-effect elsewhere I can go back a page to run all of the tests.

http://127.0.0.1:8080/registration3/

As a final touch, I have the web browser running fullscreen behind the code editor, with the code editor overlapping everything except for the browse console. That lets me easily save and see, to get immediate feedback about the results.

[Album] Autoloading code test layout

Remove duplication from login reset code

Here is the login reset code that we are updating:

    function loginResetHandler() {
        $(".inputboxmodal1").each(function() {
            var st = $(this).find(".input-check").attr("name");
            var st2 = $(this).find(".input-check").val().trim();
            if ($(this).find(".input-check").val().trim() != "") {
                $(this).find(".error").html("Your " + st);
                $(this).find(".error").css("color", "green");
                $(this).find(".error").removeClass("warning").addClass("ok");
                $(this).find(".feedback").removeClass("glyphicon glyphicon-ok ok");
            } else {
                $(this).find(".error").html("Your " + st);
                $(this).find(".error").css("color", "green");
                $(this).find(".feedback").removeClass("glyphicon glyphicon-remove");
            }
        });
    }

The inputStatus code can replace much of the code in the if statements.

            if ($(this).find(".input-check").val().trim() != "") {
                // $(this).find(".error").html("Your " + st);
                // $(this).find(".error").css("color", "green");
                // $(this).find(".error").removeClass("warning").addClass("ok");
                // $(this).find(".feedback").removeClass("glyphicon glyphicon-ok ok");
                inputStatus.ok(this, "Your " + st);
                inputStatus.feedbackNone(this);
            } else {
                // $(this).find(".error").html("Your " + st);
                // $(this).find(".error").css("color", "green");
                // $(this).find(".feedback").removeClass("glyphicon glyphicon-remove");
                inputStatus.ok(this, "Your " + st);
                inputStatus.feedbackNone(this);
            }

We now find that both parts of the if statement have the same code in them, and the tests are satisfied. Because both parts of the if statement are the same, we can remove the need for the if statement completely.

   function loginResetHandler() {
        $(".inputboxmodal1").each(function() {
            var st = $(this).find(".input-check").attr("name");
            var st2 = $(this).find(".input-check").val().trim();
            // if ($(this).find(".input-check").val().trim() != "") {
            //     inputStatus.ok(this, "Your " + st);
            //     inputStatus.feedbackNone(this);
            // } else {
            //     inputStatus.ok(this, "Your " + st);
            //     inputStatus.feedbackNone(this);
            // }
            inputStatus.ok(this, "Your " + st);
            inputStatus.feedbackNone(this);
        });
    }

Remove input value difference in the tests

Now that the code is confirmed as working without needing to worry about different input values, we can remove the value-based parts of tests too.

        // describe("when email has value", function () {
        //     beforeEach(function () {
        //         $emailInput.val("test value");
        //     });
            it("shows a message", function () {
                $emailError.html("");
                loginResetHandler();
                expect($emailError.html()).to.equal("Your E-mail");
            });
            //...
        // });
        // describe("when email is empty", function () {
        //     beforeEach(function () {
        //         $emailInput.val("");
        //     });
        //     it("shows a message", function () {
        //         $emailError.html("");
        //         loginResetHandler();
        //         expect($emailError.html()).to.equal("Your E-mail");
        //     });
        //...
            it("removes glyphicon-remove from feedback", function () {
                $emailFeedback.addClass("glyphicon-remove");
                loginResetHandler();
                expect($emailFeedback.attr("class")).to.not.contain("glyphicon-remove");
            });
        });

Give variables better names

The st variable may have been meant to refer to a string. Benefits are gained by using a more meaningful name of inputName. Also, the st2 variable doesn’t have any reason to be there so can be removed.

function loginResetHandler() {
        $(".inputboxmodal1").each(function() {
            // var st = $(this).find(".input-check").attr("name");
            // var st2 = $(this).find(".input-check").val().trim();
            var inputName = $(this).find(".input-check").attr("name");
            inputStatus.ok(this, "Your " + inputName);
            inputStatus.feedbackNone(this);
        });
    }

Improving form group selectors

The only other thing to tidy up with that code is the .inputboxmodal1 selector. It’s much clearer as to what’s going on when we know that it’s the login form group that we’re dealing with.

function loginResetHandler() {
        // $(".inputboxmodal1").each(function() {
        $("#login .form-group").each(function() {
            var inputName = $(this).find(".input-check").attr("name");
            inputStatus.ok(this, "Your " + inputName);
            inputStatus.feedbackNone(this);
        });
    }

Let other code benefit

While updating the login reset handler, I notice that the login submit handler can benefit from using the same technique for finding the form fields to check.

    function loginSubmitHandler(evt) {
        // $("#login .form-group").has(".input-check").each(function validateInput() {
        $("#login .form-group").each(function validateInput() {
        //     if (!$(this).find(".input-check").length) {
        //         return;
        //     }
            var inputName = $(this).find(".input-check").attr("name");

Repeat the tests and code improvement for change-password form

As there don’t seem to be other sets of code with the same if/else structure, we can repeat the process of the last couple of posts. That means creating tests for change password submit, and improving the code in the same way.

Let’s speed on through these because they’re the same as what we’ve just done. The tests are:

change-password-submit.test.js

describe("change password submit", function () {
    /*
       Structure
       - .form-group
         - input
         - .inputstatusmodal
           - .error
           - .feedback
    */
    const fakeEvt = {
        preventDefault: function fakeFunc() {}
    };
    function changePasswordSubmitHandler() {
        const SubmitHandler = changePassword.eventHandler.passwordSubmit;
        SubmitHandler(fakeEvt);
    }
    describe("email", function () {
        const $emailGroup = $("#changepw .form-group").has("[name='E-mail']");
        const $emailInput = $emailGroup.find("input");
        const $emailError = $emailGroup.find(".error");
        const $emailFeedback = $emailGroup.find(".feedback");
        describe("email has value", function () {
            beforeEach(function () {
                $emailInput.val("test value");
            });
            it("shows a message", function () {
                $emailError.html("");
                changePasswordSubmitHandler();
                expect($emailError.html()).to.equal("Your E-mail is OK");
            });
            it("sets the message color", function () {
                const CSSgreen = "rgb(0, 128, 0)";
                $emailError.css("color", "red");
                changePasswordSubmitHandler();
                expect($emailError.css("color")).to.equal(CSSgreen);
            });
            it("removes warning from error", function () {
                $emailError.addClass("warning");
                changePasswordSubmitHandler();
                expect($emailError.attr("class")).to.not.contain("warning");
            });
            it("adds ok to error", function () {
                $emailError.removeClass("ok");
                changePasswordSubmitHandler();
                expect($emailError.attr("class")).to.contain("ok");
            });
            it("adds glyphicon to feedback", function () {
                $emailFeedback.removeClass("glyphicon");
                changePasswordSubmitHandler();
                expect($emailFeedback.attr("class")).to.contain("glyphicon");
            });
            it("removes glyphicon-remove from feedback", function () {
                $emailFeedback.addClass("glyphicon-remove");
                changePasswordSubmitHandler();
                expect($emailFeedback.attr("class")).to.not.contain("glyphicon-remove");
            });
            it("removes warning from feedback", function () {
                $emailFeedback.addClass("warning");
                changePasswordSubmitHandler();
                expect($emailFeedback.attr("class")).to.not.contain("warning");
            });
            it("adds glyphicon-ok to feedback", function () {
                $emailFeedback.removeClass("glyphicon-ok");
                changePasswordSubmitHandler();
                expect($emailFeedback.attr("class")).to.contain("glyphicon-ok");
            });
            it("adds ok from feedback", function () {
                $emailFeedback.removeClass("ok");
                changePasswordSubmitHandler();
                expect($emailFeedback.attr("class")).to.contain("ok");
            });
        });
        describe("email is empty", function () {
            beforeEach(function () {
                $emailInput.val("");
            });
            it("uses submit event to submit the form", function () {
                $emailError.html("");
                $(".button1color2").trigger("click");
                expect($emailError.html()).to.contain("Your E-mail is empty");
            });        
            it("shows a message", function () {
                $emailError.html("");
                changePasswordSubmitHandler();
                expect($emailError.html()).to.equal("Your E-mail is empty");
            });
            it("sets the message color", function () {
                const CSSred = "rgb(255, 0, 0)";
                $emailError.css("color", "green");
                changePasswordSubmitHandler();
                expect($emailError.css("color")).to.equal(CSSred);
            });
            it("removes ok from error", function () {
                $emailError.addClass("ok");
                changePasswordSubmitHandler();
                expect($emailError.attr("class")).to.not.contain("ok");
            });
            it("adds warning to error", function () {
                $emailError.removeClass("warning");
                changePasswordSubmitHandler();
                expect($emailError.attr("class")).to.contain("warning");
            });
            it("adds glyphicon to feedback", function () {
                $emailFeedback.removeClass("glyphicon");
                changePasswordSubmitHandler();
                expect($emailFeedback.attr("class")).to.contain("glyphicon");
            });
            it("removes glyphicon-ok from feedback", function () {
                $emailFeedback.addClass("glyphicon-ok");
                changePasswordSubmitHandler();
                expect($emailFeedback.attr("class")).to.not.contain("glyphicon-ok");
            });
            it("removes ok from feedback", function () {
                $emailFeedback.addClass("ok");
                changePasswordSubmitHandler();
                expect($emailFeedback.attr("class")).to.not.contain("ok");
            });
            it("adds glyphicon-remove to feedback", function () {
                $emailFeedback.removeClass("glyphicon-remove");
                changePasswordSubmitHandler();
                expect($emailFeedback.attr("class")).to.contain("glyphicon-remove");
            });
            it("adds warning to feedback", function () {
                $emailFeedback.removeClass("warning");
                changePasswordSubmitHandler();
                expect($emailFeedback.attr("class")).to.contain("warning");
            });
        });
    });
    describe("password", function () {
        const $retypeGroup = $("#changepw .form-group").has("[name='Password']");
        const $passwordInput = $retypeGroup.find("input");
        const $passwordError = $retypeGroup.find(".error");
        const $passwordFeedback = $retypeGroup.find(".feedback");
        describe("password has value", function () {
            beforeEach(function () {
                $passwordInput.val("test value");
            });
            it("shows a message", function () {
                $passwordError.html("");
                changePasswordSubmitHandler();
                expect($passwordError.html()).to.equal("Your Password is OK");
            });
            it("sets the message color", function () {
                const CSSgreen = "rgb(0, 128, 0)";
                $passwordError.css("color", "red");
                changePasswordSubmitHandler();
                expect($passwordError.css("color")).to.equal(CSSgreen);
            });
            it("removes warning from error", function () {
                $passwordError.addClass("warning");
                changePasswordSubmitHandler();
                expect($passwordError.attr("class")).to.not.contain("warning");
            });
            it("adds ok to error", function () {
                $passwordError.removeClass("ok");
                changePasswordSubmitHandler();
                expect($passwordError.attr("class")).to.contain("ok");
            });
            it("adds glyphicon to feedback", function () {
                $passwordFeedback.removeClass("glyphicon");
                changePasswordSubmitHandler();
                expect($passwordFeedback.attr("class")).to.contain("glyphicon");
            });
            it("removes glyphicon-remove from feedback", function () {
                $passwordFeedback.addClass("glyphicon-remove");
                changePasswordSubmitHandler();
                expect($passwordFeedback.attr("class")).to.not.contain("glyphicon-remove");
            });
            it("removes warning from feedback", function () {
                $passwordFeedback.addClass("warning");
                changePasswordSubmitHandler();
                expect($passwordFeedback.attr("class")).to.not.contain("warning");
            });
            it("adds glyphicon-ok to feedback", function () {
                $passwordFeedback.removeClass("glyphicon-ok");
                changePasswordSubmitHandler();
                expect($passwordFeedback.attr("class")).to.contain("glyphicon-ok");
            });
            it("adds ok from feedback", function () {
                $passwordFeedback.removeClass("ok");
                changePasswordSubmitHandler();
                expect($passwordFeedback.attr("class")).to.contain("ok");
            });
        });
        describe("password is empty", function () {
            beforeEach(function () {
                $passwordInput.val("");
            });
            it("uses submit event to submit the form", function () {
                $passwordError.html("");
                $(".button1color2").trigger("click");
                expect($passwordError.html()).to.contain("Your Password is empty");
            });        
            it("shows a message", function () {
                $passwordError.html("");
                changePasswordSubmitHandler();
                expect($passwordError.html()).to.equal("Your Password is empty");
            });
            it("sets the message color", function () {
                const CSSred = "rgb(255, 0, 0)";
                $passwordError.css("color", "green");
                changePasswordSubmitHandler();
                expect($passwordError.css("color")).to.equal(CSSred);
            });
            it("removes ok from error", function () {
                $passwordError.addClass("ok");
                changePasswordSubmitHandler();
                expect($passwordError.attr("class")).to.not.contain("ok");
            });
            it("adds warning to error", function () {
                $passwordError.removeClass("warning");
                changePasswordSubmitHandler();
                expect($passwordError.attr("class")).to.contain("warning");
            });
            it("adds glyphicon to feedback", function () {
                $passwordFeedback.removeClass("glyphicon");
                changePasswordSubmitHandler();
                expect($passwordFeedback.attr("class")).to.contain("glyphicon");
            });
            it("removes glyphicon-ok from feedback", function () {
                $passwordFeedback.addClass("glyphicon-ok");
                changePasswordSubmitHandler();
                expect($passwordFeedback.attr("class")).to.not.contain("glyphicon-ok");
            });
            it("removes ok from feedback", function () {
                $passwordFeedback.addClass("ok");
                changePasswordSubmitHandler();
                expect($passwordFeedback.attr("class")).to.not.contain("ok");
            });
            it("adds glyphicon-remove to feedback", function () {
                $passwordFeedback.removeClass("glyphicon-remove");
                changePasswordSubmitHandler();
                expect($passwordFeedback.attr("class")).to.contain("glyphicon-remove");
            });
            it("adds warning to feedback", function () {
                $passwordFeedback.removeClass("warning");
                changePasswordSubmitHandler();
                expect($passwordFeedback.attr("class")).to.contain("warning");
            });
        });
    });
    describe("retype password", function () {
        const $retypeGroup = $("#changepw .form-group").has("[name='Password Retype']");
        const $retypeInput = $retypeGroup.find("input");
        const $retypeError = $retypeGroup.find(".error");
        const $retypeFeedback = $retypeGroup.find(".feedback");
        describe("retype has value", function () {
            beforeEach(function () {
                $retypeInput.val("test value");
            });
            it("shows a message", function () {
                $retypeError.html("");
                changePasswordSubmitHandler();
                expect($retypeError.html()).to.equal("Your Password Retype is OK");
            });
            it("sets the message color", function () {
                const CSSgreen = "rgb(0, 128, 0)";
                $retypeError.css("color", "red");
                changePasswordSubmitHandler();
                expect($retypeError.css("color")).to.equal(CSSgreen);
            });
            it("removes warning from error", function () {
                $retypeError.addClass("warning");
                changePasswordSubmitHandler();
                expect($retypeError.attr("class")).to.not.contain("warning");
            });
            it("adds ok to error", function () {
                $retypeError.removeClass("ok");
                changePasswordSubmitHandler();
                expect($retypeError.attr("class")).to.contain("ok");
            });
            it("adds glyphicon to feedback", function () {
                $retypeFeedback.removeClass("glyphicon");
                changePasswordSubmitHandler();
                expect($retypeFeedback.attr("class")).to.contain("glyphicon");
            });
            it("removes glyphicon-remove from feedback", function () {
                $retypeFeedback.addClass("glyphicon-remove");
                changePasswordSubmitHandler();
                expect($retypeFeedback.attr("class")).to.not.contain("glyphicon-remove");
            });
            it("removes warning from feedback", function () {
                $retypeFeedback.addClass("warning");
                changePasswordSubmitHandler();
                expect($retypeFeedback.attr("class")).to.not.contain("warning");
            });
            it("adds glyphicon-ok to feedback", function () {
                $retypeFeedback.removeClass("glyphicon-ok");
                changePasswordSubmitHandler();
                expect($retypeFeedback.attr("class")).to.contain("glyphicon-ok");
            });
            it("adds ok from feedback", function () {
                $retypeFeedback.removeClass("ok");
                changePasswordSubmitHandler();
                expect($retypeFeedback.attr("class")).to.contain("ok");
            });
        });
        describe("retype is empty", function () {
            beforeEach(function () {
                $retypeInput.val("");
            });
            it("uses submit event to submit the form", function () {
                $retypeError.html("");
                $(".button1color2").trigger("click");
                expect($retypeError.html()).to.contain("Your Password Retype is empty");
            });        
            it("shows a message", function () {
                $retypeError.html("");
                changePasswordSubmitHandler();
                expect($retypeError.html()).to.equal("Your Password Retype is empty");
            });
            it("sets the message color", function () {
                const CSSred = "rgb(255, 0, 0)";
                $retypeError.css("color", "green");
                changePasswordSubmitHandler();
                expect($retypeError.css("color")).to.equal(CSSred);
            });
            it("removes ok from error", function () {
                $retypeError.addClass("ok");
                changePasswordSubmitHandler();
                expect($retypeError.attr("class")).to.not.contain("ok");
            });
            it("adds warning to error", function () {
                $retypeError.removeClass("warning");
                changePasswordSubmitHandler();
                expect($retypeError.attr("class")).to.contain("warning");
            });
            it("adds glyphicon to feedback", function () {
                $retypeFeedback.removeClass("glyphicon");
                changePasswordSubmitHandler();
                expect($retypeFeedback.attr("class")).to.contain("glyphicon");
            });
            it("removes glyphicon-ok from feedback", function () {
                $retypeFeedback.addClass("glyphicon-ok");
                changePasswordSubmitHandler();
                expect($retypeFeedback.attr("class")).to.not.contain("glyphicon-ok");
            });
            it("removes ok from feedback", function () {
                $retypeFeedback.addClass("ok");
                changePasswordSubmitHandler();
                expect($retypeFeedback.attr("class")).to.not.contain("ok");
            });
            it("adds glyphicon-remove to feedback", function () {
                $retypeFeedback.removeClass("glyphicon-remove");
                changePasswordSubmitHandler();
                expect($retypeFeedback.attr("class")).to.contain("glyphicon-remove");
            });
            it("adds warning to feedback", function () {
                $retypeFeedback.removeClass("warning");
                changePasswordSubmitHandler();
                expect($retypeFeedback.attr("class")).to.contain("warning");
            });
        });
    });
});

and the updated login submit code is:

    function passwordSubmitHandler(evt) {
        $("#changepw .form-group").has(".input-check").each(function() {
            var trimmedValue = $(this).find(".input-check").val().trim();
            var inputName = $(this).find(".input-check").attr("name");
            if (trimmedValue === "") {
                evt.preventDefault();
                inputStatus.warning(this, "Your " + inputName + " is empty");
            } else {
                inputStatus.ok(this, "Your " + inputName + " is OK");
            }
        });
    }
    $(".button1color2").click(passwordSubmitHandler);

Summary

We have taken a closer look at getting live test results, removed login reset duplication, and added tests to reset-password submit so that we can remove duplication from there too.

The code as it stands today is found at v0.0.15 in releases

Next time, jsInspect has a big surprise in store for us.

[Paul_Wilkins]

Today, jsInspect finds duplication in code that we’ve already cleaned up! We will be doing the following:

• combining repeated code into one function
• remove duplication from start of login and change-password
• prevent a test from changing input button values
• have the tests reset forms after testing
• move tests out of index.html

Removing duplication reveals more duplication?

After having removed duplication from some functions, we find that those updated functions have duplication within themselves too.

change-password.js

    function passwordResetHandler() {
        $("#changepw .form-group").each(function() {
            var inputName = $(this).find(".input-check").attr("name");
            inputStatus.errorOk(this, "Your " + inputName);
            inputStatus.feedbackNone(this);
        });
    }

login.js

    function loginResetHandler() {
        $("#login .form-group").each(function() {
            var inputName = $(this).find(".input-check").attr("name");
            inputStatus.errorOk(this, "Your " + inputName);
            inputStatus.feedbackNone(this);
        });
    }

The only difference between those two sets of code is the name of the form. That becomes a good candidate for a function parameter. We can move that repeated code out to a separate function, called resetForm()

    function resetForm($form) {
        $form.find(".form-group").each(function() {
            var inputName = $(this).find(".input-check").attr("name");
            inputStatus.errorOk(this, "Your " + inputName);
            inputStatus.feedbackNone(this);
        });
    }

    function passwordResetHandler() {
        // $("#changepw .form-group").each(function() {
        //     var inputName = $(this).find(".input-check").attr("name");
        //     inputStatus.errorOk(this, "Your " + inputName);
        //     inputStatus.feedbackNone(this);
        // });
        resetForm($("#changepw"));
    }

    function loginResetHandler() {
        // $("#login .form-group").each(function() {
        //     var inputName = $(this).find(".input-check").attr("name");
        //     inputStatus.errorOk(this, "Your " + inputName);
        //     inputStatus.feedbackNone(this);
        // });
        resetForm($("#login"));
    }

However, we want to be able to access that resetForm function from either the login code or the reset-password code. Is is appropriate for inputStatus to reset an entire form? Let’s try it.

Reset form from inputStatus

It doesn’t seem suitable to create a separate module only for one function. The inputStatus module seems like a suitable place for now. If it seems that we should split things off from inputStatus later on, we can easily do that.

The only different thing between those two sets of code is the form identifier. In the function we can just take a reference to the form itself, and find the .form-group parts within it.

input-status.js

    function resetForm($form) {
        $form.find(".form-group").each(function() {
            var inputName = $(this).find(".input-check").attr("name");
            inputStatus.errorOk(this, "Your " + inputName);
            inputStatus.feedbackNone(this);
        });
    }
    //...
    return {
        resetForm
    };

We can now refer to that inputStatus.resetForm() function instead.

login.js

    function loginResetHandler() {
        // resetForm($("#login"));
        inputStatus.resetForm($("#login"));
    }
    $("#login").on("reset", loginResetHandler);

change-password.js

    function passwordResetHandler() {
        // resetForm($("#changepw"));
        inputStatus.resetForm($("#changepw"));
    }
    $("#changepw").on("reset", passwordResetHandler);

Preventing test side-effects

When running the tests I’ve seen that they leave unwanted values on the screen. That is something that should be dealt with, now that it’s been noticed.

Something in the tests is causing the [key]Submit[/key] and [key]Reset Form[/key] buttons to change their name to test value.

Using describe.only() to narrow the set of tests that run, I found the responsible test in a registration submit test. The test doesn’t need to be so broad that it affects all input elements. We can instead only set the inputs that have a check classname.

        it("doesn't call preventDefault when no fields have a warning", function () {
            chai.spy.on(fakeEvt, "preventDefault");
            // $(".form-group input").val("test value");
            $(".form-group input.check").val("test value");
            $(".form-group textarea").val("test value");
            $(".form-group .warning").removeClass("warning");
            $("#terms").prop("checked", true);
            registrationSubmitHandler(fakeEvt);
            expect(fakeEvt.preventDefault).to.not.have.been.called();
        });

I should though try to ensure that the tests clean up after themselves. The forms all have a reset event that does that, so after all of the tests have been run I can tell is to reset the form that it’s been working with.

describe("registration submit", function () {
    let fakeEvt;
    //...
    after(function () {
        $("#registration").trigger("reset");
    });
    describe("avoiding errors", function () {

Repeating that across all of the tests means that the form remains nice and clean even after all of our testing.

While we are looking at all of the tests, there is no plan to keep the tests in the index.html file, so I’d better do something about cleaning that up too.

Move tests out of index.html

The tests really shouldn’t be in the index.html file. Instead, I’ll put them in a separate file called tests/tests.html

As the html file is in a different location, we do need to rename some filepaths:

    <!--<link rel='stylesheet' href='lib/bootstrap.min.css'>-->
    <link rel='stylesheet' href='../registration3/lib/bootstrap.min.css'>
    <!--<link rel='stylesheet' href='lib/font-awesome.min.css'>-->
    <link rel='stylesheet' href='../registration3/lib/font-awesome.min.css'>
    <!--<link rel="stylesheet" href="./style.css">-->
    <link rel="stylesheet" href="../registration3/style.css">
<!-- ... -->
    <!--<script src="./lib/jquery.min.js"></script>-->
    <script src="../registration3/lib/jquery.min.js"></script>
    <!--<script src="./lib/bootstrap.min.js"></script>-->
    <script src="../registration3/lib/bootstrap.min.js"></script>
    <!--<script src="./setup.js"></script>-->
    <script src="../registration3/setup.js"></script>
    <!--<script src="./input-status.js"></script>-->
    <script src="../registration3/input-status.js"></script>
    <!--<script src="./validate.js"></script>-->
    <script src="../registration3/validate.js"></script>
    <!--<script src="./change-password.js"></script>-->
    <script src="../registration3/change-password.js"></script>
    <!--<script src="./login.js"></script>-->
    <script src="../registration3/login.js"></script>
<!-- ... -->
    <!--<script src="../node_modules/mocha/mocha.js"></script>-->
    <script src="../node_modules/mocha/mocha.js"></script>
    <!--<script src="../node_modules/chai/chai.js"></script>-->
    <script src="../node_modules/chai/chai.js"></script>
    <!--<script src="../node_modules/chai-spies/chai-spies.js"></script>-->
    <script src="../node_modules/chai-spies/chai-spies.js"></script>
<!-- ... -->
    <!--<script src="../tests/registration-input-firstname.test.js"></script>-->
    <script src="registration-input-firstname.test.js"></script>
    <!--<script src="../tests/registration-input-lastname.test.js"></script>-->
    <script src="registration-input-lastname.test.js"></script>
    <!--<script src="../tests/registration-input-email.test.js"></script>-->
    <script src="registration-input-email.test.js"></script>
    <!--<script src="../tests/registration-input-city.test.js"></script>-->
    <script src="registration-input-city.test.js"></script>
    <!--<script src="../tests/registration-input-password.test.js"></script>-->
    <script src="registration-input-password.test.js"></script>
    <!--<script src="../tests/registration-input-retypepassword.test.js"></script>-->
    <script src="registration-input-retypepassword.test.js"></script>
    <!--<script src="../tests/terms-click.test.js"></script>-->
    <script src="terms-click.test.js"></script>
    <!--<script src="../tests/registration-submit.test.js"></script>-->
    <script src="registration-submit.test.js"></script>
    <!--<script src="../tests/registration-reset.test.js"></script>-->
    <script src="registration-reset.test.js"></script>
    <!--<script src="../tests/login-reset.test.js"></script>-->
    <script src="login-reset.test.js"></script>
    <!--<script src="../tests/login-submit.test.js"></script>-->
    <script src="login-submit.test.js"></script>
    <!--<script src="../tests/change-password-reset.test.js"></script>-->
    <script src="change-password-reset.test.js"></script>
    <!--<script src="../tests/change-password-submit.test.js"></script>-->
    <script src="change-password-submit.test.js"></script>

The tests can now be removed from index.html, and that is back to normal.

We now use tests/tests.html to test the code, and registration3/index.html to run the actual web page.

A potential HTML duplication issue

There is still duplication in that the html code is duplicated across both files, but that’s not a problem as of yet.

Later on if changes happen to index.html there is a risk that tests.html gets out of date. To remedy that we can have tests.html copy over the HTML content from index.html, but that also tends to require running a local server.

I’m okay with putting off the complexity of that until another time when it actually becomes a problem, as things are better now than they were before.

Summary

We have combined repeated code into an inputStatus function to remove duplication login and change-password code, prevented a test from changing input button values, have the tests reset forms after testing, and have moved tests out of index.html into a separate tests/tests.html file instead.

The code as it stands today is found at v0.0.16 in releases

Next time jsInspect points us to duplication at the start of the login and change-password event handlers.

[Paul_Wilkins]

Removing duplication from the start of login and change-password input handlers, is what we’re up to today. We will be doing the following:

• adding tests for the login and change-password inputs
• improve the event assignment
• group event assignments together at the bottom
• move variables closer to where they are used
• remove dead comments and code

Duplication found at start of login and change-password code

The next set of duplication that’s reported to us by jsInspect is at the start of the login and change-password input handlers.

login.js

    $(".inputboxmodal1").on("focusin focusout input", function() {
        var inputattr = $(this).find(".input-check").attr("name");
        // Get the Login Name value and trim it
        var inputstr = $(this).find(".input-check").val().trim();
        var fakeReg = /(.)\1{2,}/;
        var emailReg = /^([\w-\.]+@([\w-]+\.)+[\w-]{2,4})?$/;
        var pswReglow = /^([a-zA-Z0-9]{6,})+$/;
        var pswRegheigh = /^([a-zA-Z0-9]{13,})+$/; //13 or more occurences
        if (inputstr != "") {

change-password.js

    $(".inputboxmodal2").on("focusin focusout input", function() {
        var inputattr = $(this).find(".input-check").attr("name");
        // Get the Login Name value and trim it
        var inputstr = $(this).find(".input-check").val().trim();
        var value = inputstr;
        var fakeReg = /(.)\1{2,}/;
        var emailReg = /^([\w-\.]+@([\w-]+\.)+[\w-]{2,4})?$/;
        var pswReglow = /^([a-zA-Z0-9]{6,})+$/;
        var pswRegheigh = /^([a-zA-Z0-9]{13,})+$/; //13 or more occurences
        var $form = $("form.modalresetpw");
        var inputs = $form[0].elements;
        if (inputstr != "") {

That duplication might be tricky to deal with, so let’s move on with the standard process and ensure that the code has tests.

Test everything that’s affected by the code

We need to create some tests for:

• login-input-email
• login-input-password
• change-password-input-email
• change-password-input-password
• change-password-input-retype

While creating those tests I cleaned up the formatting of some of the messages too.

The tests are mostly the same because the same type of behaviour is expected from all of them. The most detail is in the password tests, so here’s what the login-input-password tests look like:

describe("login input password", function () {
    /*
       Structure
       - .form-group
         - input
         - .inputstatusmodal
           - .error
           - .feedback
    */
    const $passwordGroup = $("#login .form-group").has("[name=Password]");
    const $passwordInput = $passwordGroup.find("input");
    const $passwordError = $passwordGroup.find(".error");
    const $passwordFeedback = $passwordGroup.find(".feedback");
    function loginInputHandler() {
        const inputHandler = login.eventHandler.loginInput;
        const thisArg = $passwordGroup.get(0);
        inputHandler.call(thisArg);
    }
    after(function () {
        $("#login").trigger("reset");
    });
    describe("password is fake", function () {
        beforeEach(function () {
            $passwordInput.val("aaabbb");
        })
        it("shows a message", function () {
            $passwordError.html("");
            loginInputHandler($passwordGroup);
            expect($passwordError.html()).to.equal("Password is Fake text: Please remove repetition");
        });
        it("adds warning to error", function () {
            $passwordError.removeClass("warning");
            loginInputHandler($passwordGroup);
            expect($passwordError.attr("class")).to.contain("warning");
        });
        it("removes ok from error", function () {
            $passwordError.addClass("ok");
            loginInputHandler($passwordGroup);
            expect($passwordError.attr("class")).to.not.contain("ok");
        });
        it("removes glyphicon-ok from feedback", function () {
            $passwordFeedback.addClass("glyphicon-ok");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.not.contain("glyphicon-ok");
        });
        it("adds glyphicon to feedback", function () {
            $passwordFeedback.removeClass("glyphicon");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.contain("glyphicon");
        });
        it("adds glyphicon-remove to feedback", function () {
            $passwordFeedback.removeClass("glyphicon-remove");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.contain("glyphicon-remove");
        });
        it("removes ok from feedback", function () {
            $passwordFeedback.addClass("ok");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.not.contain("ok");
        });
        it("adds warning to feedback", function () {
            $passwordFeedback.removeClass("warning");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.contain("warning");
        });
    });
    describe("password is too small", function () {
        beforeEach(function () {
            $passwordInput.val("ab");
        })
        it("shows a message", function () {
            $passwordError.html("");
            loginInputHandler($passwordGroup);
            expect($passwordError.html()).to.equal("Password is Incorrect: Please enter at least 6 characters");
        });
        it("adds warning to error", function () {
            $passwordError.removeClass("warning");
            loginInputHandler($passwordGroup);
            expect($passwordError.attr("class")).to.contain("warning");
        });
        it("removes ok from error", function () {
            $passwordError.addClass("ok");
            loginInputHandler($passwordGroup);
            expect($passwordError.attr("class")).to.not.contain("ok");
        });
        it("removes glyphicon-ok from feedback", function () {
            $passwordFeedback.addClass("glyphicon-ok");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.not.contain("glyphicon-ok");
        });
        it("adds glyphicon to feedback", function () {
            $passwordFeedback.removeClass("glyphicon");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.contain("glyphicon");
        });
        it("adds glyphicon-remove to feedback", function () {
            $passwordFeedback.removeClass("glyphicon-remove");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.contain("glyphicon-remove");
        });
        it("removes ok from feedback", function () {
            $passwordFeedback.addClass("ok");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.not.contain("ok");
        });
        it("adds warning to feedback", function () {
            $passwordFeedback.removeClass("warning");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.contain("warning");
        });
    });
    describe("password is valid", function () {
        beforeEach(function () {
            $passwordInput.val("testpassword");
        })
        it("shows a message", function () {
            $passwordError.html("");
            loginInputHandler($passwordGroup);
            expect($passwordError.html()).to.equal("Password is OK: Your data has been entered correctly");
        });
        it("adds ok to error", function () {
            $passwordError.removeClass("ok");
            loginInputHandler($passwordGroup);
            expect($passwordError.attr("class")).to.contain("ok");
        });
        it("removes warning from error", function () {
            $passwordError.addClass("warning");
            loginInputHandler($passwordGroup);
            expect($passwordError.attr("class")).to.not.contain("warning");
        });
        it("removes glyphicon-remove from feedback", function () {
            $passwordFeedback.addClass("glyphicon-remove");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.not.contain("glyphicon-remove");
        });
        it("adds glyphicon to feedback", function () {
            $passwordFeedback.removeClass("glyphicon");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.contain("glyphicon");
        });
        it("adds glyphicon-ok to feedback", function () {
            $passwordFeedback.removeClass("glyphicon-ok");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.contain("glyphicon-ok");
        });
        it("removes warning from feedback", function () {
            $passwordFeedback.addClass("warning");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.not.contain("warning");
        });
        it("adds ok to feedback", function () {
            $passwordFeedback.removeClass("ok");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.contain("ok");
        });
    });
    describe("password is too large", function () {
        beforeEach(function () {
            $passwordInput.val("toolongforapassword");
        })
        it("shows a message", function () {
            $passwordError.html("");
            loginInputHandler($passwordGroup);
            expect($passwordError.html()).to.equal("Password is Incorrect: Please enter no more than 12 characters");
        });
        it("adds warning to error", function () {
            $passwordError.removeClass("warning");
            loginInputHandler($passwordGroup);
            expect($passwordError.attr("class")).to.contain("warning");
        });
        it("removes ok from error", function () {
            $passwordError.addClass("ok");
            loginInputHandler($passwordGroup);
            expect($passwordError.attr("class")).to.not.contain("ok");
        });
        it("removes glyphicon-ok from feedback", function () {
            $passwordFeedback.addClass("glyphicon-ok");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.not.contain("glyphicon-ok");
        });
        it("adds glyphicon to feedback", function () {
            $passwordFeedback.removeClass("glyphicon");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.contain("glyphicon");
        });
        it("adds glyphicon-remove to feedback", function () {
            $passwordFeedback.removeClass("glyphicon-remove");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.contain("glyphicon-remove");
        });
        it("removes ok from feedback", function () {
            $passwordFeedback.addClass("ok");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.not.contain("ok");
        });
        it("adds warning to feedback", function () {
            $passwordFeedback.removeClass("warning");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.contain("warning");
        });
    });
    describe("password is empty", function () {
        beforeEach(function () {
            $passwordInput.val("");
        })
        it("shows a message", function () {
            $passwordError.html("");
            loginInputHandler($passwordGroup);
            expect($passwordError.html()).to.equal("Password is empty");
        });
        it("adds warning to error", function () {
            $passwordError.removeClass("warning");
            loginInputHandler($passwordGroup);
            expect($passwordError.attr("class")).to.contain("warning");
        });
        it("removes ok from error", function () {
            $passwordError.addClass("ok");
            loginInputHandler($passwordGroup);
            expect($passwordError.attr("class")).to.not.contain("ok");
        });
        it("removes glyphicon-ok from feedback", function () {
            $passwordFeedback.addClass("glyphicon-ok");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.not.contain("glyphicon-ok");
        });
        it("adds glyphicon to feedback", function () {
            $passwordFeedback.removeClass("glyphicon");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.contain("glyphicon");
        });
        it("adds glyphicon-remove to feedback", function () {
            $passwordFeedback.removeClass("glyphicon-remove");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.contain("glyphicon-remove");
        });
        it("removes ok from feedback", function () {
            $passwordFeedback.addClass("ok");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.not.contain("ok");
        });
        it("adds warning to feedback", function () {
            $passwordFeedback.removeClass("warning");
            loginInputHandler($passwordGroup);
            expect($passwordFeedback.attr("class")).to.contain("warning");
        });
    });
});

Improve event assignment

The login and changepassword input events can use the same improvement that we’ve made to other parts of the event assignment.

login.js

    // $(".inputboxmodal1").on("focusin focusout input", loginInputHandler);
    $("#login .form-group").on("focusin focusout input", loginInputHandler);

change-password.js

    // $(".inputboxmodal2").on("focusin focusout input", passwordInputHandler);
    $("#changepw .form-group").on("focusin focusout input", passwordInputHandler);

Group together event assignments

Now that all of the event assignments are being assigned in the same way, we can move them all together at the end of the code.

login.js

    $("#login .form-group").on("focusin focusout input", loginInputHandler);
    $("#login").on("submit", loginSubmitHandler);
    $("#login").on("reset", loginResetHandler);
    return {
        eventHandler: {

change-password.js

    $("#changepw .form-group").on("focusin focusout input", passwordInputHandler);
    $("#changepw").on("submit", passwordSubmitHandler);
    $("#changepw").on("reset", passwordResetHandler);
    return {
        eventHandler: {

Make improvements to the login input code

Here’s the start of the login input code

        var inputattr = $(this).find(".input-check").attr("name");
        // Get the Login Name value and trim it
        var inputstr = $(this).find(".input-check").val().trim();
        var fakeReg = /(.)\1{2,}/;
        var emailReg = /^([\w-\.]+@([\w-]+\.)+[\w-]{2,4})?$/;
        var pswReglow = /^([a-zA-Z0-9]{6,})+$/;
        var pswRegheigh = /^([a-zA-Z0-9]{13,})+$/; //13 or more occurences
        if (inputstr != "") {

Improve the input-check

The find of input-check ends up finding the first input. But that’s not what the comment tells us we intend to do. Instead we are to specifically get the one for the login name.

We don’t have a login name. Instead, it is an email address or password. We could rename the comment to remove the login name part:

        var inputattr = $(this).find(".input-check").attr("name");
        // Get the value and trim it
        var inputstr = $(this).find(".input-check").val().trim();

But then that comment is useless, as it’s easy to see that we are getting the value and trim it. This type of comment where it tells you exactly what the code is doing is superfluous, and should be removed.

        var inputattr = $(this).find(".input-check").attr("name");
        var inputstr = $(this).find(".input-check").val().trim();

Move variables closer to where they’re used

Several of the defined variables aren’t used anywhere else than at specific locations. We can use a refactoring technique called push down field to move those assignments to better locations instead.

    function loginInputHandler() {
        var inputattr = $(this).find(".input-check").attr("name");
        var inputstr = $(this).find(".input-check").val().trim();
//        var fakeReg = /(.)\1{2,}/;
//        var emailReg = /^([\w-\.]+@([\w-]+\.)+[\w-]{2,4})?$/;
//        var pswReglow = /^([a-zA-Z0-9]{6,})+$/;
//        var pswRegheigh = /^([a-zA-Z0-9]{13,})+$/; //13 or more occurences
        if (inputstr != "") {
            var fakeReg = /(.)\1{2,}/;
            if (fakeReg.test(inputstr)) {
                //...
            } else {
                if (inputattr === "E-mail") {
                    var emailReg = /^([\w-\.]+@([\w-]+\.)+[\w-]{2,4})?$/;
                    if (emailReg.test(inputstr)) {
                        //...
                    }
                }
                if (inputattr === "Password") {
                    var pswReglow = /^([a-zA-Z0-9]{6,})+$/;
                    var pswRegheigh = /^([a-zA-Z0-9]{13,})+$/;

Remove dead code

When putting together the tests, I found that it was impossible to test some of the code. That’s because the effects of the code are completely undone by other code that runs later on.

login.js

        if (inputstr != "") {
            $(this).find(".error").html(inputattr + '  is ok').removeClass("warning").addClass("ok");
            $(this).find(".feedback").removeClass("glyphicon glyphicon-remove").addClass("glyphicon glyphicon-ok").removeClass("warning").addClass("ok");
            if (fakeReg.test(inputstr)) {

We can apply another refactoring technique here where we remove dead code.

        if (inputstr != "") {
            // $(this).find(".error").html(inputattr + '  is ok').removeClass("warning").addClass("ok");
            // $(this).find(".feedback").removeClass("glyphicon glyphicon-remove").addClass("glyphicon glyphicon-ok").removeClass("warning").addClass("ok");
            if (fakeReg.test(inputstr)) {

I don’t mean just commenting out the code either. All of the comments that I use in these code examples are to show the difference between before and after.

The tests all still work perfectly when that code is commented out, so it can be completely removed.

        if (inputstr != "") {
            if (fakeReg.test(inputstr)) {

There’s other dead code in the password section too, where the fake test code is duplicated. That duplicate can be removed too.

We can now more easily improve the code structure, but that will be for the next post.

Summary

We added some tests for login and change-password inputs, improved the event assignment, grouped event assignments together at the bottom, moved variables closer to where they are used, and removed dead comments and code.

The code as it stands today is found at v0.0.17 in releases

Next time we make some quite severe structural improvements to the login and change-password input handlers.

[Paul_Wilkins]

Removing duplication from the login and change-password input handler is what we’re up to today. We will be doing the following:

• improve the password regex’s
• simplify the if/else code
• use inputStatus to remove some duplication
• prepare for major structural change
• push empty string and fake text checks down to email and password
• realise the need for a validate module
• rename existing validate.js to registration.js
• take a first glance at what validate() might do

Improve the password low and high regex’s

The test for a low password looks to be incorrect, because we are testing for more than 6 characters. I’ll update that regex so that it’s checking for less than 6 characters instead.

The password high test doesn’t need the plus on the end either, so that gets removed and cleaned up too.

        if (inputattr === "Password") {
            // var pswReglow = /^([a-zA-Z0-9]{6,})+$/;
            var pswReglow = /^([a-zA-Z0-9]{0,5})$/;
            // var pswRegheigh = /^([a-zA-Z0-9]{13,})+$/;
            var pswRegheigh = /^([a-zA-Z0-9]{13,})$/;
            if (fakeReg.test(inputstr)) {
                //...
            } else {
                // if (!pswReglow.test(inputstr)) {
                if (pswReglow.test(inputstr)) {
                    //...
                } else {
                    if (!pswRegheigh.test(inputstr)) {
                        //...
                    } else {
                        //...
                    }
                }

            }
        }

If/else simplification

Another improvement that I’ve made to the code is to simplify conditional statements to reduce the overall amount of nested if/else/if/else code.

Because we have a good set of tests that exercise everything that the code is supposed to do, it’s really easy to refactor the code because we get immediate feedback when something we do causes tests to fail.

Here’s the structure before it gets simplified:

        if (inputstr != "") {
            //...
            if (fakeReg.test(inputstr)) {
                //...
            } else {
                if (inputattr === "E-mail") {
                    //...
                    if (emailReg.test(inputstr)) {
                        //...
                    } else {
                        //...
                    }
                }
                if (inputattr === "Password") {
                    //...
                    if (fakeReg.test(inputstr)) {
                        //...
                    } else {
                        if (pswReglow.test(inputstr)) {
                            //...
                        } else {
                            if (pswRegheigh.test(inputstr)) {
                                //...
                            } else {
                                //...
                            }
                        }
                    }
                }
            }
        } else {
            //...
        }
    }

We now have a much simpler if/else structure, where empty values and fake text are checked, before dividing up into email and password tests.

        if (inputstr === "") {
            //...
        } else if (fakeReg.test(inputstr)) {
            //...
        } else if (inputattr === "E-mail") {
            //...
            if (!emailReg.test(inputstr)) {
                //...
            } else {
                //...
        } else if (inputattr === "Password") {
            //...
            if (pswReglow.test(inputstr)) {
                //...
            } else if (pswRegheigh.test(inputstr)) {
                //...
            } else {
                //...
            }
        }

Remove duplication from the code

This is a good time to remove duplication from the code, by using inputStatus ok and warning methods.

Here is an example with the fake text section:

        } else if (fakeReg.test(inputstr)) {
            // $(this).find(".error").html(inputattr + " is Fake text: Please remove repetition");
            // $(this).find(".error").addClass('warning').removeClass('ok');
            // $(this).find(".feedback").removeClass("glyphicon glyphicon-ok").addClass("glyphicon glyphicon-remove").removeClass("ok").addClass("warning");
            inputStatus.warning(this, inputattr + " is Fake text: Please remove repetition");

All of the other sections are updated in the same way, and we end up with the much reduced code:

        if (inputstr === "") {
            inputStatus.warning(this, inputattr + " is empty");
        } else if (fakeReg.test(inputstr)) {
            inputStatus.warning(this, inputattr + " is Fake text: Please remove repetition");
        } else if (inputattr === "E-mail") {
            var emailReg = /^([\w-\.]+@([\w-]+\.)+[\w-]{2,4})?$/;
            if (!emailReg.test(inputstr)) {
                inputStatus.warning(this, inputattr + " is Incorrect: Please enter it correctly");
            } else {
                inputStatus.ok(this, inputattr + " is Ok: Your data has been entered correctly");
            }
        } else if (inputattr === "Password") {
            var pswReglow = /^([a-zA-Z0-9]{0,5})$/;
            var pswRegheigh = /^([a-zA-Z0-9]{13,})$/;
            if (pswReglow.test(inputstr)) {
                inputStatus.warning(this, inputattr + " is Incorrect: Please enter at least 6 characters");
            } else if (pswRegheigh.test(inputstr)) {
                inputStatus.warning(this, inputattr + " is Incorrect: Please enter no more than 12 characters");
            } else {
                inputStatus.ok(this, inputattr + " is OK: Your data has been entered correctly");
            }
        }

Duplication is still there

Checking jsInspect we are still told that there’s duplication in the login and change-password input handlers. After the work we’ve done on them, it’s easier than ever to see that duplication.

We don’t really have any option anymore but to remove the duplication. That means, putting the email checking part into a separate function, and do the same with the password checking too.

To achieve that, we want the checks in the email and password sections to be rather self-contained. More simplification needs now to occur.

Remove if/else structure

To make things easier for us, we should get rid of the else/if structure, at least for now, and return out of the function on resolving a validation.

Fortunately that’s as easy as adding return to the start of the inputStatus lines.

        if (inputstr === "") {
            // inputStatus.warning(this, inputattr + " is empty");
            return inputStatus.warning(this, inputattr + " is empty");
        // } else if (fakeReg.test(inputstr)) {
        }
        if (fakeReg.test(inputstr)) {
            // inputStatus.warning(this, inputattr + " is Fake text: Please remove repetition");
            return inputStatus.warning(this, inputattr + " is Fake text: Please remove repetition");
        //...

Push down the empty string and fake text sections

When we validate the email and validate the password, it’s better if those sections each individually care about the empty value and the fake text, so I’m pushing those sections down into the email and password areas.

This is also when I use const instead of var, to have better control over the scope of the variables.

        const inputattr = $(this).find(".input-check").attr("name");
        // if (inputstr === "") {
        //     return inputStatus.warning(this, inputattr + " is empty");
        // }
        // const fakeReg = /(.)\1{2,}/;
        // if (fakeReg.test(inputstr)) {
        //     return inputStatus.warning(this, inputattr + " is Fake text: Please remove repetition");
        // }
        if (inputattr === "E-mail") {
            const inputstr = $(this).find(".input-check").val().trim();
            if (inputstr === "") {
                return inputStatus.warning(this, inputattr + " is empty");
            }
            const fakeReg = /(.)\1{2,}/;
            if (fakeReg.test(inputstr)) {
                return inputStatus.warning(this, inputattr + " is Fake text: Please remove repetition");
            }

We need a validate module

This is now a good time to move code out to a separate validate module. The plan that I have is to make a single function call, such as:

validate(this, "email");

Which internally runs several validations for [“hasValue”, “notFake”, “noRepetition”, “isEmail”]

To easily achieve that, we should add those hasValue and other parts to a validate function a piece at a time.

There is a problem though. We already have some code in a file called validate already.

Rename validate.js

The code in the validate file is actually for the registration form. We can call it registration.js instead.

<!--<script src="../registration3/validate.js"></script>-->
<script src="../registration3/registration.js"></script>
<script src="../registration3/change-password.js"></script>
<script src="../registration3/login.js"></script>

To avoid confusion though, we should rename anything relating to validate, to registration instead.

Rename validation module to registration

The first line of the registration file is still validate, so we rename that part.

// const validate = (function() {
const registration = (function() {

That breaks several tests, so we fix those up.

Fortunately, each test file has the reference in only one place, making it really easy to fix.

    function callRegistrationInputHandler(thisArg) {
        // const registrationInputHandler = validate.eventHandler.registrationInput;
        const registrationInputHandler = registration.eventHandler.registrationInput;
        registrationInputHandler.call(thisArg);
    }

and with one change like that to each test file, the rename is easily completed.

Create validate file

As the validate file will be used to determine the input status, we will load it between input-status and registration.

<script src="../registration3/input-status.js"></script>
<script src="../registration3/validate.js"></script>
<script src="../registration3/registration.js"></script>

At a rough guess, the validation code might look something like this:

function validate(formGroup, type) {
    validationTypes = {
        email: [hasValue, notFake, noRepetition, isEmail]
    };
    if (!validationTypes[type]) {
        throw new Error(type + " validation not supported");
    }
    return validateByTypes(formGroup, validationTypes);
}

That way something like validate(this, “email”) might actually work.

That gives us plenty of room to grow to cater for other types of validation too.

As that looks to be a rather large project though, it will have to wait until the next post.

Summary

We improved the password regex code, simplifed the if/else structure, used inputStatus to remove some duplication, prepared for major structural change, pushed empty string and fake text checks down to email and password, realised the need for a validate module, renamed the existing validate.js to registration.js, and took a first glance at what validate() might do.

The code as it stands today is found at v0.0.18 in releases

Next time we create that validate function.

[Paul_Wilkins]

Removing duplication from login and change-password input handlers is what we’re up to today. We will be doing the following:

• Sheer Sorcery! There’s no other explanation!

Extract a check empty function

Let’s create the functions that we’ll need in place, so that it’s easier to move them across to the validate function.

It will make things easier for us when each validation function has a consistent behaviour, of returning true when it’s found a problem, for example.

I was going to call the function isEmpty(), but that type of name implies that it’s just a true/false return value with no other effects.

In this case we are also showing a warning if it’s false, so calling it isEmpty would lead to misunderstandings. A better name for it is checkEmpty instead.

function checkEmpty(inputGroup) {
    const name = $(inputGroup).find(".input-check").attr("name");
    const value = $(inputGroup).find(".input-check").val().trim();
    if (value === "") {
        inputStatus.warning(inputGroup, name + " is empty");
        return true;
    }
}
//...
            // const inputstr = $(this).find(".input-check").val().trim();
            // if (inputstr === "") {
            //     return inputStatus.warning(this, inputattr + " is empty");
            // }
            if (checkEmpty(this)) {
                return;
            }

Having name and value defined in the function is a bit clumsy for now. They can be adjusted when the function is moved into the validate function. What we we waiting for? Let’s do that now, and get the clumsiness out of the way.

The validate function

Here is the updated validate function, with checkEmpty as a validation criteria.

validate.js

function validate(inputGroup) {
    const validationTypes = {
        "E-mail": [checkEmpty]
    };

    function getValue(inputGroup) {
        return $(inputGroup).find(".input-check").val().trim();
    }
    function getName(inputGroup) {
        return $(inputGroup).find(".input-check").attr("name");        
    }
    function checkEmpty(inputGroup) {
        if (getValue(inputGroup) === "") {
            inputStatus.warning(inputGroup, getName(inputGroup) + " is empty");
            return true;
        }
    }
    function validateByTypes(inputGroup, types) {
        return types.some(function (check) {
            return check(inputGroup);
        });
    }
    const name = getName(inputGroup);
    if (!validationTypes[name]) {
        throw new Error(name + " validation not supported");
    }
    return validateByTypes(inputGroup, validationTypes[name]);
}

The parts of the validation function are quite simple. A list of the tests for different validation types, getValue and getName helpers, then validation functions, followed by validateByTypes which does the check itself.

In the validateByTypes function, the types.some command runs each function in the array until it gets a true value. See Array.some.

We can now use the validate function from the login input code:

login.js

        if (inputattr === "E-mail") {
            if (validate(this)) {
                return;
            }
            const inputstr = $(this).find(".input-check").val().trim();
            //...

We have the beginnings of a validate command. We just need to add more details to it for the emails.

Check for fake

The next email validation to work on is checking for fake text.

In the validate function we’ve made things a bit easier, by having name and value always available via scope. That helps to keep the functions easier to manage.

The checkFake function that gets added to the validate function is:

    function checkFake(inputGroup) {
        const fakeReg = /(.)\1{2,}/;
        if (fakeReg.test(value)) {
            inputStatus.warning(inputGroup, getName(inputGroup) + " is Fake text: Please remove repetition");
            return true;
        }
    }

The first time I tried, I forgot to rename the this keyword to input instead. But the tests quickly caught that problem and helped me to understand the issue.

The checkFake function gets added to the email validation:

    const validationTypes = {
        // email: [checkEmpty]
        "E-mail": [checkEmpty, checkFake]
    };

The tests all pass telling us that we’re on the right track, so we can remove that part from the login code:

            if (validate(this)) {
                return;
            }
            const inputstr = $(this).find(".input-check").val().trim();
            // const fakeReg = /(.)\1{2,}/;
            // if (fakeReg.test(inputstr)) {
            //     return inputStatus.warning(this, inputattr + " is Fake text: Please remove repetition");
            // }

And the tests are all happy with our process.

This is easy! And it’s all thanks to having a good structure on which to build.

Check email regular expression

The email regular expression is next. We add a checkEmailReg function to the validate code:

    function checkEmailReg(inputGroup) {
        const emailReg = /^([\w-\.]+@([\w-]+\.)+[\w-]{2,4})?$/;
        const value = getValue(inputGroup);
        if (!emailReg.test(value)) {
            inputStatus.warning(inputGroup, getName(inputGroup) + " is Incorrect: Please enter it correctly");
            return true;
        }
    }

It gets added to the email validation:

    const validationTypes = {
        // "E-mail": [checkEmpty, checkFake]
        "E-mail": [checkEmpty, checkFake, checkEmailReg]
    };

And the tests have a problem, saying that inputStr is not defined.

Silly me, I should have renamed that to be value instead.

    function checkEmailReg(inputGroup) {
        const emailReg = /^([\w-\.]+@([\w-]+\.)+[\w-]{2,4})?$/;
        const value = getValue(inputGroup);
        // if (!emailReg.test(inputstr)) {
        if (!emailReg.test(value)) {
            inputStatus.warning(input, name + " is Incorrect: Please enter it correctly");
            return true;
        }
    }

The problem was easy to find, easy to fix, and the tests are now all happy.

We can then remove the emailReg part of the login code:

        if (inputattr === "E-mail") {
            if (validate(this)) {
                return;
            }
            const inputstr = $(this).find(".input-check").val().trim();
            // const emailReg = /^([\w-\.]+@([\w-]+\.)+[\w-]{2,4})?$/;
            // if (!emailReg.test(inputstr)) {
            //     return inputStatus.warning(this, inputattr + " is Incorrect: Please enter it correctly");
            // }
            return inputStatus.ok(this, inputattr + " is Ok: Your data has been entered correctly");
        }

We can now pirouette on our merry way to the last thing to do for email validation, the success message.

Validate a success

When the validations all fail to find a problem, we should then show that things are valid. We can use a showValid() function to do that.

    function showValid(inputGroup) {
        inputStatus.ok(inputGroup, getName(inputGroup) + " is Ok: Your data has been entered correctly");
    }

There are a number of fancy ways to have the success occur after validations find nothing, but let’s start easy. We assign the result of validation to an isNotValid variable. That will be truthy if a problem was found, so we can check for that and return it.

    const isNotValid = validateByTypes(inputGroup, validationTypes[name]);
    if (isNotValid) {
        return isNotValid;
    }

After that, we can show the success message. The name variable is defined in the validateByTypes function. We can do that again outside of the function, being aware that we have a count of two times for how many times we’ve defined the name variable.

    const isNotValid = validateByTypes(inputGroup, validationTypes[name]);
    if (isNotValid) {
        return isNotValid;
    }
    showValid(inputGroup);

Tidy up the login email input code

Now that the validate code works on successful validations too, we can remove the related code from login.js

        if (inputattr === "E-mail") {
            if (validate(this)) {
                return;
            }
            // const inputstr = $(this).find(".input-check").val().trim();
            // return inputStatus.ok(this, inputattr + " is Ok: Your data has been entered correctly");
        }

and the if statement and return isn’t needed there either.

        if (inputattr === "E-mail") {
            // if (validate(this)) {
            //     return;
            // }
            validate(this);
        }

Make the same simplification to change-password inputs

After removing else statements from the change-password code, it’s really easy to add the validate function for email.

        if (inputattr === "E-mail") {
            // if (inputstr === "") {
            //     return inputStatus.warning(this, inputattr + " is empty");
            // }
            // var fakeReg = /(.)\1{2,}/;
            // if (fakeReg.test(inputstr)) {
            //     return inputStatus.warning(this, inputattr + " is Fake // text: Please remove repetition");
            // }
            // var emailReg = /^([\w-\.]+@([\w-]+\.)+[\w-]{2,4})?$/;
            // if (!emailReg.test(inputstr)) {
            //     return inputStatus.warning(this, inputattr + " is Incorrect: Please enter it correctly");
            // }
            // return inputStatus.ok(this, inputattr + " is Ok: Your data has been entered correctly");
            return validate(this);
        }

We can now add the password checks to the validate function.

Password validation function

The password checks are similar to the the other checks that we’ve already done.

Add some basic password validations

We can add another validation type for password:

validate.js

    const validationTypes = {
        "E-mail": [checkEmpty, checkFake, checkEmailReg],
        "Password": [checkEmpty, checkFake]
    };

and we don’t need those password checks anymore in the login code:

login.js

        if (inputattr === "Password") {
            if (validate(this)) {
                return;
            }
            const inputstr = $(this).find(".input-check").val().trim();
            // if (inputstr === "") {
            //     return inputStatus.warning(this, inputattr + " is empty");
            // }
            // const fakeReg = /(.)\1{2,}/;
            // if (fakeReg.test(inputstr)) {
            //     return inputStatus.warning(this, inputattr + " is Fake text: Please remove repetition");
            // }
            //...
        }

Checking for short and long passwords

The short and long password checks are really easy to add, as they follow a similar structure to the other checks:

validate.js

    const validationTypes = {
        "E-mail": [checkEmpty, checkFake, checkEmailReg],
        "Password": [checkEmpty, checkFake, checkPasswordShort, checkPasswordLong]
    };
    //...
    function checkPasswordShort(inputGroup) {
        const pswReglow = /^([a-zA-Z0-9]{0,5})$/;
        if (pswReglow.test(value)) {
            inputStatus.warning(inputGroup, getName(inputGroup) + " is Incorrect: Please enter at least 6 characters");
            return true;
        }
    }
    function checkPasswordLong(inputGroup) {
        const pswReghigh = /^([a-zA-Z0-9]{13,})$/;
        if (pswReghigh.test(value)) {
            inputStatus.warning(inputGroup, getName(inputGroup) + " is Incorrect: Please enter no more than 12 characters");
            return true;
        }
    }

As the tests are still working, we can remove the login.js code for checking short and long passwords:

        if (inputattr === "Password") {
            if (validate(this)) {
                return;
            }
            // const inputstr = $(this).find(".input-check").val().trim();
            // const pswReglow = /^([a-zA-Z0-9]{0,5})$/;
            // if (pswReglow.test(inputstr)) {
            //     return inputStatus.warning(this, inputattr + " is Incorrect: Please enter at least 6 characters");
            // }
            // const pswRegheigh = /^([a-zA-Z0-9]{13,})$/;
            // if (pswRegheigh.test(inputstr)) {
            //     return inputStatus.warning(this, inputattr + " is Incorrect: Please enter no more than 12 characters");
            // }
            // return inputStatus.ok(this, inputattr + " is OK: Your data has been entered correctly");
        }

Standardise on some of the messaging

As some of the messages for when ok are either “is Ok” or “is OK” I’ve chosen to standardise on “is Ok”.

login-input-password.test.js

        it("shows a message", function () {
            $passwordError.html("");
            loginInputHandler($passwordGroup);
            // expect($passwordError.html()).to.equal("Password is OK: Your data has been entered correctly");
            expect($passwordError.html()).to.equal("Password is Ok: Your data has been entered correctly");
        });

Remove the if statement

Now that we don’t have other code below the validation, the if statement is no longer needed.

login.js

    function loginInputHandler() {
        const inputattr = $(this).find(".input-check").attr("name");
        if (inputattr === "E-mail") {
            validate(this);
        }
        if (inputattr === "Password") {
            // if (validate(this, "password")) {
            //     return;
            // }
            validate(this);
        }
    }

and because we earlier said, we can return the if/else structure back to the code.

login.js

    function loginInputHandler() {
        const inputattr = $(this).find(".input-check").attr("name");
        if (inputattr === "E-mail") {
            validate(this);
        // }
        // if (inputattr === "Password") {
        } else if (inputattr === "Password") {
            validate(this);
        }
    }

Remove more duplication

The if/else structure doesn’t really matter now though because we see a clear pattern of duplication. The validate statements in the if structure are identical. That means that the if conditions aren’t needed. They are the only use of the name variable, so that isn’t needed too. We can delete those as well.

    function loginInputHandler() {
        // const inputattr = $(this).find(".input-check").attr("name");
        // if (inputattr === "E-mail") {
        //     validate(this);
        // } else if (inputattr === "Password") {
        //     validate(this);
        // }
        validate(this);
    }

That validate commnand is just passing things on now. We can replace loginInputHandler with a reference to the validate function instead.

// $("#login .form-group").on("focusin focusout input", loginInputHandler);
$("#login .form-group").on("focusin focusout input", validate);

And the loginInputHandler function disappears in a puff smoke.

Summary

We were aiming at removing duplication, and I think that we over-achieved in that area!

That I think it a good place to leave things for today.

The code as it stands today is found at v0.0.19 in releases

Next time we clean up after all of this excitement today.