Remove duplication to improve code

JavaScript
47

Removing duplication from when accessing the input field is what we’re up to today. We will be doing the following:

  • Make the form reset messages similar to each other
  • Investigate the cause of duplication
  • Devise a plan to remove that duplication

Duplication found when accessing the .check field

The next set of duplication is in regard to finding the input field to check in a form group.

Match - 3 instances

./registration3\input-status.js:115,119
formGroups.forEach(function resetGroup(formGroup) {
    var inputName = $(formGroup).find(".check").attr("name");
    inputStatus.errorOk(formGroup, "Your " + inputName);
    inputStatus.feedbackNone(formGroup);
});

./registration3\registration.js:115,117
function resetMessages(ignore, formGroup) {
    const name = $(formGroup).find(".check").attr("name");
    inputStatus.errorOk(formGroup, name);

./registration3\validate.js:113,115
function checkFormEmpty(form) {
    const $formGroups = $(form).find(".form-group").has(".check");
    $formGroups.each(function validateGroup(ignore, formGroup) {

Is it appropriate to have each section use a separate function to get the input to check? I don’t think so.

Another option is to have one of the sections of code have a public function to retrieve the input to check, and have the other sections of code use that function. That doesn’t seem right either.

Make messages consistent with each other

Another thing that I notice that could be useful is that the errorOk function is being given different names to show. In one case it’s being given "Your " + inputName and in another case it’s just being given name. As the registration form isn’t using “Your”, we should stay consistent with that.

This means updating the tests so that any which expect Your at the start of a message, excluding of course Your City which is a field name in its own right.

inputstatus.test.js

        it("resets one message", function () {
            $error1.find(".error").html("");
            inputStatus.resetForm($form);
            // expect($error1.html()).to.equal("Your " + $name1);
            expect($error1.html()).to.equal($name1);
        });
        it("resets another message", function () {
            $error2.find(".error").html("");
            inputStatus.resetForm($form);
            // expect($error2.html()).to.equal("Your " + $name2);
            expect($error2.html()).to.equal($name2);
        });

We can now update the input-status code so that the Your prefix is not used:

    function resetForm($form) {
        const formGroups = $form.find(".form-group").toArray();
        formGroups.forEach(function resetGroup(formGroup) {
            var inputName = $(formGroup).find(".check").attr("name");
            // inputStatus.errorOk(formGroup, "Your " + inputName);
            inputStatus.errorOk(formGroup, inputName);
            inputStatus.feedbackNone(formGroup);
        });
    }

The remaining tests that care about the Your prefix can be updated too.

login-reset.test.js

   it("uses reset event to reset the form", function () {
        $emailError.html("");
        $("#login").trigger("reset");
        // expect($emailError.html()).to.contain("Your E-mail");
        expect($emailError.html()).to.equal("E-mail");
    });
    it("shows email message", function () {
        $emailError.html("");
        loginResetHandler();
        // expect($emailError.html()).to.equal("Your E-mail");
        expect($emailError.html()).to.equal("E-mail");
    });

changepassword-reset.test.js

    it("resets email message", function () {
        const $emailGroup = $("#changepw .form-group").eq(1);
        const $emailError = $emailGroup.find(".error");
        $emailError.html("test content");
        passwordResetHandler(fakeEvt);
        // expect($emailError.html()).to.equal("Your E-mail");
        expect($emailError.html()).to.equal("E-mail");
    });

Exploring duplication removal options

There might be something that we can do to remove the duplication that was found. I’m not quite sure what that is yet, so let’s work through a few possible ideas to explore what can be done.

The errorOk function almost always receives the same type of message.

[Note: This is where my hindbrain becomes disturbed]

As the inputGroup and the message are connected, in that the input field’s name attribute is what starts the message, the tests must follow that same behaviour too.

[Note: This is where I become aware of the problem]

But that’s not all. The reset messages also use errorOk which tends to indicate that we should have a separate errorReset function too.

[Note: And here is where I understand the issue]

Another problem comes to mind too. if I add an errorReset function just so that the errorOk function can show a boiler-place success message, that causes the inputStatus code to be too specific. That isn’t where the consistant success message should come from.

Instead, the success messages should only come from the validate code itself.

It all becomes clear to me now. Each set of code needs to have clearly defined boundaries.

  1. The inputStatus code must know nothing about which error message to give.
  2. The registration code (along with login, and changepassword code) must tell something else to reset the form.
  3. The validate code is the only place where the name is retrieved from input fields.

In terms of the duplication that was found, inputStatus needs to receive a reset message, the registration code needs to tell something else (validate.js for now) to reset the form, and the validate code needs to be the only place where the name of the input field is retrieved.

Summary

We made the form reset messages similar to each other, investigate the cause of duplication, and devised a plan to remove that duplication.

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

Next time we remove duplication from when accessing the input fields.

2 Likes