Is there an even shorter way of writing this script?

Question

I have been learning how to use the console and Firefox's ScratchPad.

I have a form that im writing validation for based on regex. So far it does exactly what i want it to do. The issue is i feel like its too much. Each input requires a different pattern and not all are required.

I attempted to come up with a for loop to handle this but it wasn't giving me the control i needed for each individual input. Is there a way to write for loops for only certain inputs? Or would i have to write one for loop per regular expression

And if what i have is the correct way to do this, is there at least a shorter way of writing it?

Please keep in mind at this point im just testing all the regex as i write them, hence the red stroke green stroke. This is not a validation question. I just want to know a shorter syntax instead of writing each line one by one, because i have about 16 inputs to account for.

// grabs the form
var myForm = document.forms["main-contact"]

// regular expressions
var onlyText = /^[A-Za-zÀ-ÖØ-öø-ÿ]+$/;
var textNumbers = /^[A-Za-zÀ-ÖØ-öø-ÿ0-9\s]+$/;
var onlyEmail = /^[\w-\.]+@([\w-]+\.)+[\w-]{2,4}$/;

// Testing value matches the regular expression
myForm[0].value.match(onlyText) && myForm[0].value.length >= 2 ? myForm[0].setAttribute("style","outline: unset") : myForm[0].setAttribute("style","outline: 2px solid crimson");
myForm[1].value.match(onlyText) && myForm[1].value.length >= 2 ? myForm[1].setAttribute("style","outline: unset") : myForm[1].setAttribute("style","outline: 2px solid crimson");
myForm[2].value.match(onlyEmail) && myForm[2].value.length >= 2 ? myForm[2].setAttribute("style","outline: unset") : myForm[2].setAttribute("style","outline: 2px solid crimson");
myForm[3].value.match(textNumbers) && myForm[3].value.length >= 2 ? myForm[3].setAttribute("style","outline: unset") : myForm[3].setAttribute("style","outline: 2px solid crimson");

you use regular expressions without any comments in your code ? (that is very bad practice) — Mister Jojo, Feb 04 '19 at 16:49
@MrJ: Is `var onlyText = /^[A-Za-zÀ-ÖØ-öø-ÿ]+$/; // text only` really better and more informative than just `var onlyText = /^[A-Za-zÀ-ÖØ-öø-ÿ]+$/;`? — Eric Duminil, Feb 04 '19 at 16:52
Appreciate it. again, this is just a small script that im quicly trying to test something new with. My final versions are usually overly commented but right now im still messing around with everything. And since im working alone the comments dont really matter at this stage to me if i know what im looking at. — Curtis Steele, Feb 04 '19 at 23:19

score 0 · Answer 1 · answered Feb 04 '19 at 17:08

This question should probably be on CodeReview.

myForm is a collection, it should probably be plural.
The 4 last lines are very similar, they could be refactored in a function, accepting a form and a regex as arguments.
You could replace the ternary operator with if
You could define CSS classes (correct & incorrect) instead of modifying the style.
You could zip myForms and an array of regexen ([onlyText, onlyText, onlyEmail, textNumbers]) in order to get a regex for each form, and send them to the new function.

brk · Accepted Answer · 2019-02-05T03:36:17.930

You can segregate the repeating code and create a common function. For example adding style is repeated in all the condition , which can be replaced with a common function and css class instead of inline style

var myForm = document.forms["main-contact"]

// regular expressions
var onlyText = /^[A-Za-zÀ-ÖØ-öø-ÿ]+$/;
var textNumbers = /^[A-Za-zÀ-ÖØ-öø-ÿ0-9\s]+$/;
var onlyEmail = /^[\w-\.]+@([\w-]+\.)+[\w-]{2,4}$/;


