4 ms·

At first blush based on the patch to fix this bug, I’d say the code looks terrible: https://github.com/indutny/node-ip/commit/32f468f1245574785ec080705737a579be

by jey 2y ago

At first blush based on the patch to fix this bug, I’d say the code looks terrible: https://github.com/indutny/node-ip/commit/32f468f1245574785ec080705737a579be1223aa https://github.com/indutny/node-ip/commit/32f468f1245574785e... Seems impossible to read and verify. Wouldn’t it be simpler and more consistent to have a first parse the IP into an internal format then perform all logic on that?

jstasiak 2y ago
> Wouldn’t it be simpler and more consistent to have a first parse the IP into an internal format then perform all logic on that? So not speaking about this library specifically but yes, separating the address parsing part (https://www.netmeister.org/blog/inet_aton.html https://www.netmeister.org/blog/inet_aton.html is a fun read) from the "is this address considered private?" part should make things much simpler. Vast majority of related code in this space that I've seen works like that. I'm actually struggling to come up with counterexamples.
audiodude 2y ago
I feel like the main reason someone uses a library like this is that they don't want to write or maintain such a regex. Don't tell me how the sausage is made!
appplication 2y ago
It may be, but perhaps there is some performance concern? I’m not familiar with this domain and application so it’s not clear to me whether the decisions made are sensible just from reading the code.
- piva00 2y ago
  It's testing using regex, which is much less performant than converting an IP address to some numeric representation and running comparisons on that.
- lelanthran 2y ago
  > It may be, but perhaps there is some performance concern? I'm skeptical of that concern: executing multiple regular expression matches on a string is never going to be faster than parsing the entire string once, into a 4-octet structure, and then performing integer comparisons on the octets.
cjk2 2y ago
I concur. Any time I see tangles of regexes and if statements, it's going to be full of holes. I spent several months replacing messes like this with proper parsers and you'd be surprised at how many fuck ups fell out of the mess.
dgb23 2y ago
Parts of the code actually parses the IP strings into a sensible type (toBuffer). Why not use this representation for _all_ the queries and operations instead of mixing in regexes? Regexes can be fine in order to parse the string representations initially. But for everything else I would stick with byte arrays etc.
leptons 2y ago
"Impossible" to read? No. I didn't find it difficult to read at all. There are even comments that tell what the code is supposed to do, and tests that show how to use the functions. Are regular expressions "impossible" to read? Well maybe if someone doesn't understand regular expressions. There are a hundred million other ways to write these functions, sure, but if we wrote code to appease the personal tastes of every programmer in the world, we'd never get anything done.

by jey 2y ago

jstasiak 2y ago

> Wouldn’t it be simpler and more consistent to have a first parse the IP into an internal format then perform all logic on that? So not speaking about this library specifically but yes, separating the address parsing part (https://www.netmeister.org/blog/inet_aton.html https://www.netmeister.org/blog/inet_aton.html is a fun read) from the "is this address considered private?" part should make things much simpler. Vast majority of related code in this space that I've seen works like that. I'm actually struggling to come up with counterexamples.

audiodude 2y ago

I feel like the main reason someone uses a library like this is that they don't want to write or maintain such a regex. Don't tell me how the sausage is made!

appplication 2y ago

It may be, but perhaps there is some performance concern? I’m not familiar with this domain and application so it’s not clear to me whether the decisions made are sensible just from reading the code.

piva00 2y ago
It's testing using regex, which is much less performant than converting an IP address to some numeric representation and running comparisons on that.
lelanthran 2y ago
> It may be, but perhaps there is some performance concern? I'm skeptical of that concern: executing multiple regular expression matches on a string is never going to be faster than parsing the entire string once, into a 4-octet structure, and then performing integer comparisons on the octets.

cjk2 2y ago

I concur. Any time I see tangles of regexes and if statements, it's going to be full of holes. I spent several months replacing messes like this with proper parsers and you'd be surprised at how many fuck ups fell out of the mess.

dgb23 2y ago

Parts of the code actually parses the IP strings into a sensible type (toBuffer). Why not use this representation for _all_ the queries and operations instead of mixing in regexes? Regexes can be fine in order to parse the string representations initially. But for everything else I would stick with byte arrays etc.

leptons 2y ago

"Impossible" to read? No. I didn't find it difficult to read at all. There are even comments that tell what the code is supposed to do, and tests that show how to use the functions. Are regular expressions "impossible" to read? Well maybe if someone doesn't understand regular expressions. There are a hundred million other ways to write these functions, sure, but if we wrote code to appease the personal tastes of every programmer in the world, we'd never get anything done.