how to rewrite this function to be more Java 8 with Optionals? Or should I just leave it as it is?
public void setMemory(ArrayList<Integer> memory) {
if (memory == null)
throw new IllegalArgumentException("ERROR: memory object can't be null.");
if (memory.contains(null))
throw new IllegalArgumentException("ERROR: memory object can't contain null value.");
this.memory = memory;
}
You've got a pattern condition -> throw an exception which can be moved to a method:
private void checkOrElseThrow(boolean condition, Supplier<? extends RuntimeException> exceptionSupplier) {
if (condition) {
throw exceptionSupplier.get();
}
}
public void setMemory(List<Integer> memory) {
checkOrElseThrow(memory == null, () -> new IllegalArgumentException("message #1"));
checkOrElseThrow(memory.contains(null), () -> new IllegalArgumentException("message #2"));
this.memory = memory;
}
If the type of the exception is not going to be changed, it's reasonable to pass only the message of the exception (thank @tobias_k for pointing it out):
private void checkOrElseThrow(boolean condition, String exceptionMessage) {
if (condition) {
throw new IllegalArgumentException(exceptionMessage);
}
}
public void setMemory(List<Integer> memory) {
checkOrElseThrow(memory == null, "message #1");
checkOrElseThrow(memory.contains(null), "message #2");
this.memory = memory;
}
If you want to stick to IllegalArgumentException and you have guava on the class path, you could use this:
Preconditions.checkArgument(memory != null,
"ERROR: memory object can't be null.");
Preconditions.checkArgument(!memory.contains(null),
"ERROR: memory object can't contain null value.");
You can't really use Optional here since you want different error messages for different conditions.
If you are OK having a single error message on the other hand, you could do:
this.memory = Optional.ofNullable(memory)
.filter(x -> !x.contains(null))
.orElseThrow(() -> new IllegalArgumentException(
"memory object is null or contains null values"));
For the first case I would use:
Objects.requireNonNull().
I don't think Optional is a way to go here as null is an illegal value.
I usually avoid Optional for such cases as it tends to obscure what's going on.
But first I'd like to mention that the original code lets the caller retain a reference to what is now an internal field memory of the containing class. Maybe you trust your callers not to be malicious, but the caller might accidentally reuse the list passed as an argument. If it does, despite the meticulous argument checking, the memory list might end up containing nulls after all. Or, it could change unexpectedly, leading to other malfunctions.
The solution is to make a defensive copy of the argument list. The straightforward way to do this is as follows:
public void setMemory(ArrayList<Integer> memory) {
if (memory == null)
throw new IllegalArgumentException("memory is null");
List<Integer> temp = new ArrayList<>(memory);
if (temp.contains(null))
throw new IllegalArgumentException("memory contains null");
this.memory = temp;
}
Note that the copy is made and stored in a local variable temp prior to being checked. Obviously, you don't want to store into the field before the list is checked for containing nulls. But the check for containing nulls should be done on the copy, not on the argument list, otherwise, the caller could modify the list after the check but before the copy. (Yes, this is being paranoid.)
If you don't care about the exact exception message, this could be shortened as follows:
public void setMemory(ArrayList<Integer> memory) {
List<Integer> temp;
if (memory == null || ((temp = new ArrayList<>(memory)).contains(null)))
throw new IllegalArgumentException("memory is or contains null");
this.memory = temp;
}
Now this could be rewritten to use Optional:
public void setMemory(ArrayList<Integer> memory) {
this.memory = Optional.ofNullable(memory)
.map(ArrayList::new)
.filter(list -> ! list.contains(null))
.orElseThrow(() -> new IllegalArgumentException("memory is or contains null"));
}
Compared to the usual abuses :-) of Optional I see frequently, this one isn't too bad. The chaining here serves to avoid creation of a local variable, which is a bit of a win. The logic is fairly straightforward, especially if one has Optional on the forebrain. However, I'd be somewhat concerned about revisiting this code in, say, a month. You'd probably have to squint at it a while before convincing yourself it does what you intended it to do.
Finally, a couple general style comments.
The usual preference (at least in the JDK) is to use NullPointerException for these cases. I've stuck with IllegalArgumentException for these examples because that's what the OP is using.
I'd recommend using List<Integer> instead of ArrayList<Integer> for the argument type and possibly the field type. This will enable the use of unmodifiable lists in situations where it's appropriate (e.g., using JDK 9's List.of).
First, it may be a good idea to use the more general list type as input parameter, so change your implementation to:
public void setMemory(List<Integer> memory) {
//stuff
}
and then as others mentioned, checking for null values for every "set" operation is a bit of an overkill.
If this "memory list" comes from some of your code and you can use guava, then maybe use guavas immutable list. This list throws an exception when someone tries to add "null" to your list.
ImmutableList.of( //your Integers)
If you cannot use guava but stillt want to use that approach you could always write your own list implementation that does this null checking for you.
And last, if all of this is not possible for you, just leave your code as is. It is very easy to read and everyone knows what you're doing. Using Optionals can be quite confusing as you can see in other answers here.
Do not use Optionals, they won't benefit you here.
Instead use a more suitable type in place of ArrayList. Storing Integers in collection incurs (un)boxing costs and does not make sense when nulls are not allowed.
Few possible collection libraries, that may suite your needs better:
All of those libraries provide specialized implementations of Lists, Maps and other containers for primitives. Those implementations are generally significantly faster than anything that involves ArrayList<Integer> (unless all integers in your ArrayList are small enough to fit into global Integer instance cache).
As a nice side-effect, using specialized Lists of primitive integers won't allow caller to store nulls by default.
One liner with Optionals:
public void setMemory(ArrayList<Integer> memory) {
this.memory = Optional.ofNullable(memory).map((a) -> Optional.ofNullable(a.contains(null) ? null : a).orElseThrow(() -> new IllegalArgumentException("ERROR: memory object can't contain null value."))).orElseThrow(() -> new IllegalArgumentException("ERROR: memory object can't be null."));
}
Sorry for adding yet another answer, but based on reading comments to the question, there might be even better way to change signature of the method: replace ArrayList<Integer> with IntStream:
public void setMemory(@NonNull IntStream input) {
Objects.requireNonNull(input);
this.memory = ...; // collect the stream into the storage
}
Primitive streams do not incur cost of (un)boxing.
This way you don't have to worry about the caller changing List contents under your feet, and will be able to chose the suitable storage for integers as explained in my other answer (or even resolve the stream contents lazily!).
My solution may be used only if you need two different exceptions and more functional style. But it looks complicated and even longer.
.map(e -> false) maps element of list (integer in this case) to boolean which is required for filter().
this.memory = Optional.ofNullable(memory)
.orElseThrow(() -> new IllegalArgumentException("ERROR: memory object can't be null."))
.stream()
.filter(element ->
Optional.ofNullable(element)
.map(e -> true)
.orElseThrow(
() -> new IllegalArgumentException("ERROR: memory object can't contain null value.")))
.collect(Collectors.toList());
