Java – Checking preconditions the proper way

designdesign-patternsjavaobject-orientedobject-oriented-design

I have a class with around 1300 lines and it has many CRUD-like methods that need parameters to be checked, for some of them it's more than just a few rules.

For clarity purposes, I am going to use generic names for my class and methods.

I finished implementing all the preconditions check recently, and although I have been doing it the cassical way with if block at the beginning of the methods and similar procedures, I ended up having kind of repeated chunks (which I tried extracting into private methods but that didn't help completely) and a class that is alright but being tidy and clear as I am I really want to restructure it.

So, this is a very succint version of my class, so you get the idea of how it looks like:

public class Event {
    public void setAttribute1(List<Attribute1> attribute1) {
        if (attribute1 == null)
            throw new IllegalArgumentException("Attribute1 cannot be null");

        if (attribute1.isEmpty())
            throw new IllegalArgumentException("Attribute1 cannot be empty");

        // check for repeated elements

        // check for more things

        this.attribute1 = attribute1;
    }

    public void addAttribute1(Attribute1 attr) {
        // null check

        // check not existing

        // check for more things

        attribute1.add(attr);
    }

    public void removeAttribute1(Attribute1 attr) {
        // null check

        // check not existing

        attribute1.remove(attr);
    }

    public void setMyMap(Map<K, V> myMap) {
        // null check

        // keys check

        // values check

        // more and more

        this.myMap = myMap;
    }

    // a long etc...
}

I end up with huge, occasionally partially repeated chunks of precondition checks. This is untidy and confusing. Especially when I have many similar methods for the same attribute, where I have the same checks, except that they are not the same. In case the example above doesn't help understand what I mean, this would be a case that repeats a lot in my class:

public class Event {
    private List<K> kDomain;
    private List<V> vDomain;
    private int kConfig;

    public void setMap(Map<K, Set<V>> map) {
        // null check

        // all k in K should be in kDomain

        // the length of the map should match an arithmetical operation based on the value of kConfig
        // (and similar checks)

        // a null Set<V> associated to a K should be illegal

        // for each k, each v in V should be in vDomain
    }

    public void addVtoK(K k, V v) {
        // null check k and v

        // k must be in kDomain

        // v must be in vDomain

        // if k has already a set of Vs, v must not be in that set already (already existing object check)
    }

    public void addVsToK(K k, Set<V> vs) {
        // null check k and vs

        // k must be in kDomain

        // all v in vs should be in vDomain

        // no v in vs can already be associated to k
    }
}

As you can see I have long checks that look very similar, but they are not the same.

I am trying to look for best ways to restructure my whole class which I am pretty sure I am going to end up rewriting.

I have looked into Guava Preconditions and it looks neats, but although it'd be an enhancement to the code, there are certain preconditions to be checked that are a tiny bit more complex to process and need deeper design thinking.

What would be the a proper approach? Or I am overthinking my issue?

Best Answer

Whilst it's admirable to check your preconditions, I wonder:

  1. is your class usage such that you'll need to check all these ? e.g. I will check preconditions for a set of components that are exposed for more general usage, but for more limited usage in which I know how the component will be used, I won't perform so many checks. The counter argument to this is that if you extract a component for more widespread use (e.g. in a library) then you'll have to apply more preconditions, but it's a pragmatic approach
  2. Are all of these checks really necessary ? e.g. if I specify an empty Set of elements to be added, do I really care that nothing will get added ? I don't want my client code to be littered with empty Set checks when adding to this entity. Just let the entity deal with it (i.e. do nothing)

If, despite the above, you have masses of repeated code for your preconditions, maybe check out Java's dynamic proxies. These allow you to write a wrapper method that will wrap all method invocations on your object. You can then check if the method you're calling is a setXYZ(), and if you're adding a set of attributes, or just one, and you can invoke the preconditions generically before delegating further to your real implementation.

Related Topic