[Clam-devel] [PATCH] (work in progress) Harmonizer

[Pau Arumi]

En/na Hernán Ordiales ha escrit:
> hi, i'm sending another little patch:
> 
> * Minor improvements on Harmonizer Do() (for example i had forgotten
> to follow ignore residual option to SMSPitchShifting i was only
> skipping some processing)
> * I've added the new SetDefault() function to InControls and also
> adapted it in ProcessingBox and Canvas classes (and already applied it
> in many processings)

Nice! Commited on revision 10365.
However, I'd appreciate if you can send these 2 changes in a new patch

1) In CLAM we use "early returns" whenever possible, which 
simplifies the conditionals and prevent having nested blocks

>  TControlData InControl::DefaultValue() const
>  {
> -	return (mUpperBound+mLowerBound)/2;
> +	if (mHasDefault)
> +		return mDefault;
> +	else
> +	{
> +		if (mBounded)
> +			return (mUpperBound+mLowerBound)/2;
> +		else
> +			return 0.;
> +	}
>  }

So it'd look like:

if (mHasDefault) return mDefault;
if (!mBounded) return 0;
return (mUpperBound+mLowerBound)/2;

2) Control api is something very "core" and so it needs to be
covered by unit tests. So please, try adding simple tests that
checks the behaviour of SetDefault() in
CLAM/test/UnitTests/ControlsTests/ControlsTest.cxx
Actually unit tests not only assess that code is working, but also
is a means for doing api specification.

While we are on it, you could also doxygen document the
DefaultValue() method which is not totally obvious. Do you know
whow to do it? hint: /** doc */

3) DefaultValue() vs SetDefault()
Probably I suggested the SetDefault name but now seeing the getter
and setter names I feel that both should end with "Value"
(SetDefaultValue). And maybe we should also rethink the getter name
but not sure about this.

pau