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);
        }
    }
}

~"form my understanding of e". You need to check your grammar. — IgorGanapolsky, Feb 24 '14 at 16:43

Mitesh Pathak · Answer 1 · 2013-12-20T07:07:36.990

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

edited Dec 20 '13 at 07:07

answered Dec 20 '13 at 06:59

Mitesh Pathak

1,306
10
14

1. not sure what your are looking ? I am looking for the check for all positive integers that is performed in the public function. 2. "so make sure you initialize the objects or handle it", well my questions is "what if i get a null" we all know to never pass null, but bugs happen. 3. When you say "prefer exception handling myself" are u suggesting and NPE over assert ? – JavaDeveloper Dec 20 '13 at 07:02
Its always better to perform sanity checks – Mitesh Pathak Dec 20 '13 at 07:03
There are 2 ways to do it - use asserts or use NPE, whats your take ? – JavaDeveloper Dec 20 '13 at 07:03
assert: I would recommend you never throw NPE by yourself. – Mitesh Pathak Dec 20 '13 at 07:05

score 0 · Answer 2 · edited Dec 20 '13 at 06:52

0

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.

edited Dec 20 '13 at 06:52

Deepak Bhatia

6,230
2
24
58

answered Dec 20 '13 at 06:49

robnick

1,720
17
27

I do not understand your questions 1. you would do what in for loop ? I am using a for loop too ? My calling code is expecting list of pairs of numbers that add to x. 2. you should not initialize a list, its a method param, it can contain some values already in it. What do you mean by what i expect to happen ? 3. You check for arr == null per question. ? does it mean you suggest using asserts ? – JavaDeveloper Dec 20 '13 at 06:54
I am guessing English is not your first language... sorry if I got you confused. 1. Do the checks in your existing for() loop, rather than have a second loop. 2. Okay don't initialise the list... there was no indication it could contain existing values. So do an assert. Adding method summary comments indicating what you expect really help. 3. Yes. – robnick Dec 23 '13 at 02:27

score 0 · Answer 3 · answered Dec 20 '13 at 07:15

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);
        }
    }
}

I am not sure what you mean by THEN IF WE `REASSIGN`. I am not talking about reassigning anything in any question I asked ? — JavaDeveloper, Dec 20 '13 at 07:20
Just adding the more information that if we check for null and reassign the variable then too the changes will not be reflected back to original array or list.... — Deepak Bhatia, Dec 20 '13 at 07:23

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

3 Answers3