Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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 < maskArray.length; i++) {
            intArray[i] = Integer.parseInt(maskArray[i], 2);
        }
        return intArray;
    }

    String[] getArrayBinaryStrings(int mask, String fill){
        String s = "";
        String[] binArray = new String[4];
        String notFill = "0";
        if (fill.equals("0")){
            notFill = "1";
        }

        // loop of four, since there are four array elements:
        for (int x = 0 ; x < 4; x++){
            // 8 parts of each array element:
            for(int y = 0; y<8; y++){
                if(mask > 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) < 0;
    }

    public long countHosts(){

        // host count should be hostMax - hostMin + 1
        return getHostMax().toDecimal() - getHostMin().toDecimal() + 1;
    }

    public IPAddress getHostMax(){
        // host max is one less than Broadcast
        return new IPAddress(getBroadcastAddress().toDecimal() - 1);
    }

    public IPAddress getHostMin(){
        // host min is simply adding one to the network address
        return new IPAddress(this.getNetworkAddress().toDecimal()+1);
    }

    public IPAddress getNetworkAddress(){
        String[] maskArray = getArrayBinaryStrings(mask, "1");
        return this.ip.and(getIpAddressFromBinary(maskArray));
    }

    public IPAddress getBroadcastAddress(){
        String[] invertedMaskArray = getArrayBinaryStrings(mask, "0");
        return getIpAddressFromBinary(invertedMaskArray).or(getNetworkAddress());
    }

    private IPAddress getIpAddressFromBinary(String[] maskArray) {
        String[] octetArray = new String[4];
        for (int i = 0; i < maskArray.length; i++) {
            octetArray[i] = String.valueOf(Integer.parseInt(maskArray[i], 2));
        }
        try {
            return new IPAddress(String.join(".", octetArray));
        } catch (InvalidIPException iip){
            iip.printStackTrace();
            return null;
        }
    }

    /**
     *
     * @return list of all IP addresses in network defined by cidr
     */
    public ArrayList<IPAddress> ipList(){
        ArrayList<IPAddress> result = new ArrayList<IPAddress>();
        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.split("/");
        try {
            this.ip = new IPAddress(ip_mask[0]);
        } catch (InvalidIPException e) {
            e.printStackTrace();
        }
        this.mask = Integer.parseInt(ip_mask[1]);
    }
}

Here is the IPAddress class:

package com.company;

import java.util.stream.Stream;
import static com.company.StringUtils.padLeft;

public class IPAddress implements Comparable {

    private int[] octets;

    public IPAddress(int[] octets) {
        this.octets = octets;
    }

    @Override
    public int compareTo(Object otherObject) {
        if (otherObject != null) {
            IPAddress secondIP = (IPAddress) otherObject;
            return (int) (this.toDecimal() - secondIP.toDecimal());
        } else {
            return -1;
        }
    }

    @Override
    public boolean equals(Object obj) {
        /* two ip addresses are equal if toStrings are equal: */
        return obj instanceof IPAddress && this.toString().equals(obj.toString());
    }

    public int[] getOctets() {
        return octets;
    }

    private boolean validIP(String ip){
        if(ip.split("\\.").length != 4){
            return false;
        }
        for (String octet : ip.split("\\.")){
            if (Integer.parseInt(octet) > 255 || Integer.parseInt(octet) < 0){
                return false;
            }
        }

        return true;
    }

    /**
     * create IP address using string
     * @param ip is an IP Address (ex: "10.10.232.133")
     */
    public IPAddress(String ip) throws InvalidIPException {
        // make sure the ip string is valid:
        // all the octets should fit into bytes
        if(!validIP(ip)){
            throw new InvalidIPException();

        }

        this.octets = Stream.of(ip.split("\\.")).mapToInt(Integer::parseInt).toArray();
    }

    /**
     * create IPAddress object using a decimal number
     * @param decimalRep decimal representation of IP address
     */
    public IPAddress(long decimalRep){

        String binString = padLeft(Long.toBinaryString(decimalRep),32,"0");
        // divide 32 character string into byte array:
        int[] octets = new int[4];
        int index = 0;
        int counter = 0;
        String newBinString;
        while (index < binString.length()){
            newBinString = binString.substring(index,index + 8);
            index = index + 8;
            int b = Integer.parseInt(newBinString, 2);
            octets[counter++] = b;
        }
        this.octets = octets;
    }

    public IPAddress and(IPAddress otherIP){

        int[] otherOctet = otherIP.getOctets();
        int[] thisOctets = this.getOctets();
        int[] newOctets = new int[4];
        for (int i = 0; i < otherOctet.length; i++) {
            newOctets[i] = thisOctets[i] & otherOctet[i];
        }
        return new IPAddress(newOctets);
    }

    public IPAddress or(IPAddress otherIP){

        int[] otherOctet = otherIP.getOctets();
        int[] thisOctets = this.getOctets();
        int[] newOctets = new int[4];
        for (int i = 0; i < otherOctet.length; i++) {
            newOctets[i] = otherOctet[i] | thisOctets[i];
        }
        return new IPAddress(newOctets);
    }

    public long toDecimal(){
        // turn each int into binary string
        String binString = "";
        for (int x : octets){
            //got to pad this:
            binString += padLeft(Integer.toBinaryString(x));
        }
        // turn it into a long:
        return Long.parseLong(binString,2);
    }

    @Override
    public String toString() {
        return StringUtils.implode(this.getOctets());
    }
}

StringUtils for manipulations that did not seem to belong in the other classes:

package com.company;

public class StringUtils {
    public static String padLeft (String input, int count, String pad){
        String output = "";
        for(int c = 0; c < count - input.length(); c++){
            output += pad;
        }
        return output + input;
    }

    public static String padLeft(String input){
        String pad = "0";
        return padLeft(input, 8, pad);
    }

    static String implode(int[] intArray){
        String result = "";
        for (int i = 0; i < intArray.length; i++) {
            int i1 = intArray[i];
            if (i != intArray.length -1 ) {
                result += i1 + ".";
            } else {
                result += i1;
            }
        }
        return result;
    }
}

Finally, The Unit Tests:

package com.company;

import org.junit.Test;

import java.util.ArrayList;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;

public class CidrTest {

    @Test(expected=InvalidIPException.class)
    public void testInvalidLarge() throws Exception {
        IPAddress ipOne = new IPAddress("10.20.33.500");
    }

    @Test(expected=InvalidIPException.class)
    public void testInvalidLetter() throws Exception {
        IPAddress ipTwo = new IPAddress("a.b.c.d");
    }

    @Test(expected=InvalidIPException.class)
    public void testInvalidSmall() throws Exception {
        IPAddress ipThree = new IPAddress("-1.10.-3.-4");
    }

    @Test(expected=InvalidIPException.class)
    public void testInvalidFormat() throws Exception {
        IPAddress ipFour = new IPAddress("20|33|3|0");
    }

    @Test(expected=InvalidIPException.class)
    public void testInvalidCount() throws Exception {
        IPAddress ipFour = new IPAddress("20.33.3.33.55");
    }

    @Test
    public void testCompareIP() throws Exception{
        IPAddress ipOne = new IPAddress("10.20.33.44");
        IPAddress ipTwo = new IPAddress("10.20.33.45");
        assertTrue(ipOne.compareTo(ipTwo) < 0);
    }

    @Test
    public void testIsInRange() throws Exception {
        // an IP is in range if the ip is between hostMin and hostmax
        IPAddress ipThree = new IPAddress("10.20.33.28");
        Cidr cidr = new Cidr("10.20.33.4/24");
        assertTrue(cidr.isInRange(ipThree));
        // null address should never be in range.
        assertFalse(cidr.isInRange(null));
    }

    @Test
    public void testCountHosts() throws Exception {
        Cidr cdr = new Cidr("10.10.15.10/16");
        assertEquals(65534,cdr.countHosts());

        cdr = new Cidr("10.10.1.97/23");
        assertEquals(510,cdr.countHosts());

    }

    @Test
    public void testGetNetMask() throws Exception {
        IPAddress ip = new IPAddress("255.255.0.0");
        assertEquals(ip, new Cidr("10.10.15.10/16").getNetMask());
    }

    @Test
    public void testGetNetworkAddress() throws Exception {
        IPAddress ip = new IPAddress("10.10.0.0");
        assertEquals(ip, new Cidr("10.10.15.10/16").getNetworkAddress());

        IPAddress ip2 = new IPAddress("10.10.0.0");
        Cidr cdr = new Cidr("10.10.1.97/23");
        assertEquals(ip2,cdr.getNetworkAddress());

        IPAddress net3 = new IPAddress("192.168.0.0");
        cdr = new Cidr("192.168.0.3/25");
        assertEquals(net3,cdr.getNetworkAddress());

        IPAddress net4 = new IPAddress("172.16.5.0");
        cdr = new Cidr("172.16.5.34/26");
        assertEquals(net4,cdr.getNetworkAddress());


    }
    @Test
    public void testGetBroadcast() throws Exception {
        // broadcast is inverted netmask ANDED with our network address:
        IPAddress broadcast = new IPAddress("10.10.255.255");
        assertEquals(broadcast,new Cidr("10.10.15.10/16").getBroadcastAddress());
        broadcast = new IPAddress("172.16.5.63");
        assertEquals(broadcast,new Cidr("172.16.5.34/26").getBroadcastAddress());
    }

    @Test
    public void testGetAddress() throws Exception {
        Cidr cdr = new Cidr("10.10.15.10/16");
        IPAddress ip = new IPAddress("10.10.15.10");
        assertEquals(ip, cdr.getAddress());
    }

    @Test
    public void testGetHostMax() throws Exception {
        Cidr cdr = new Cidr("10.10.233.233/23");
        IPAddress min = new IPAddress("10.10.233.254");
        assertEquals(min,cdr.getHostMax());
    }

    @Test
    public void testGetHostMin() throws Exception {
        Cidr cdr = new Cidr("10.10.233.233/23");
        IPAddress min = new IPAddress("10.10.232.1");
        assertEquals(min,cdr.getHostMin());
    }

    @Test
    public void testIpList() throws Exception {
        ArrayList<IPAddress> testArrayList = new ArrayList<IPAddress>();
        testArrayList.add(new IPAddress("10.10.233.233"));
        testArrayList.add(new IPAddress("10.10.233.234"));
        Cidr cdr = new Cidr("10.10.233.233/30");
        assertEquals(cdr.ipList(), testArrayList);
    }
}
share|improve this question

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.
share|improve this answer

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.