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

[Pau Arumi]

En/na Hernán Ordiales ha escrit:
> On 7/9/07, Pau Arumi <parumi at iua.upf.edu> wrote:
>> 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.
> 
> ok, added

Commited it as is (in rev 10371). However I'd like more changes
--it's a matter of details. Tell me if you prefer me to take it.

> void testInControl_doesntHaveDefaultValueBydefault()
>     {
>         CLAM::InControl inControl("inControl");
>         CPPUNIT_ASSERT_EQUAL( false, inControl.HasDefaultValue() );
>     }
>     void testInControl_defaultValue()
>     {
>         CLAM::InControl inControl("inControl");
>         CPPUNIT_ASSERT_EQUAL( 0.0f, inControl.DefaultValue() );
>     }
>     void testInControl_settingDefaultValue()
>     {
>         CLAM::InControl inControl("inControl");
>         inControl.SetDefaultValue(5.f);
>         CPPUNIT_ASSERT_EQUAL( 5.f, inControl.DefaultValue() );
>     }
>     void testInControl_hasDefaultWhenTrue()
>     {
>         CLAM::InControl inControl("inControl");
>         inControl.SetDefaultValue(0.f);
>         CPPUNIT_ASSERT_EQUAL( true, inControl.HasDefaultValue() );
>     }


It's good to see developers starting (I assume) to be used to
unit-testing. In this case, however, I think it doesn't cover the
"interesting" case. When you have set bounds and also set a
default, the set default prevails.

Actually I would not add a HasDefaultValue method because the
semantics are not so clear: note that we are using DefaultValue()
even when not HasDefaultVale(). So we always have default, but its
value depends on the situation.

My definitions would be: DefaultValue() *is* the bounds mean unless
another default value have been fixed with SetDefaultValue()
Note that this reduce the needed testing to only one (the one
described before: "set default prevails to bounds")

Yes, that means that I'm for eliminating the 0 as default for not
bounded controls. Thus default would always be the bounds mean. It
seems a simpler behaviour to me. If a processing needs a specific
default then it should define it. If don't agree explain your
reasons. I'm always ready to get convinced.

About the method doc:
+       /** Returns default value if exists. If don't and bounds 
were set, returns the mean between them or 0 as general default */
         TControlData DefaultValue() const;

If we agree on my proposal it would be something like :
/** Returns the bounds mean or the value set with SetDefaultValue() 
if its the case */


>> 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 */
> 
> yep ( i added doxygen comments previously in others patchs ;-) )
> added too

Oh yes indeed (momentarily lapsus).

Cheers!
pau