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.

I've written a DNS tool in Node and would like to know if there's a better and more efficient way of handling the conversion from an IP to a long. I'm not too good at bitwise just yet and would like to hear any suggestions. Psuedo-code has been provided below.

var _parse = function (ip) {
  var octets = ip.split('.');
  return octets.map(function (octet) {
    return octet | 0;
  });
},

 _isIPv4 = function (ip) {
  var octets = _parse(ip),
    len = octets.length,
    isValid = true;

  if (len !== 4) return false;

  octets.forEach(function (octet) {
    if (octet < 0 || octet > 255) {
      isValid = false;
    }
  });
  return isValid;
},

toLong = function (ip) {
  var octets = _parse(ip);

  if (!_isIPv4(ip)) {
    throw 'Invalid IPv4 address!';
  }
  return ((octets[0] << 24) >>> 0) +
         ((octets[1] << 16) >>> 0) +
         ((octets[2] <<  8) >>> 0) +
         (octets[3]);
};
share|improve this question
add comment

3 Answers 3

up vote 14 down vote accepted

I wouldn't both prefixing 'internal' functions with an underscore. Instead, just put this in a module, and only export the functions you want to export.


If you care about following the node style guide, you should know that node suggests ignoring Crockford's fetish for one var declaration.

Note: this is pretty controversial. I'm not a fan of var declarations at the top, especially when they're all crammed into one long, continual declaration. Tons of people would fight me to the death over this though.


Your validation rules aren't strict enough, unless you care only about there being 3 periods present. ..., a.b.c.d are just two simple examples of things that are clearly, completely invalid, but your code allows.

In short, for situations like these, implicit conversions are evil. An implicit 0 just masks the error rather than letting you know it happened. Instead of explicitly converting to a number, use parseInt() to verify that you don't get back NaN: var i = parseInt(s, 10); if (Number.isNaN(i)) { /* uh oh! */ }.


Don't throw a string. Instead, throw some kind of exception object (like something deriving from Error).


toLong is a very, very vague name. Once again, in a module this would be fine, but as a bare function, it makes me wonder what in the world it actually does. (Also, longs don't exist in JavaScript, so it's a bit of a misnomer).


The zero-fill shift operators aren't doing anything here since the numbers are guaranteed to be non-negative.

Edit: apparently JS interprets numbers bit shifted left as signed (dubious decision, but hey...). This means that x << 24 is going to be negative when x is between 128 and 255. In short, the >>> 0 is actually doing something on the shift by 24. On the other steps though, it is indeed doing nothing.


octets.forEach should really be using every instead: return octets.every(function(v) { return v >= 0 && v <= 255; });


Considering how mismatched these functions all feel interacting, I would probably just stick to one function. Yes, it would be nice to handle validation separately, but I would worry about that when the time comes, and if it does come, you can always break this into two functions then.

function ipStringToLong(ip) {
    var octets = ip.split('.');
    if (octets.length !== 4) {
        throw new Error("Invalid format -- expecting a.b.c.d");
    }
    var ip = 0;
    for (var i = 0; i < octets.length; ++i) {
        var octet = parseInt(octets[i], 10);
        if (Number.isNaN(octet) || octet < 0 || octet > 255) {
            throw new Error("Each octet must be between 0 and 255");   
        }
        ip |= octet << ((octets.length - i) * 8);
    }
    return ip;
}

