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?
12
Whilst it’s admirable to check your preconditions, I wonder:
- 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
- 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.
3
It is the responsibility of the method to validate its own inputs. It should do so thoroughly and rigorously, regardless of whether or not you are building a library. There’s rarely (imo) a good reason to skip on proper input validation. Handling edge cases as part of the natural set of inputs if they return a meaningful result is acceptable (e.g. letting empty sets iterate 0 times over a loop).
Validating parameters for each method independently can involve some repetition if similar validation happens in different methods. You can be OK with that repetition:
DRY is over-rated – it almost always creates tighter coupling between processes, and in this context could also effectively introduce coupling, indirectly between your (assumedly) otherwise independent methods. If you were to refactor this in a way that validates the parameters centrally, then you’ll make multiple methods dependent on validator method(s). I would not always recommend that approach, it can quickly explode in combinatorial complexity, especially if then later you find that your validator methods depend on other methods and/or later call many other methods… etc. Then you have a trivial piece of code (the validator) has become embedded deep within the call hierarchy and can become very hard to refactor to a less spaghetti-like architecture.
The parts that can be OK to DRY in this context are single responsibility, re-used, non-trivial (but not huge) pieces of logic that are likely to remain fairly simple “leaf” methods of the call tree over time. Do not however, apply DRY to simple checks e.g. checking for not null. It’s a balance between code simplicity and maintainability on the one hand and not creating too much tight coupling that could entrench inflexibility in future.
2