IPv4 filter in Java

Question

I have a simple class for representing IPv4-addresses via int values, and a simple IP-address filter that works like a set of IPv4-addresses.

com.github.coderodde.util.net.IPv4Address.java:

package com.github.coderodde.util.net;

import java.util.regex.Matcher;
import java.util.regex.Pattern;

/**
 * This class implements IPv4 addresses.
 * 
 * @author Rodion "rodde" Efremov
 * @version 1.6 (Mar 21, 2023)
 * @since 1.6 (Mar 21, 2023)
 */
public class IPv4Address {
    
    private final int addressInt;
    private String addressString;
    private static final String ADDRESS_REGEX = 
            "[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}";
    
    private static final Pattern ADDRESS_PATTERN = Pattern.compile(ADDRESS_REGEX);
    
    public IPv4Address(String addressString) {
        checkAddressString(addressString);
        this.addressInt = toInt(addressString);
        this.addressString = toAddressString(addressInt);
    }
    
    @Override
    public String toString() {
        return addressString;
    }
    
    @Override
    public boolean equals(Object o) {
        IPv4Address other = (IPv4Address) o;
        return addressInt == other.addressInt;
    }

    @Override
    public int hashCode() {
        return addressInt;
    }
    
    private static String toAddressString(int address) {
        StringBuilder stringBuilder = new StringBuilder();
        
        int b0 = address & 0xff;
        int b1 = (address & 0xff00) >> 8;
        int b2 = (address & 0xff0000) >> 16;
        int b3 = (address & 0xff000000) >>> 24;
        
        stringBuilder.append(b3)
                     .append(".")
                     .append(b2)
                     .append(".")
                     .append(b1)
                     .append(".")
                     .append(b0);
        
        return stringBuilder.toString();
    }
    
    private static int toInt(String addressString) {
        addressString = addressString.trim();
        checkAddressString(addressString);
        String[] addressStringParts = addressString.split("\\.");
        int b0 = Integer.parseInt(addressStringParts[3]);
        int b1 = Integer.parseInt(addressStringParts[2]);
        int b2 = Integer.parseInt(addressStringParts[1]);
        int b3 = Integer.parseInt(addressStringParts[0]);
        return (b3 << 24) | (b2 << 16) | (b1 << 8) | b0;
    }
    
    private static void checkAddressString(String addressString) {
        Matcher matcher = ADDRESS_PATTERN.matcher(addressString);
        
        if (!matcher.matches()) {
            throw new IllegalArgumentException(
                    "The input string '" 
                            + addressString 
                            + "' is definitely an IPv4 address.");
        }
        
        String[] parts = addressString.split("\\.");
        
        int b0 = Integer.parseInt(parts[0]);
        int b1 = Integer.parseInt(parts[1]);
        int b2 = Integer.parseInt(parts[2]);
        int b3 = Integer.parseInt(parts[3]);
        
        checkByte(b0, "Byte " + b0 + " is not within the range [0, 255].");
        checkByte(b1, "Byte " + b1 + " is not within the range [0, 255].");
        checkByte(b2, "Byte " + b2 + " is not within the range [0, 255].");
        checkByte(b3, "Byte " + b3 + " is not within the range [0, 255].");
    }
    
    private static void checkByte(int byteInt, String exceptionMessage) {
        if (byteInt > 255) {
            throw new IllegalArgumentException(exceptionMessage);
        }
    }
}

com.github.coderodde.util.net.IPFilter.java:

package com.github.coderodde.util.net;

import java.util.HashMap;
import java.util.Map;

/**
 * This class implements a IPv4-address filter.
 * 
 * @author Rodion "rodde" Efremov
 * @version 1.6 (Mar 21, 2023)
 * @since 1.6 (Mar 21, 2023)
 */
public class IPFilter {

    private final Map<String, IPv4Address> table = new HashMap<>();
    
    public void add(IPv4Address address) {
        table.put(address.toString(), address);
    }
    
    public void add(String addressString) {
        table.put(addressString, new IPv4Address(addressString));
    }
    
    public void remove(IPv4Address address) {
        table.remove(address.toString());
    }
    
    public void remove(String addressString) {
        table.remove(addressString);
    }
    
    public boolean contains(IPv4Address address) {
        return table.containsKey(address.toString());
    }
    
    public boolean contains(String addressString) {
        return table.containsKey(addressString);
    }
    
    public int size() {
        return table.size();
    }
    
    public boolean isEmpty() {
        return table.isEmpty();
    }
}