for (var x = 0; x < 4; x++) {
  let elem = myForm[x];
  let elemValue = myForm[x].value;
  if (x === 2) {
    testCond(elemValue, 'onlyEmail') ? setOutline(elem) : setOutline(elem,
    'outlineCrimson');
  } else if (x === 3) {
    testCond(elemValue, 'textNumbers') ? setOutline(elem) : setOutline(elem, 'outlineCrimson');
  } else {
    testCond(elemValue, 'onlyText') ? setOutline(elem) : setOutline(elem, 'outlineCrimson');
  }
}
// function to test the regex and check the length of value
function testCond(b, a) {
  return elemValue.match(a) && 2 <= elemValue.length ? !0 : !1;
};

// function to set the class, by default it will be setOutline
function setOutline(elem, defClass = 'setOutline') {
  elem.classList.add(defClass)
}

.setOutline {
  outline: unset
}

.outlineCrimson {
  outline: 2px solid crimson;
}

score 0 · Answer 3 · answered Feb 04 '19 at 17:48

I don't know if there is a shorter way, since I have no experience working with ScratchPad, but since you are hardcoding the same step over and over again, you can (if the myForm object is within the scope) make a boolean function (that can also set a value), like this:

function matchReg(field, num){
    if (myForm[num].value.match(field) && myForm[num].value.length >= 2){
        //attribute setting can also be done out of function, with boolean value
        myForm[num].setAttribute("style","outline: unset");
        return true;
    } else {
        myForm[num].setAttribute("style","outline: 2px solid crimson");
        return false;
    }
}

and call it like this:

matchReg(onlyText, 0);
matchReg(onlyText, 1);
//etc...

If this is still to much hard coding, you could write an array or some other sort of iterable object (like a dictionary, or a 2d array), like so:

//js 2d array
var toIterate = [
         [onlyText, 0],
         [onlyText, 1],
         [onlyEmail,2],
         //etc....
]
//js dictionary object
var toIterate = {
        0: onlyText,
        1: onlyText,
        2: onlyEmail,
        //etc....
}

and iterate over it :

//for the dict:
Object.keys(toIterate).forEach(key => {
    matchReg(toIterate[key],key); 
});
//for the array-object:
for (var i = 0; i < toIterate.length; i++) {
   matchReg(toIterate[i][0],toIterate[i][1])
}

(seen on https://stackoverflow.com/a/41550077)

generally speaking, there are lots of ways to solve this, and every programmer should be seeing that something is afoot when there are very similar lines of code showing up. it is common practice to take the code snippet that is coming up over and over again and design a function for it, but i guess you'd already known this.

also, coming from a java background, the functions presented here may be faulty or not time efficient.

Thanks. I guess my question was poorly worded. But since i screwed up on the for loop, what i wanted to know was a better way of writing a function that does what i wrote up top. Solved — Curtis Steele, Feb 04 '19 at 23:24

Slai · Answer 4 · 2019-02-05T07:16:07.147

{2,} can be used instead of + to match 2 or more, and outline can be set with .style.outline :

var onlyText = /^[A-Za-zÀ-ÖØ-öø-ÿ]{2,}$/;
var textNumbers = /^[A-Za-zÀ-ÖØ-öø-ÿ0-9\s]{2,}$/;
var onlyEmail = /^[\w-\.]+@([\w-]+\.)+[\w-]{2,4}$/;
var myForm = document.forms["main-contact"]

function setOutline(i, r) { 
  myForm[i].style.outline = r.test(myForm[i].value) ? "unset" : "2px solid crimson"; 
}

setOutline(0, onlyText);
setOutline(1, onlyText);
setOutline(2, onlyEmail);
setOutline(3, textNumbers);

Alternative with loop and array with the regular expressions :

var onlyText = /^[A-Za-zÀ-ÖØ-öø-ÿ]{2,}$/;
var textNumbers = /^[A-Za-zÀ-ÖØ-öø-ÿ0-9\s]{2,}$/;
var onlyEmail = /^[\w-\.]+@([\w-]+\.)+[\w-]{2,4}$/;
var myForm = document.forms["main-contact"]
var patterns = [onlyText, onlyText, onlyEmail, textNumbers];

for (var i = 0; i < 4; ++i) {
  myForm[i].style.outline = patterns[i].test(myForm[i].value) ? "unset" : "2px solid crimson"; 
}

Is there an even shorter way of writing this script?

4 Answers4