Throw an early exception to prevent divide by zero error (#2481)

[jatin69]

Closes #2481

Added a try catch block to throw a length error exception to report that at least 2 elements are required in array to perform the operation.

[taketwo]

Why not less than 1?

[jatin69]

Because standard deviation needs division by n and n-1 as done in this function's last few lines. and for n-1 to be atleast 1, n needs to be atleast 2.

[taketwo]

You don't need division to compute standard deviation of a single number, it's simply zero.

[jatin69]

Yes. But the code does not handle it. I just tested and stddev is nan when a single number is passed. I wonder how did that skip testing. I'll update the code to handle it.

[taketwo]

What's the point of throwing an exception and immediately catching it?

[jatin69]

I did not know if any top-level hierarchy is catching any exceptions or not, so I had to catch it here itself. Although I did forget to add a return statement after printing the error message. I'll add it.
But even that leaves the function call in a weird result state. What will the user expect when he passes an empty array? Should I find where this function is called and add a catch block there itself?
Something else on your mind?

[SergioRAgostinho]

The idea it to let it blow up for the user. That way he'll need to sanitize values before passing it to this method.

[jatin69]

oh. If you want to blow up, why throw an exception at all? Aren't exceptions caused to be handled?
Anyway, I'll assume you have a use case in mind for expecting such a behavior.
So, in short,
You want to display a message saying atleast 2 values and then crash, yes?

[SergioRAgostinho]

oh. If you want to blow up, why throw an exception at all? Aren't exceptions caused to be handled?

Yes but, to be handled by the user ('s code). We have no way of guessing what he wants to do in case he doesn't have a sanitized input. We just want to make sure that he is explicitly dealing with it.

You want to display a message saying atleast 2 values and then crash, yes?

Yes.

[jatin69]

Hi, sorry it took a long time for such a trivial change. I'm not a frequent contributor.
Please let me know if this needs any more changes.

[SergioRAgostinho]

In your error message: there's a typo with "at least".

@taketwo should we throw a pcl derived exception, like the ones present in here?

[jatin69]

I'll update this, once you settle on which exception to use.

[taketwo]

Well there is BadArgumentException which sort of fits, we may use it. Though I feel there is no consistency within PCL regarding exceptions.

[jatin69]

throw pcl::BadArgumentException("Input array must have at least 2 elements.", NULL, "getMeanStd");

is this alright?

[taketwo]

I'd use PCL_THROW_EXCEPTION following the example here:

pcl/common/include/pcl/exceptions.h

Lines 45 to 49 in bf61b88

	/** PCL_THROW_EXCEPTION a helper macro to be used for throwing exceptions.
	* This is an example on how to use:
	* PCL_THROW_EXCEPTION(IOException,
	* "encountered an error while opening " << filename << " PCD file");
	*/

[jatin69]

PCL_THROW_EXCEPTION(BadArgumentException, "Input array must have at least 2 elements "); 

This will do? I thought mentioning the filename will barely make any sense here, as it's just a utility function.

[taketwo]

Yes. But please note my other comment regarding the minimum size of the array.

[jatin69]

Yes. I commented on that thread. Please see to it.

[jatin69]

I handled a missing base case of input size 1 and added an exception for input size 0. Please let me know if is alright and I'll squash commits.

[taketwo]

Thank you, looks good. Please check a few minor comments. We'll also need to format to PCL style (e.g. add spaces between function/macro/if and opening braces) before we can merge this.

[taketwo]

Perfect, thank you.

[jatin69]

@taketwo Thank you 😄