com.github.coderodde.util.net.IPv4AddressTest.java:

package com.github.coderodde.util.net;

import static org.junit.Assert.assertEquals;
import org.junit.Test;

public final class IPv4AddressTest {
    
    @Test
    public void test() {
        IPv4Address a = new IPv4Address("201.6.80.22");
        assertEquals("201.6.80.22", a.toString());
        
        a = new IPv4Address("123.5.117.95");
        assertEquals("123.5.117.95", a.toString());
    }
}

com.github.coderodde.util.net.IPFilterTest.java:

package com.github.coderodde.util.net;

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

public final class IPFilterTest {
    
    @Test
    public void test() {
        IPFilter filter = new IPFilter();
        
        assertTrue(filter.isEmpty());
        assertEquals(0, filter.size());
        
        filter.add("111.222.33.4");
        
        assertFalse(filter.isEmpty());
        assertEquals(1, filter.size());
        
        assertTrue(filter.contains(new IPv4Address("111.222.33.4")));
        assertTrue(filter.contains("111.222.33.4"));
        assertFalse(filter.contains("111.222.33.5"));
        
        filter.remove("111.222.33.4");
        
        assertTrue(filter.isEmpty());
        assertEquals(0, filter.size());
    }
}

Critique request

Is there anything to improve? Tell me, please, anything.

Answer 1

The IPFilter class The implementation appears to be simple. The IPFilter class includes methods for adding and removing IPv4 addresses from the filter, as well as methods for determining whether an IPv4 address is contained in the filter, determining the size of the filter, and determining whether the filter is empty. Nevertheless, the implementation of the filter itself is not optimum for space utilisation, since it utilises a HashMap<String, IPv4Address> to hold the addresses. Each address is thus saved twice: once as a key and once as a value. Instead, a HashSetIPv4Address>, which only saves each address once, would be a more space-efficient solution.

Class IPv4AddressTest The test appears to cover the IPv4Address class's fundamental functions, such as creating an IPv4 address from a string, validating the string representation of an IPv4 address, and testing the equals() method. Nevertheless, the test might be enhanced by include more test cases, such as ensuring that incorrect IPv4 addresses generate an error.

Generally, the implementation appears to be clean and simple, although there are several areas for imprvement, such as the IPFilter class's space efficiency and the handling of errors in the IPv4AddressTest class.

Answer 2

Often we want sensible collation in a text file or DB table.

I will touch on this topic below.

---

IPv6 is clearly way out of scope for this ATM. But I assume that if this class gets used much, IPv6 will eventually creep into scope. So I have an eye on it being the topic of some future sprint.

Rejecting loops in favor of manual unroll seems to be the biggest concern.

---

Both classes should have stronger javadoc sections that "sell" me on what each class offers and why I should use it.

I will grant you that Inet4Address has some weird details, such as accepting class-B numbers in "128.net.host" format. If a detail like that is what motivated re-inventing this particular wheel, then please explicitly write down the reason.

---

    private final int addressInt;
    private String addressString;

Oh, wow! 32-bit signed representation. Well, life is full of compromises, didn't want to blow 64 bits on it, I guess. And I can see where a 4-byte representation may be less convenient. Though if you tackle IPv6, that won't fit in an int.

It's not too late to turn the unboxed integer into your custom object which enforces 32-bit unsigned range. But I can understand if you choose not to.

The redundant string seems like more than I would expect. We're representing the identical thing two ways, and must keep them in sync. Absent some very specific performance requirement, I recommend deriving the string each time caller asks for it.

---

    private static final String ADDRESS_REGEX = 
            "[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}";
