HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Do I have too many regexes in my validation logic?

Submitted by: @import:stackexchange-codereview··
0
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.

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 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 test

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 matched


You'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 matched
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);
}
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.