dev@javaserverfaces.java.net

RE: Re: [REVIEW] Fix for spec issue 172

From: Jason Lee <lee_at_iecokc.com>
Date: Thu, 17 Aug 2006 15:59:52 -0500

I can't comment on the issue tracker, so I'll do it here.
 
The bundle looks good to me, with the exception of the HTML errors in
the javadoc (,/code> instead of </code>). Assuming that's corrected
(minor as it is)
 
r=jdlee
 
-----
Jason Lee
Programmer/Analyst
http://www.iecokc.com <http://www.iecokc.com/>
 


  _____

        From: Roger.Kitain_at_Sun.COM [mailto:Roger.Kitain_at_Sun.COM]
        Sent: Wednesday, August 16, 2006 10:44 AM
        To: dev_at_javaserverfaces.dev.java.net
        Subject: Re: [REVIEW] Fix for spec issue 172
        
        
        I like the verbage and approach.
        I've posted another change bundle.
        
        -roger
        
        Robertson, Tony wrote:
        

                I also don't understand why the
"targetClass.getEnumConstants()" is used at all.
                If you want to check that the "value" really is one of
the enum constants,
                then calling "targetClass.isInstance(value)" is
sufficient.
                Given that all enums are effectively "final" classes
with only private
                constructors, any instance of an enum class must also be
a member of the
                "enumConstants" list.
                This seems to be another case where the javadoc comment
for this interface
                (which is therefore part of the JSF-API spec) is
describing an implementation
                instead of describing a behaviour.

                If you are going to change the javadoc (and therefore
the API spec) I would
                rather see a clear description of the required
"behaviour" only.
                Perhaps something more like this:

                <p>Convert the enum constant given by the
<code>value</code>
                argument into a String. If no target class argument has
been
                provided to the constructor of this instance, throw a
                <code>ConverterException</code> containing the
                {_at_link #ENUM_NO_CLASS_ID} message with proper
parameters.
                If the <code>value</code> argument is <code>null</code>,

                return <code>null</code>.
                If the value is an instance of the provided target
class,
                Return its string value (<code>value.toString()</code>).

                Otherwise, throw a {_at_link ConverterException} containing
the
                {_at_link #ENUM_ID} message with proper parameters.</p>


        
----------------------------------------------------------
                Date: Tue, 15 Aug 2006 15:40:30 -0500
                From: Jason Lee <lee_at_iecokc.com> <mailto:lee_at_iecokc.com>

                Subject: [REVIEW] Fix for spec issue 172

                Just out of curiosity, why use the break, then the test?
Why not do
                this?

                        for (int i=0; i<enumConstants.length; i++) {
                            if (enumConstants[i].equals(value)) {
                                return value.toString();
                            }
                        }
                      
                        // No match found, so throw an exception
                        throw new ConverterException(
                            MessageFactory.getMessage(context,
                                ENUM_ID,
                                value,
                                value,
                                MessageFactory.getLabel(context,
                                component)));

                -----
                Jason Lee
                Programmer/Analyst
                http://www.iecokc.com <http://www.iecokc.com>
                  

> -----Original Message-----
> From: Roger.Kitain_at_Sun.COM
[mailto:Roger.Kitain_at_Sun.COM <mailto:Roger.Kitain_at_Sun.COM> ]
> Sent: Tuesday, August 15, 2006 2:55 PM
> To: dev_at_javaserverfaces.dev.java.net
> Subject: [REVIEW] Fix for spec issue 172
>
>
https://javaserverfaces-spec-public.dev.java.net/issues/show_bug.cgi?id=
172
<https://javaserverfaces-spec-public.dev.java.net/issues/show_bug.cgi?id
=172>
>
> Changes attached to issue.
>