5
\$\begingroup\$

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 different solution that doesn't involve a large amount of nested ifs, fors and substrings that ultimately would be just as tricky to read and maintain as these 3 regexes.

Is there an alternative to these regexes?

\$\endgroup\$
  • 2
    \$\begingroup\$ At least use [0-9] and \d consistently. And I'd personally replace \d\d\d with \d{3}. As to the number of expressions, I think you're fine. Fewer, more complex ones, wouldn't make it more readable. \$\endgroup\$ – jessehouwing Jul 31 '14 at 13:59
  • \$\begingroup\$ @jessehouwing Isn't there a semantical difference between those 2 options? I thought \d also included a couple of non-Arabian numerals. \$\endgroup\$ – Nzall Jul 31 '14 at 14:11
  • \$\begingroup\$ @jessehouwing Now obviously, I'm not going to get many non-Arabian numerals from my users, and it doesn't really matter in the end, but still, it's not exactly the same. \$\endgroup\$ – Nzall Jul 31 '14 at 14:34
  • \$\begingroup\$ Right, so if they're not exactly the same, why are you mixing and matching them? That was kind of my point. Either use one, or use the other. \$\endgroup\$ – jessehouwing Jul 31 '14 at 14:43
  • \$\begingroup\$ @jessehouwing - your comments would make for a good answer, why not do that? \$\endgroup\$ – rolfl Jul 31 '14 at 14:48
4
\$\begingroup\$

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:

  1. 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.

  2. 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;
    
  3. 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);
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Flambino, English may not be your first language, but one would never know it from the way you write, both code and words. Your command of a second language is impressive, especially to those of us who are proficient in but one. \$\endgroup\$ – Cary Swoveland Jul 31 '14 at 16:42
  • \$\begingroup\$ @CarySwoveland Thank you! That's very kind of you to say :) And if I'm being honest, I do take a lot of pride in my English. Unfortunately, it's also the only foreign language that's stuck, despite teachers' best efforts to teach me 3-4 other languages (not all at once, thankfully). Oh, well... \$\endgroup\$ – Flambino Jul 31 '14 at 17:27
  • \$\begingroup\$ Do you think it's worth it to pre-assign match[1] between the 2 if statements to improve readability (giving a clearer name like localPart)? And yes, I already replaced my variable names right after I asked the question, but forgot to update the question itself. \$\endgroup\$ – Nzall Aug 1 '14 at 10:10
  • \$\begingroup\$ @NateKerkhofs Your call - it certainly can't hurt to do that. I thought about it myself, because it would make the code more readable, but, personally, when it's only used in the lines immediately following, and the regex only has 1-2 capture groups to keep track of, I skip it. You can also just use a comment, much like the one in your original code // the first capture group, match[1], is the phone number (without the country code) \$\endgroup\$ – Flambino Aug 1 '14 at 10:31

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.