Don't return null for functions returning collections/iterables


  • 6

    Usually when a method returns a collection and the method has no result to return, we have two options:

    Return null: This results in breakage of client code if it doesn't check for that null.

    public List<Order> getOrders() {
    	//..
    }
    .
    .
    List<Order> orders = getOrders();
    for(Order order : orders) { //NullPointerException here if getOrders returned null!
    	System.out.println(order);
    }
    

    So the client will be forced to write:

    if(orders != null) { //guard
    	for(Order order : orders) { //NullPointerException here if getOrders returned null!
    		System.out.println(order);
    	}
    }
    

    Return an empty list: Client code will not break and no need to introduce a guard! Much cleaner code! So the below code will suffice:

    List<Order> orders = getOrders();
    for(Order order : orders) {
    	System.out.println(order);
    }
    

    EDIT : Best way to return an empty list is to return one that is immutable by returningCollections.emptyList() from the concerned function.

    Note: The above example dealt with lists but you can extend this discussion to anything that is a Collection/Iterables.


  • 0
    D

    Great question. This is straight out of Clean Code.


  • 0

    @derek18 Yep, hell of a book that..


  • 0
    B

    Returning null for that method is deceiving and perhaps equivalent to breaking a promise. By looking at the method signature, calling code assumes that it is going to return List<Order> but when it encounters null, it wasn't really prepared to handle that. If a method is supposed to return null as things may go wrong, the signature should be Option<List<Order>> instead. Same goes true for exception(and for any side effects). Instead of throwing an exception and altering control flow, it should be wrapped in an error so calling application can decide how best to handle the situation. The return type in that case would be Either<Error, List<Order>>. With this signature, possibility of returning an error is explicit and visible to clients of this code.


  • 0

    @bool2 In cases involving exceptions, advertising that the method raises an exception (checked exception) would be better design. That way the client code can keep the error handling code separate (in a catch block) instead of having to unwrap/parse from <Error, List<Order>>.


  • 0
    B

    @soumyadeep2007 Checked exceptions are certainly more expressive and explicit than the code block arbitrarily throwing unhandled exceptions. The point of wrapping value into a context(Monad) is to later use it with combinators without unwrapping it. For instance your example above can be written as orders.map(order -> System,out.printn(order)).

    It works whether the list is null or not. The value needs to be unwrapped when code wants to transform to a different value when value is different.


  • 0

    @soumyadeep2007 said in Don't return null for functions returning collections/iterables:

    Best way to return an empty list is to return one that is immutable by returningCollections.emptyList() from the concerned function.

    How would you use that to make for example this function better?

    private static List<Integer> range(int start, int stop, int step) {
        List<Integer> result = new ArrayList();
        for (int i = start; i < stop; i += step)
            result.add(i);
        return result;
    }
    

    (demo here)


  • 0

    @StefanPochmann

    private static List<Integer> range(int start, int stop, int step) {
        List<Integer> result = new ArrayList();
        for (int i = start; i < stop; i += step)
            result.add(i);
        return result.size() == 0 ? Collections.emptyList() : result;
    }
    

    This simply enhances code readability a little, compared to your solution. Only for scenarios in which the list returned has to be modified, we can't use the above approach, as Collections.emptyList() returns an immutable. Your solution is perfectly fine of course as it guarantees an empty list instead of null.


  • 0

    @soumyadeep2007 Hmm... how is that supposed to enhance readability? It requires extra reading and extra thinking, and it's unnatural.

    Besides readability, it's also worse because making the empty list immutable when every other result is mutable is again creating a special case. Similar to returning null. If the caller wants to modify the list, they now need add extra code for it, similar to the extra code for null.

    P.S. Why result.size() == 0 instead of result.isEmpty()?


  • 0

    @StefanPochmann Yes I see your reasoning. We should not be creating special cases. And my bad, I should be using isEmpty. If we want to return an immutable list, we would simply do:

    private static List<Integer> range(int start, int stop, int step) {
        List<Integer> result = new ArrayList();
        for (int i = start; i < stop; i += step)
            result.add(i);
        return Collections.unmodifiableList(result);
    }
    

    I will update my post. Thanks for your insight!


  • 0

    @soumyadeep2007 Yeah, if you for some reason do want immutability, then I guess that's a reasonable way. Just some further thoughts:

    • It's only shallow immutability. The list's elements can still be mutable (demo). Not the case in my example with Integers, of course.
    • Apparently not even Collections::unmodifiableList bothers to check for emptiness and using Collections::emptyList when it could. Not sure why, I guess the authors don't think it's worth it.
    • Its documentation talks about a use case: "allows modules to provide users with "read-only" access to internal lists". And really that's the only real reason I can imagine. That it's the producer of the list who's interested in immutability. If it were the caller who's interested in immutability, they should probably call it themselves instead. But I can't imagine why a caller would want immutability.
    • Overall the only use case I see for Collections::emptyList is when a producer wants to return an immutable list and its "regular" code can't handle some cases where the result will be empty. Then it might be good to start with something like if (..specialcase..) return Collections.emptyList();. Could also be done for optimization, i.e., if the "regular" code can handle the special case and would return an empty list in the end anyway, but we can detect this early on and just return an empty list right away. Though I really dislike that. I've seen many people do that and I'm not sure I ever thought it wasn't bad. Because it adds code, suggests that the "regular" code can't handle the case, and only makes one or few special cases faster while making all other cases slower. So usually it's more code, misleading, and achieves the opposite of the intended optimization.

  • 0

    @StefanPochmann hey thanks again for your eye-opener!

    1. Coudn't agree more with your first point. Indeed it is shallow immutability. Here's an example to supplement yours.
    class MyClass {
    	int a;
    	@Override
    	public String toString() {
    		return a + "";
    	}
    }
    
    public class Solution {
    	public static void main (String[] args)	{
    		List<MyClass> list = new ArrayList<>();
    		
    		MyClass obj = new MyClass();
    		obj.a = 5;
    		list.add(obj);
    		List<MyClass> unmodList = Collections.unmodifiableList(list);
    		
    		obj.a = 7;
    		System.out.println(list); //O/P : [7]
    		System.out.println(unmodList); // O/P : [7]
    		
    	}
    }
    
    1. I completely subscribe to the view that code should not be littered with sanity checks. However, I could form somewhat an argument to your last point for a certain use case : What about base cases in recursive functions? Isnt it better or even convention to put up a base case at the beginning with a conditional check and a return?
      As for all other cases, your are spot on: benefits of such optimization are often overestimated:
      few special cases faster while making all other cases slower.

  • 0

    @soumyadeep2007 said in Don't return null for functions returning collections/iterables:

    What about base cases in recursive functions? Isnt it better or even convention to put up a base case at the beginning with a conditional check and a return?

    Maybe. Can you show an example?


  • 0

    @StefanPochmann For example this is a nice convention from the book : The Algorithm Design Manual that I follow for backtracking problems:

    Backtrack-DFS(A, k)
    	if A = (a1, a2, ..., ak) is a solution, report it.
    	else
    		k = k + 1
    		compute Sk
    		while Sk != null do
    			ak = an element in Sk
    			Sk = Sk − ak
    			Backtrack-DFS(A, k)
    

    An example:

    public class Solution {
        public List<List<Integer>> permute(int[] nums) {
            Set<Integer> numSet = Collections.newSetFromMap(new ConcurrentHashMap<Integer, Boolean>());
            for(int num : nums) {
                numSet.add(num);
            }
            List<List<Integer>> result = new ArrayList<>();
            List<Integer> solutionVector = new ArrayList<>();
            
            permute(numSet, solutionVector, result);
            
            return result;
        }
        
        private void permute(Set<Integer> numSet, List<Integer> solutionVector, List<List<Integer>> result) {
            if(isSolutionFound(numSet)) {
                processSolution(solutionVector, result);
            }
            
            for(Integer num : numSet) {
                makeMove(numSet, num, solutionVector);
                permute(numSet, solutionVector, result);
                undoMove(numSet, num, solutionVector);
            }
        }
        
        private boolean isSolutionFound(Set<Integer> numSet) {
            return numSet.isEmpty();
        }
        
        private void processSolution(List<Integer> solutionVector, List<List<Integer>> result) {
            List<Integer> permutation = new ArrayList<>();
            for(Integer choice : solutionVector) {
                permutation.add(choice);
            }
            result.add(permutation);
        }
        
        private void makeMove(Set<Integer> numSet, Integer choice, List<Integer> solutionVector) {
            numSet.remove(choice);
            solutionVector.add(choice);
        }
        
        private void undoMove(Set<Integer> numSet, Integer choice, List<Integer> solutionVector) {
            numSet.add(choice);
            solutionVector.remove(solutionVector.size() - 1);
        }
    }
    

  • 0

    @soumyadeep2007 Hmm, I don't see what your example has to do with my last point or even with your argument you mentioned. You're not using Collections::emptyList and you don't have code for base cases returning early.


  • 0

    @StefanPochmann Ok I see that there has been sort of a miscommunication. I thought you were talking generically with respect to the if special case.. construct. Now I understand that you were talking about the specific case.


  • 0

    @soumyadeep2007 Well, I'd say I did have a more general situation in mind, but I still don't think we disagree. I guess you mean the if-part here, right?

    private void permute(Set<Integer> numSet, List<Integer> solutionVector, List<List<Integer>> result) {
        if(isSolutionFound(numSet)) {
            processSolution(solutionVector, result);
        }
        
        for(Integer num : numSet) {
            makeMove(numSet, num, solutionVector);
            permute(numSet, solutionVector, result);
            undoMove(numSet, num, solutionVector);
        }
    }
    

    That then rather falls in the ""regular" code can't handle" category, as the "regular" code (the for-loop) doesn't do what the if-part does (processing the solution). So then you actually have less special treatment than I talked about, because I said to return (with the appropriate result, though in this case the appropriate side-effect). You don't return there. You still run into the loop. Which loops over nothing, so does nothing, so it's ok. That's how you stop the recursion. Not with extra code. So in a sense, you just gave an example of not having extra code for the special case :-)


  • 0

    @StefanPochmann I see what you are saying with respect to special treatment and returns. Thanks again!


Log in to reply
 

Looks like your connection to LeetCode Discuss was lost, please wait while we try to reconnect.