current community

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

We have a lot of phone numbers that contain dashes and start with a zero which are from another country.

I am trying to strip the dashes and the one leading zero and select the country code from a drop-down box.

I'm using "raw" JS with DOM here (for the sake of not having to pull in a framework):

<html>
<head><title>Dash Remover</title></head>
<body>
<form method="POST">
<select id="countryCallingCode">
    <option value="0043">+43</option>
    <option value="0044">+44</option>
    <option value="0049">+49</option>
    <option value="">Other</option>
</select>
<input id="numbertodial" type="text" size="16"
       oninput="prepareTelNo()" />

<button name="callBtn" onclick="callTelNo();">Call!</button>
</form>

<script>
function getCCC() { return document.getElementById('countryCallingCode').value; }

function prepareTelNo() {
    var wd = document.getElementById('numbertodial');
    wd.value = wd.value.replace(/-/g, ''); // strip - from the tel. no.

    // Cut off leading zeros
    if(wd.value.charAt(0) === '0' & getCCC() != "")
        wd.value = wd.value.substring(1);
}

function callTelNo() {
    var telNo = document.getElementById('numbertodial').value;
    window.alert('Calling '+getCCC()+telNo);
}
</script>
</body>
</html>

Any room for improvement? I know that oninput is only supported in some browsers.

That's ok, it's for an intranet app that is usually used with IE 10.

share|improve this question
 
You're only cutting off 'a' leading zero (and not all of them), (all that needs fixing is changing that if to a while). You're also not caching your selectors (not a big deal if that's all your JS). Maybe use addEventListener in favor of inlining onclick or oninput. Other than that your code looks fine :) –  Benjamin Gruenbaum Jun 26 '13 at 1:35
 
Thanks. We only happen to have numbers with one leading zero, so that's ok. I'll try addEventListener instead. Thanks! –  phw Jun 26 '13 at 1:57
add comment

1 Answer

up vote 2 down vote accepted

Just a few points

  • You're missing a & in your if conditional. Right now you're doing a bitwise AND, rather than a logical AND (which would be &&).

  • I'd say you should always use curly braces, even for one-liners. JS accepts one-liners just fine, but as a code-hygiene thing it's better to have braces there, I think.

  • You could rewrite the reg exp to remove everything that isn't a numerical digit (instead of only removing dashes). I don't know if that's what you need, but if it is

    wd.value = wd.value.replace(/[^\d]/g, ""); // remove all non-digit characters

  • You also could use a little more reg exp to make sure you remove all leading zeros (even if you say there's only going to be 1). Incidentally, this allows you to skip the "starts with zero" check, and only have the getCCC() != "" check. Point is, you can do a replace for leading zeros specifically, rather than "blindly" chopping something off of the string.

function prepareTelNo() {
    var wd = document.getElementById('numbertodial');
    wd.value = wd.value.replace(/-/g, ''); // strip - from the tel. no.

    // Cut off leading zeros if a country code is set
    if(getCCC() != "") {
        wd.value = wd.value.replace(/^0+/, ""); // cuts off all leading zeros
    }
}

If you really, truly only want to remove 1 leading zero rather than all of them, use /^0/ (without the plus sign) as the reg exp pattern.

share|improve this answer
 
Nice suggestion! I see again: I really need to improve my regexp knowledge :). Thanks again! –  phw Jun 27 '13 at 20:08
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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