Remove duplication to improve code

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.

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.

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.

2 Likes

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>

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.

2 Likes

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.

2 Likes

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.

1 Like