HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

CIDR Notation parser

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
parsercidrnotation

Problem

I created the following code to better understand CIDR notation. Also to learn some of the Stream features in Java8. I posted my unit tests too.

I'm interested in ways it might be improved especially by using Java8 features. I reviewed the InetAddr class and realized it uses at array of bytes instead of ints. I did not find that convenient.

```
package com.company;

import java.util.ArrayList;

/**
* This class accepts a String in CIDR format in the constructor
* It calculates IP range and has methods for isInRange and count
*/
public class Cidr {
public IPAddress ip;
public int mask;

// get an array of ints given a mask (like /24 /16 /8 etc...)
int[] getIntMaskArray(int mask){
int[] intArray = new int[4];
String[] maskArray = getArrayBinaryStrings(mask, "1");
for (int i = 0; i 0){
s = s+fill;
mask = mask -1;
} else {
s = s+notFill;
}
}
binArray[x] = s;
s = "";
}
return binArray;
}

public IPAddress getNetMask() throws InvalidIPException {
// netmask is the mask converted to an IP Address:
// can't use getArrayBinaryStrings because that returns array of binary strings!
return new IPAddress(StringUtils.implode(getIntMaskArray(mask)));
}

IPAddress getAddress(){
return this.ip;
}

public boolean isInRange(IPAddress ipAddress){
return this.getHostMax().compareTo(ipAddress) > 0 && this.getHostMin().compareTo(ipAddress) ipList(){
ArrayList result = new ArrayList();
IPAddress hostMin = getHostMin();
long longip = hostMin.toDecimal();
while(longip <= getHostMax().toDecimal()){
result.add(new IPAddress(longip));
longip++;
}
return result;
}

public Cidr(String cidr){

//divide string into IP + mask:
String[] ip_mask = cidr.s

Solution

Working on String int[] int

You're working on String and int[]. I have absolutely no idea why. The cool thing about CIDR notation is that it consists of 4 "octets". The value range 0-255 is exactly that of an unsigned byte.

Funnily enough, \$4 * 8 = 32\$ ... Interestingly that's exactly the size of an Integer. What you need to represent an IPv4 address is a single int. If you want to keep track of a subnet (which is a significantly different thing), you just need to add another int for the subnet mask.

Properly working with this you can then determine the first and last IP address in a subnet by the following rather easy process:

int lowestAddress = address & subnetMask;
int highestAddress = address | !subnetMask;


Unfortunately java is a little unfriendly if we look for unsigned primitive types, which means that trying to parse 255 into a byte results in an Exception.

As soon as you consider that working on a single integer is significantly faster (and more idiomatic), I think it becomes obvious you should consider reimplementing this by using byte-shifting, bitmasks and bitwise operators.

I strongly suggest you completely reimplement that class using just that representation.

Nitpicks:

  • You're indenting by 2 spaces. It's usual to indent by 4 spaces in java. Then again you are consistent, soo ...



  • You're using default visibility a lot. I don't like that, because I'd expect that to be explained by a comment. That visibility is not needed very often.

Code Snippets

int lowestAddress = address & subnetMask;
int highestAddress = address | !subnetMask;

Context

StackExchange Code Review Q#115831, answer score: 2

Revisions (0)

No revisions yet.