Converting an IP

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  255) {
      isValid = false;
    }
  });
  return isValid;
},

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

  if (!_isIPv4(ip)) {
    throw 'Invalid IPv4 address!';
  }
  return ((octets[0] >> 0) +
         ((octets[1] >> 0) +
         ((octets[2] >> 0) +
         (octets[3]);
};

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

StackExchange Code Review Q#54775, answer score: 19