...
        if (!matcher.matches()) {
            throw new IllegalArgumentException(

Tiny style nit. This code is correct. Most high level languages have good regex support these days. If you collaborate with multi-language engineers, consider prefering java .find() over .matches() and explicitly throw in the ^ caret / $ dollar anchors. Spelling it out can save the Gentle Reader a trip to the man page to verify the semantics. Also, no biggie, consider using more compact notation:

            "^([0-9]{1,3}\\.){3}[0-9]{1,3}$";

I will grant you that simple repetition of four verbose octet expressions does have a certain charm; I have an eye on parallel structure with a future IPv6 regex.

Perhaps we're not using \d{1,3} due to some locale concern about "٤.٣.٢.١", "一个.两个.三个.四个" or similar?

Thank you very much for the .parseInt() / .checkByte() approach, I really appreciate it. I have seen too many silly expressions that try verify the 255 limit via a complex regex.

And I do appreciate that we .compile() just once, thank you.

The four calls to checkByte, with four temp vars, is not "too much". But do consider using a loop, again with an eye on IPv6.

Splitting on . dot, plus .parseInt(), does a fair job of validating the address. Maybe simplify the regex so it's just verifying character set, like no "-" minus sign?

---

                    "The input string '" 
                            + addressString 
                            + "' is definitely an IPv4 address.");

Typo. Author's Intent was "not an IPv4".

---

I note with pleasure that parsing either

• "10.0.1.255" or
• "010.000.001.255"

works out identically. Consider explicitly documenting this, encouraging callers to rely on it as part of the contract.

Collation order can really matter to this class's consumers.

---

toAddressString is wonderfully clear, thank you. Again, maybe use a loop?

nit: Pretty sure that b3 + "." + b2 ... would compile down to the same StringBuilder calls you wrote out explicitly.

Of course, with a loop you might want to hang onto that.

---

    private static int toInt(String addressString) {
        addressString = addressString.trim();

The .trim() is lovely, but surprising, in that the ctor doesn't do it. DRY, isolate this behavior to just one place, so caller doesn't trip over inconsistent API details.

        checkAddressString(addressString);

Sigh, that's a 2nd (redundant) check, for the ctor code path. Not the end of the world. But something we could eliminate.

Kudos on very clear code. Consider using a loop.

---

 * This class implements a IPv4-address filter.
 ...
public class IPFilter {

I guess I'm slightly surprised that generics / inheritance couldn't address the caller's needs without adding this class. I know, composition is good. And so is boilerplate?

The big change from HashMap appears to be substituting .add for the idiomatic .put. Unclear on the need for that; consider *// documenting it.

We might use this class to filter IPs. But it appears to me it is more of a container. Consider revisiting the name.

---

Yay, it comes with tests! Brilliant, thank you.

I note with pleasure that "201.6.80.22" pushes us into negative territory, while other addresses verify that positive ints also work properly.

However, I have my doubts that it compares nicely with "123.5.117.95". Or rather, absent a spec the code does what it does and is "correct", though caller might be surprised at the result.

I don't see an implements Comparable ... public int compareTo().

I wouldn't mind seeing verification that ArrayList .sort() works well with this class. Include both loopback and "128.1.2.3" addresses in the dataset.

This submission contains no OpenClover stats or other coverage measurements. I agree with @jimmy90 that exercising the throw would be useful. Running coverage during unit tests would have highlighted that omission so author could remedy it prior to requesting review.

I agree with @Zahid Khan that the class should be final, and that the test suite really needs to include an ipv4 == null test, which is Green. The target code will need to change to make that happen.

Incorporating a maven dep on nl.jqno.equalsverifier would let us automatically notice the equals() contract violation:

@Test
public void equalsHashCodeContracts() {
    EqualsVerifier.forClass(IPv4Address.class).verify();
}

---

All of this code impresses me as mostly doing the Right Thing in a clear, correct, maintainable way, with some minor caveats:

1. redundant string should maybe go, recomputing it when needed
2. signed 32-bit representation might pose testing surprises for a future maintainer
3. using loops would reveal parallel structure with a (not yet tackled) IPv6 class

That said, let me wander off into feature requests.

(1.) Collation order often matters, for example when merging a pair of /usr/bin/sort'ed address lists. This class might choose to trivially add one or more formatters that complement .toString():

• withZeros --> "010.000.001.020" (verbose)
• asHex --> "0a000114" (compact, plus it might want a ctor)

(2.) We see a HashMap here, but doing .sort() on an ArrayList of IPs would seem a natural use of this class.

(3.) An (IPv4Address, maskLen) tuple named IPv4Prefix would be a natural extension to this, especially to support filtering.

---

This code achieves its design goals.

I would be happy to delegate or accept maintenance tasks on this codebase.

Answer 3

I'm reviewing the IPV4 addresses class for now. But may review further.

1. You can add @see tag and add a reference to IPV4 addresses.

    * This class implements IPv4 addresses.
    * 
    * @author Rodion "rodde" Efremov
    * @version 1.6 (Mar 21, 2023)
    * @since 1.6 (Mar 21, 2023)
 

2. You are not complying with the Object's equal and hashCode contract. So, this code will have a lot of issues when used with the collection framework.

   For any non-null reference value x, x.equals(null) must return false .

   link : https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/Object.html