I agree with Adam about the logging issue. The point of the change would be to make it ok to pass null key to the FacesBean so that renderer can save a redundant 'if' to check if the property is supported on the current component. So INFO level would spam the log for nothing.
This suggestion is completely subjective. We cannot even base ourselves on java.util.Map semantics to keep a consistent behavior for users as some Map implementations permits null keys while other don't. I may have another option that might resolve it all. We could have two methods in FacesBeans, .getProperty and something like getPropertyIfSupported. The former would works just like now while the latter would first check for null key. That way, renderers able to work properly even if the property does not exist would use getPropertyIfSupported in their getPropertyName(FacesBean) protected method, and the .getProperty one if the property is mandatory to ensure correct behavior. This last solution would not break existing code in any way and would give an utility method to reduce boilerplate code within renderer code. Regards, ~ Simon On 8/28/07, Adam Winer <[EMAIL PROTECTED]> wrote: > > Logging as INFO seems the worst of all worlds. > Either null property keys are right or wrong. > > If they're wrong, then logging at SEVERE and returning null might be > OK, NullPointerException is also acceptable - which you want depends > mostly on who will see the error. If end users will see the error, > logging at > SEVERE is (often) better. If developers on the project will see it, > throwing > an exception is correct. > > If null PropertyKeys are OK, then logging at FINE or FINER or FINEST > is acceptable - but not at INFO. > > -- Adam > > > > On 8/28/07, Steven Murray <[EMAIL PROTECTED]> wrote: > > This type of thing should be logged but not throw an exception. The > level of error should be INFO. So I like the idea of returning null, > certainly null pointer is awful but throwing an exception is not much of an > improved either. > > > > -- Steve > > > > ________________________________ > > > > From: Adam Winer [mailto:[EMAIL PROTECTED] > > Sent: Tue 8/28/2007 4:52 PM > > To: MyFaces Development > > Subject: Re: [Trinidad] Should FacesBean.getProperty(null) really throw > an exception? > > > > > > > > I'd personally rather have it throw an exception - it makes it harder to > > catch errors if nulls are ignored in general. > > > > -- Adam > > > > > > On 8/28/07, Simon Lessard <[EMAIL PROTECTED]> wrote: > > > Hello all, > > > > > > Currenty, FacesBean.getProperty(null) throws a NullPointerException > from the > > > checkNotListKey method call. However, I feel it should rather return > null > > > and not throw an exception. That way, we would no longer have to use > the > > > following code snippet in our renderer to know if the property is > supported > > > on any given component: > > > if (someKey == null) > > > { > > > return null; > > > } > > > else > > > { > > > return ComponentUtils.resolveSomeType(bean.getProperty(someKey), > > > someDefaultValue); > > > } > > > > > > If getProperty were to return null instead of throwing an exception, > only > > > return line would be needed, reducing some boilerplate code in the > various > > > renderers. > > > > > > Any objection to make that change to FacesBeanImpl and reflect it in > the > > > FacesBean's javadoc? > > > > > > > > > Regards, > > > > > > ~ Simon > > > > > > > > > > > >