(I can't decide how I feel about the ip |= stuff... Might be cleaner to stick with addition like in your code.)

share|improve this answer
    
Very good point. I tried to keep things separate just because I'm so use to maintaining things modular, but I like your idea here. I've never been good with ```every`` I think that would be much better in this situation. Could you explain a bit more about why I should use parseInt instead? Great post by the way. –  Lucien Lachance Jun 20 at 3:08
    
One more thing, it appears when given '172.2.6.139' it returns -1409284469 Returning ip >>> 0 seems to do the trick. –  Lucien Lachance Jun 20 at 4:40
    
@LucienLachance Ah, I should have seen that coming. The >>> 0 on the highest byte is actually doing something. Basically what happens is that 172 has the sign bit high (since 172 & 128 != 0), and this means that JS treats 172 << 24 as a negative number (oddly, in my opinion -- but hey, it's JS, we can't have too high of hopes for sane operations). Anyway, the easy fix is to just do return ip >>> 0 instead of return ip. That makes it realize the number is meant to be unsigned. –  Corbin Jun 20 at 4:46
    
Nice answer, although I don't like the hard-coded (3-i) when the loop uses octets.length. I know the length is checked, but still... it irks me a little. Maybe for(var i=0, l=array.length-1; i<=l; i++) and then (l-i) * 8. Or you could reverse the array before looping and just have i * 8 –  Flambino Jun 20 at 14:25
    
@Flambino Wow.... I considered iterating backwards, but I can't believe I used a hard coded 3. I'm embarrassed that I didn't catch that myself. Thanks! –  Corbin Jun 20 at 21:35
add comment

Your bitwise operations are fine, though the >>> right-shifts are unnecessary. @Lucien and @Corbin are right! The right-shift operators prevent (128 << 24) from being interpreted as a negative number.

The rest of the code is fine too, but a bit long for my taste. Personally, I'd use a long regular expression to do all the parsing and validation. That takes care of the .split(), the length == 4 check, the .forEach() and .map() loops, and the 0 ≤ octet ≤ 255 checks.

var ipv4QuadToLong = (function() {
    var octet = '([01]?[0-9]?[0-9]|2[0-4][0-9]|25[0-5])';
    var ipRegExp = new RegExp('^' + octet + '\.' + octet + '\.' + octet + '\.' + octet + '$');

    return function ipv4QuadToLong(string) {
        var octets = string.match(ipRegExp);
        if (octets === null) {
            throw 'Invalid IPv4 address!';
        } else {
            // @Lucien and @Corbin are right! The right-shift operators
            // prevent (128 << 24) from being interpreted as a negative
            // number.
            return ((octets[1] << 24) >>> 0) +
                   ((octets[2] << 16) >>> 0) +
                   ((octets[3] <<  8) >>> 0) +
                   (octets[4] <<  0);
        }
    };
})();
share|improve this answer
    
I avoided regex because my skills as a ninja in regex are limited, haha. When I was writing tests, I was trying to aim for the simplest approach first and then re-iterate as needed. I'm glad you pointed that out though. I need to get my chops up in regex! –  Lucien Lachance Jun 20 at 3:13
add comment

Your parsing of IPv4 addresses is not incorrect, since it succeeds at parsing IPv4 addresses in dotted-quad notation as specified in RFC 810. However, you may be interested to know that the inet_aton() function in Unix is more lenient, supporting some unconventional forms:

1         :   0.  0.  0.  1
127.1     : 127.  0.  0.  1
192.168.1 : 192.168.  0.  1
8.8.8.8   :   8.  8.  8.  8

You should be prepared to accept between one to four octets, inclusive, and stick them in the appropriate positions. While most people use fully formed IP addresses, it may come to be that lazy users may try to use a clever shortcut and end up with an unexpected error.

This syntax is also allowed in IPv6 using the :: syntax, so you should at least be aware of the fact that shortcuts do exist. For example, you can do this on modern computers: ping ::1 (localhost).

Here's how I'd handle 1 to 3 octets:

var _parse = function (ip) {
  var octets = ip.split('.');
  while(octets.length < 4) {
      octets.splice(0, octets.length-1, 0);
  }
  return octets.map(function (octet) {
    return octet | 0;
  });
},
share|improve this answer
    
I didn't know that. How would you recommend handling this? –  Lucien Lachance Jun 20 at 3:20
    
@LucienLachance Just add a simple padding loop, as illustrated in my updated answer. –  phyrfox Jun 20 at 3:41
    
Oh, I get it now! This would be a good feature to include. I like this idea a lot. –  Lucien Lachance Jun 20 at 3:55
    
Citation please? I don't believe that "127.1" is conventional or legal. RFC 810 defines <address> ::= <octet> "." <octet> "." <octet> "." <octet> — no shortcuts accepted. –  200_success Jun 20 at 5:18
3  
@200_success On second thought, this answer came from my own flawed understanding of IPv4 based on a situation whereby I discovered this. I thought for sure it was a glitch, as I grew up on the 4 octet form and thought anything else was in error. It seems that my first impression was correct; some RFC material incorrectly allowed 1-4 octets, outlined in this Internet-Draft: tools.ietf.org/html/draft-main-ipaddr-text-rep-02. Thanks for spurring further research on this topic, as I now know why they work, and why we shouldn't care that they work. –  phyrfox Jun 20 at 5:44
show 3 more comments

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.