[dna-issues] [JBoss JIRA] Commented: (DNA-102) setProperty method in SequencerOutput interface should enforce limitations on property value types

[Greg Haber (JIRA)]

    [ http://jira.jboss.com/jira/browse/DNA-102?page=comments#action_12415585 ] 
            
Greg Haber commented on DNA-102:
--------------------------------

If we really have to support Object property values, then I don't think it would be a good idea to have overloaded methods for the different concrete types, since this would be confusing as to which method would be called.  This is especially true since we are using the varargs syntax (Object...) - in fact the doc at http://java.sun.com/j2se/1.5.0/docs/guide/language/varargs.html specifically warns against this.

But, why do we have to support Object property values,when whatever the sequencer puts in SequencerOutput for a property value needs to be something that DNA can put into a property value of a javax.jcr.Node, and it doesn't look like either JCR 1.0 or JCR 2.0 supports arbitrary objects as Node property values?

Since the values can't really be arbitrary objects, the main advantage of having separate interface methods for each concrete type is of course that you would then see any typing errors at compile-time rather than run-time (when we throw an exception), which will make developing sequencers a bit easier. 

A secondary advantage is that it is a more definitive contract between the API and the developer, than having an Object method where we manually list all of the allowed types in the Javadoc.  As a developer, when I see this sort of manual list, I have a lot less trust that such a manual list is accurate, than if I see separate explicit methods, and I would probably be a little more cautious in using this API.  Of course, there's no real reason why the implentor of the interface would have done any better job in implementing individual methods correctly vs. a single method that takes Object, this is purely a question of developer perception.

I would also like to question why we are using varargs syntax here for setProperty and setReference, rather than having explicit methods for Object and Object[].  Obviously there is the advantage of convenience in the short term, in that the syntax Object value1, Object value2 in a method call will be automatically converted by varargs in a single Object[].  But I worry that there are long term maintenance issues here - we tie our hands as to future enhancement of this interface (and limit our flexibility in building implementations of this interface) by limiting future overloading of these methods because of our use of varargs.  And furthermore, the 1.5.0 doc (http://java.sun.com/j2se/1.5.0/docs/guide/language/varargs.html) warns of the varargs "As an API designer, you should use them sparingly, only when the benefit is truly compelling."

Now, just throwing a run-time exception as requested in the original JIRA description is perfectly fine - just putting in my two cents that I think a more dramatic overhaul of this interface such as mentioned in DNA-77 (removing the existing methods and replacing them entirely with non-varargs methods for the appropriate types) would be even better.  



> setProperty method in SequencerOutput interface should enforce limitations on property value types
> --------------------------------------------------------------------------------------------------
>
>                 Key: DNA-102
>                 URL: http://jira.jboss.com/jira/browse/DNA-102
>             Project: DNA
>          Issue Type: Bug
>          Components: API
>    Affects Versions: 0.1, 0.2
>            Reporter: Greg Haber
>            Priority: Minor
>
> The setProperty method of the SequencerOutput method, as it currently exists, accepts arbitrary single Objects or arrays of Objects as property values via its use of "Object..." in the method signature.  In DNA-77, it was suggested that the current method definition should be replaced with multiple type-specific methods, but it was decided to not do this, and instead recommend use of ValueFactory to create the values used in this method.
> The problem with this is that, by leaving the interface as is and having use of ValueFactory be optional, it is easy for users to set properties to value types that are not currently supported (meaning that they will see a run-time error somewhere else in the code in these cases, rather than a compile-time or run-time error at the actual point where the issue was introduced (their misuse of setProperty).
> One possible way to enforce this in the interface is to define the interface as throwing some sort of exception if the value is not an instance of an allowed class.  Another solution would be a more substantial revision of the interface to mandate use of ValueFactory to create property values.

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: http://jira.jboss.com/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira