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

[David Gobbi]

The point that Brad King brought up is that this actually will not
work.  It's very bad to have a Set method in a derived class return a
different type than in the base class.  And it would break the Java
wrappers, because in Java this is not only bad, but illegal.  So I'm
out completely.

   David



On Fri, Jan 29, 2010 at 10:04 AM, Michael Halle <mhalle at bwh.harvard.edu> wrote:
> 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
>>>
>>>
>>
>>
>>
>
> _______________________________________________
> 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
>
>