How should private functions do parameter validation or user input and internal data structure?

Question

The following code, would return pairs of integers which sum to x, eg: if arr {1, 2, 3, 4, 5] and x is 7 then list should contain {3, 4} and {2, 5}. Main goal is to understand how parameter validation should be performed in private method. Question is nested inside the comments and please restrict suggestions to questions asked only. Thanks for diving in code to check my questions.

public static List<Pair> getPairsFromPositiveArray(int[] arr, int x) {
    // check for all positive integers
    for (int i : arr) {   // if arr is null, then this loop would throw NPE. So no need to make an exclicit check for null.
        if (i < 0) throw new IllegalArgumentException("No integer should be negative.");
    }
    final List<Pair> list = new ArrayList<Pair>();
    getPair(arr, x, list);
    return list;
}

private static void getPair(int[] arr, int x, List<Pair> list) {
    // QUESTION 1: Should check of "all positive integers" be done here too ?

    /*
     * QUESTION 2: 
     * list is data structure which we created internally ( user did not provide it )  
     * Does anyone suggest, it throw an NPE or do an explicit assert check for list != null ? 
     */
    assert list != null;  // form my understanding of e effective java. 
    assert arr != null;   // form my understanding of e effective java. 

    final Set<Integer> set = new HashSet<Integer>();
    /*
     * QUESTION 3:
     * arr is a data structure which was input by the user.
     * Should we check for assert arr != null or let loop throw a NPE ?
     */
    for (int i : arr) { 
        if (set.contains(i)) {
            System.out.println(i + " : ");
            list.add(new Pair(i, x - i));
        } else {
            set.add(x - i);
        }
    }
}

Answer 1

Question 1: I'd do it in the for() loop otherwise you have to loop through twice. But... what is your calling program expecting in such a case?

Question 2: If list is null why not initialize it? If arr is null, then yes there is a problem. Again what do you expect in this case o happen?

Question 3: You check for arr == null per question.

Normally good programming is defensive programming. Always assume bad or wrong inputs provided. On the other hand, for a reasonable simple method you don't necessary want to over complicate things.

Answer 2

QUESTION 1: Should check of "all positive integers" be done here too ?

Ans: Nope; coz you will be calling getPairsFromPositiveArray() before getPair() to get the list

QUESTION 2: * list is data structure which we created internally ( user did not provide it )
* Does anyone suggest, it throw an NPE or do an explicit assert check for list != null ?

Ans: assert

QUESTION 3: * arr is a data structure which was input by the user. * Should we check for assert arr != null or let loop throw a NPE

Ans:

• Always perform sanity check.... so yes, do check for arr != null

• Always prefer assert over throwing NPE

Answer 3

The answers are really straight forward,

public static List<Pair> getPairsFromPositiveArray(int[] arr, int x) 
{       
     // if arr is null, then this loop would throw NPE. So no need to make an explicit check for null.

     //     for (int i : arr) //No need we can check downward 
     //     {  
     //     }
    final List<Pair> list = new ArrayList<Pair>();
    getPair(arr, x, list);
    return list;
}

private static void getPair(int[] arr, int x, List<Pair> list) throws NullPointerException //TELLS THIS WILL THROW NPE
{
    // QUESTION 1: Should check of "all positive integers" be done here too ?

    /*
     * QUESTION 2: 
     * list is data structure which we created internally ( user did not provide it )  
     * Does anyone suggest, it throw an NPE or do an explicit assert check for list != null ? 
     */
    //NO CHECK ARE REQUIRED HERE AS List<Pair> list IS NULL THEN IF WE REASSIGN IT ALSO THEN IT WILL NOT BE REFLECTED BACK TO ORIGINAL, YOU CAN THROW A CUSTOM EXCPETION OR NPE AND DOCUMENT IT (ADD TO METHOD THROWS NPE)   


    final Set<Integer> set = new HashSet<Integer>();
    /*
     * QUESTION 3:
     * arr is a data structure which was input by the user.
     * Should we check for assert arr != null or let loop throw a NPE ?   
     //NO CHECK ARE REQUIRED HERE AS int[] arr IS NULL THEN IF WE REASSIGN IT ALSO THEN IT WILL NOT BE REFLECTED BACK TO ORIGINAL, YOU CAN THROW A CUSTOM EXCPETION OR NPE AND DOCUMENT IT (ADD TO METHOD THROWS NPE)
     */
    for (int i : arr) 
    { 
        if (i < 0) // ANSWER TO QUESTION 1
            throw new IllegalArgumentException("No integer should be negative.");

        if (set.contains(i)) 
        {
            System.out.println(i + " : ");
            list.add(new Pair(i, x - i));
        }
        else 
        {
            set.add(x - i);
        }
    }
}