patternjavascriptMinor
Do I have too many regexes in my validation logic?
Viewed 0 times
regexesvalidationtoologicmanyhave
Problem
The below function uses Javascript to validate a Belgian telephone number and format it according to the guidelines for phone number formatting.
As you can see, I first use a Regex to strip any excess delimiters from the code, then I use another Regex to check if the number is valid, then I use another Regex to format the number in a way that's more easily readable.Now, it works, most of it is readable (the validation regex is tricky but has a comment to explain it), but I'm not sure if using Regex everywhere is a good idea.
However, I don't immediately see a
function acm_mscrm2011_psa_FormatPhoneNumber(executioncontext) {
//get the phone number from the field that starts the validation
var telefoonnummer = executioncontext.getEventSource().getValue();
//check if not empty
if (telefoonnummer) {
telefoonnummer = telefoonnummer.replace(/(\s|-|\/|\.)+/g, "");
//check if it's a Belgian number
//Optional country code of +32 or 0032, followed by optional 0, followed by a local part of either (an 8 digit number) OR (a 4 followed by 8 digits)
var telefoonRegex = /^(?:\+32|0032)?0?([1-9][0-9]{7}|4[0-9]{8})$/;
if (telefoonRegex.test(telefoonnummer)) {
var telefoonMetFormaat;
//get the first captured group of the previous test, i.e. the local part
var localpart = RegExp.$1;
if (localpart.length == 8) {
//landline or fax
//group in format # ### ## ##
telefoonMetFormaat = localpart.replace(/(\d)(\d\d\d)(\d\d)(\d\d)/, "+32 $1 $2 $3 $4");
} else {
//cellphone number
//group in format ### ## ## ##
telefoonMetFormaat = localpart.replace(/(\d\d\d)(\d\d)(\d\d)(\d\d)/, "+32 $1 $2 $3 $4");
}
//put the validated and formatted number back into the system
executioncontext.getEventSource().setValue(telefoonMetFormaat);
}
}
}As you can see, I first use a Regex to strip any excess delimiters from the code, then I use another Regex to check if the number is valid, then I use another Regex to format the number in a way that's more easily readable.Now, it works, most of it is readable (the validation regex is tricky but has a comment to explain it), but I'm not sure if using Regex everywhere is a good idea.
However, I don't immediately see a
Solution
For one, I wouldn't worry about the number of regexps here. There's nothing terribly complex going on here. I'd say regular expressions are the right tools for this job, so no reason to change that.
I do have some other comments though:
-
I'd advice you to always use English in your code, so
-
You can combine the first step of sanitizing the input and checking if there is any. To avoid indenting the code, you can also just return early.
While you're at it, you may want to simplify the regexp a bit. You basically just want to replace everything that isn't a digit or a plus sign:
-
The large regexp may be simplified a bit (and be made more consistent by always using
You've already got the formatting part handled well. I've changed it a little, but nothing major. I've swapped out
You could also combine a couple of steps, and do
I do have some other comments though:
-
I'd advice you to always use English in your code, so
phoneNumber, formattedPhoneNumber etc.. I'm not a native English-speaker myself, but I never write code in any other language. Just for consistency.-
You can combine the first step of sanitizing the input and checking if there is any. To avoid indenting the code, you can also just return early.
While you're at it, you may want to simplify the regexp a bit. You basically just want to replace everything that isn't a digit or a plus sign:
var input = executioncontext.getEventSource().getValue();
var phoneNumber = String(input).replace(/[^\d+]/g, '');
if(!phoneNumber) return;-
The large regexp may be simplified a bit (and be made more consistent by always using
\d in place of 0-9). I'd write it as follows, and use match instead of testvar match = phoneNumber.match(/^(?:\+32|0032)?0?([1-9]\d{7}|4\d{8})$/);
if(!match) return; // again we can return if the number wasn't matchedYou've already got the formatting part handled well. I've changed it a little, but nothing major. I've swapped out
\d for simply . (since we know we're only dealing with digits at this point) but that's not necessary - \d would do equally well.function acm_mscrm2011_psa_FormatPhoneNumber(executioncontext) {
var input, phoneNumber, match, formattedNumber;
input = executioncontext.getEventSource().getValue();
phoneNumber = String(input).replace(/[^\d+]/g, '');
if(!phoneNumber) return;
match = phoneNumber.match(/^(?:\+32|0032)?0?([1-9]\d{7}|4\d{8})$/);
if(!match) return;
if(match[1].length === 8) {
formattedNumber = match[1].replace(/(.)(...)(..)(..)/, "+32 $1 $2 $3 $4");
} else {
formattedNumber = match[1].replace(/(...)(..)(..)(..)/, "+32 $1 $2 $3 $4");
}
executioncontext.getEventSource().setValue(formattedNumber);
}You could also combine a couple of steps, and do
function acm_mscrm2011_psa_FormatPhoneNumber(executioncontext) {
var input, match, formattedNumber;
input = executioncontext.getEventSource().getValue();
match = String(input)
.replace(/[^\d+]/g, '')
.match(/^(?:\+32|0032)?0?([1-9]\d{7}|4\d{8})$/);
if(!match) return;
if(match[1].length === 8) {
formattedNumber = match[1].replace(/(.)(...)(..)(..)/, "+32 $1 $2 $3 $4");
} else {
formattedNumber = match[1].replace(/(...)(..)(..)(..)/, "+32 $1 $2 $3 $4");
}
executioncontext.getEventSource().setValue(formattedNumber);
}Code Snippets
var input = executioncontext.getEventSource().getValue();
var phoneNumber = String(input).replace(/[^\d+]/g, '');
if(!phoneNumber) return;var match = phoneNumber.match(/^(?:\+32|0032)?0?([1-9]\d{7}|4\d{8})$/);
if(!match) return; // again we can return if the number wasn't matchedfunction acm_mscrm2011_psa_FormatPhoneNumber(executioncontext) {
var input, phoneNumber, match, formattedNumber;
input = executioncontext.getEventSource().getValue();
phoneNumber = String(input).replace(/[^\d+]/g, '');
if(!phoneNumber) return;
match = phoneNumber.match(/^(?:\+32|0032)?0?([1-9]\d{7}|4\d{8})$/);
if(!match) return;
if(match[1].length === 8) {
formattedNumber = match[1].replace(/(.)(...)(..)(..)/, "+32 $1 $2 $3 $4");
} else {
formattedNumber = match[1].replace(/(...)(..)(..)(..)/, "+32 $1 $2 $3 $4");
}
executioncontext.getEventSource().setValue(formattedNumber);
}function acm_mscrm2011_psa_FormatPhoneNumber(executioncontext) {
var input, match, formattedNumber;
input = executioncontext.getEventSource().getValue();
match = String(input)
.replace(/[^\d+]/g, '')
.match(/^(?:\+32|0032)?0?([1-9]\d{7}|4\d{8})$/);
if(!match) return;
if(match[1].length === 8) {
formattedNumber = match[1].replace(/(.)(...)(..)(..)/, "+32 $1 $2 $3 $4");
} else {
formattedNumber = match[1].replace(/(...)(..)(..)(..)/, "+32 $1 $2 $3 $4");
}
executioncontext.getEventSource().setValue(formattedNumber);
}Context
StackExchange Code Review Q#58654, answer score: 4
Revisions (0)
No revisions yet.