2
\$\begingroup\$

Since I have a function which can be done in few ways as usual, I was wondering which way is the best to achieve the form validation where I have only one input field, and I am showing a warning on keyup if the number of characters are in between 1 and 3 as well as enabling disabling submit button based on number of characters. Here is the fiddle. So I have two functions:

var myFunction = function () {
    var input = document.getElementById("input");
    var warning = document.getElementById("warning");
    var button = document.getElementById("button");

    if (input.value.length >= 1 && input.value.length <= 3){
        warning.style.display = 'block';
    }
    else {
        warning.style.display = 'none';
    }

    if(input.value.length > 3){
        button.disabled = false;
    }
    else {
        button.disabled = true;
    }
}

Here I have two if else statements, so I was wondering what would be the cleanest way to achieve the same thing? The only thing I could come up with was this function:

var alternative = function () {
    var input = document.getElementById("input");
    var warning = document.getElementById("warning");
    var button = document.getElementById("button");
    warning.style.display = 'none';
    button.disabled = true;

    if (input.value.length >= 1 && input.value.length <= 3)
        warning.style.display = 'block';

    if(input.value.length > 3)
        button.disabled = false;
}

But I am not sure what is the best approach performance wise, since in the second function I have states that will always be the same on calling the function and then changing them based on conditions and in the first one I first check the conditions and then manipulate the elements?

\$\endgroup\$
4
\$\begingroup\$

Ternary operator

You could use the ternary operator for your first if-else case:

warning.style.display = input.value.length >= 1 && input.value.length <= 3 ? 'block' : 'none';

Now, this is a bit lengthy so we might choose to store input.value.length inside another variable like so:

var length = input.value.length;
warning.style.display = length >= 1 && length <= 3 ? 'block' : 'none';

Note, that if you can use ES6 features you could benefit from using const and object matching and declare length as follows:

const { length } = input.value;

Direct assignment

As for your second scenario, you could store the conditional directly inside your variable since you just want the negated version of what you're checking:

button.disabled = length <= 3;

A more general code review

The comparison against 1 and 3 is very vague and they are considered magic numbers. What do these variables represent?

You could incorporate another self explaining variable, such as isWarning as follows:

// ES5
var length = input.value.length;
var isWarning = length >= 1 && length <= 3;

// ES6
const { length } = input.value;
const isWarning = length >= 1 && length <= 3;

warning.style.display = isWarning ? 'block' : 'none';
button.disabled = length <= 3;

This helps the structure, but still doesn't answer the question of what do the values 1 and 3 represent?

I'm assuming you aren't checking 0 since you don't want a warning to show on an empty input, so I'll provide an example of what could be done:

// For ES5 change const to var
const minLength = 4; 

const isEmpty = length === 0;
const isValidLength = length >= minLength;
const isWarning = !isValidLength && !isEmpty;

warning.style.display = isWarning ? 'block' : 'none';
button.disabled = !isValidLength;

Note, that minLength would ideally have a more descriptive name related to what it is checking. minNameLength, minPostcodeLength, etc. This way the code reads relating to the logic it's checking, now how that logic is implemented each time.

Misc - ES6 deep object matching

If you are using ES6 you could use deep object matching to get length directly:

const { value: { length } } = document.getElementById('input');

This is useful since we are only concerned with the input's length, not the element itself.

Misc - Consistent use of quotes

Another small remark is your use of quotes. Either is fine but it's best to be consistent - you're using double quotes in getElementById but single quotes for setting display.

I'll continue to add things when I have time. If you have any questions/concerns please leave a comment.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.