[vtk-developers] chaining vtk Set... methods

[Michael Halle]

Thanks for everyone's comments, and thanks for the pointer to the 
previous discussion -- sorry I missed it.

I'm not that worried about the question of debugging.  Set... methods 
are generally relatively simple, and those that aren't still modularize 
state changes.  If anything makes debugging difficult, it would probably 
be the macro implementation of vtkSet..., and that's a long-established 
and useful part of VTK.

It might be a reasonable rule or thumb that if you can't figure out 
which Set... method is mucking up your object's internal state, you 
probably should refactor.

If that's the rule, then all Set methods (macro-defined or not) could 
adapt to this pattern.  I think that would be the best result for users, 
because they don't know what's defined by macro and what isn't.  I would 
suggest limiting the change to all Set and maybe all Add methods.  If 
that works, individual implementers could return "this" from other 
methods, but it wouldn't be guaranteed (you'd have to check the return 
value, just like you would with any other method use).

The alternative idea to modify Set... methods to return a boolean 
whether or not they succeed is a valid one, but it would be far more 
invasive and would require more manual editing (particularly for custom 
methods that may do more than just alter a private or protected 
variable).  It might just be too late in VTK's life cycle to make such a 
change.

In contrast, the huge majority of the edits required to implement the 
setter chain proposal could be made automatically.

As for the question, 'does all the setter chain idea do is get rid of 
"object->Set...; object->Set...; object->Set...;"' pattern?', the answer 
is yes.  As I said before, the change gets rid of redundancy and extra 
typing, forms natural code blocks, and reduces the risk of cut and paste 
errors.

The pattern is widely used in other libraries and has a logical 
interpretation (i.e., we're not overloading -> to do anything strange; 
in fact, we're not using operator overloading at all).

Finally, the proposal does no harm.  You can still use the traditional 
syntax if you wish, depending on context.  Since the semantics are 
basically the same, it's barely even "two ways of doing the same thing". 
It's more like the difference between:

actor->GetProperty()->SetDiffuse(1, 1, 1);
actor->GetProperty()->SetAmbient(0, 0, 0);

// and
vtkProperty *prop = actor->GetProperty();
prop->SetDiffuse(1, 1, 1);
prop->SetAmbient(0, 0, 0);

One might be preferable to the other in some cases, but they are both 
valid, both intuitive, and both can co-exist in the same code base 
without dissonance.

--Mike


On 1/29/10 11:19 AM, Francois Bertel wrote:
> Hello,
>
> I'm against that. See other comments about the "Declarative API":
>
> http://vtk.uservoice.com/forums/31508-general/suggestions/361302-declarative-api?ref=title
>
> On Fri, Jan 29, 2010 at 10:45 AM, Michael Halle<mhalle at bwh.harvard.edu>  wrote:
>> All this talk of vtkNew and vtkSmartPointer has got me thinking.
>>
>> One of the elements of VTK syntax that has always bothered me is the
>> redundancy of multiple "object->SetXXX();" method calls in typical
>> applications.  You end up with a long list of "object->Set...;" lines.  That
>> quite redundant. It's also error prone, since cutting and pasting code might
>> result in an "object2->..." being added when "object->..." was intended.
>>
>> The fix, as far as I know, is actually relatively simple (if not highly
>> pervasive): change the return value of SetXXX() methods to be "this".
>>
>> I don't know why I didn't think about it years ago, when the change would
>> not have been as major.
>>
>> // Compare (old way):
>>
>>   vtkProperty *property = vtkProperty::New();
>>   property->SetColor(1.0, 1.0, 1.0);
>>   property->SetDiffuse(0.7);
>>   property->SetSpecular(0.4);
>>   property->SetSpecularPower(20);
>>
>> // new way:
>>
>>   vtkNew<vtkProperty>  property;  // or old-style New() call
>>   property->
>>     SetColor(1.0, 1.0, 1.0)->
>>     SetDiffuse(0.7)->
>>     SetSpecular(0.4)->
>>     SetSpecularPower(20);
>>
>> // or
>>   actor->GetProperty()->
>>     SetColor(1.0, 1.0, 1.0)->
>>     SetDiffuse(0.7)->
>>     SetSpecular(0.4)->
>>     SetSpecularPower(20);
>>
>>
>> I believe the change to be backwards compatible (since SetXXX methods
>> invariably return "void" now), and also compatible with the scripting
>> wrappers.  You get natural indentation, reduced verbosity without loss of
>> clarity, and the ability to pick and choose between old and new syntax.
>>
>> Perhaps I missed a discussion about this idea sometime in the history of
>> VTK, but are there technical downsides to this scheme?  The only major
>> problem I see is that the new calling syntax can't be used with unmodified
>> third party libraries that still return void instead of "this".
>>
>> Insane?
>>
>> --Mike
>> _______________________________________________
>> Powered by www.kitware.com
>>
>> Visit other Kitware open-source projects at
>> http://www.kitware.com/opensource/opensource.html
>>
>> Follow this link to subscribe/unsubscribe:
>> http://www.vtk.org/mailman/listinfo/vtk-developers
>>
>>
>
>
>