On Monday 06 August 2018 03:02 PM, Chris Yin wrote:
Hi, Vyom
Many thanks for your review and comments, inline and updated webrev as below,
thanks
http://cr.openjdk.java.net/~xyin/8208483/webrev.01/
On 6 Aug 2018, at 4:12 PM, vyom tewari <vyom.tew...@oracle.com> wrote:
Hi Chris,
1-> DirAFactory.java, "getIbjectInstance" returns "null" if it fails to construct object. I will
suggest you to throw some "RuntimeException" instead returning null. If you return null then caller of
"getObjectInstance" had to check for null and it will end in lots of boilerplate code.
‘getObjectInstance’ methods are override from DirObjectFactory and
ObjectFactory, per my understanding on javadoc, return null should indicate
object cannot be created and allow other factories can be tried, feel free to
let me know if I misunderstand something on the api doc, thanks
Paste a few javadoc here against ‘getObjectInstance’ method as below
* @return The object created; null if an object cannot be created.
* @exception Exception If this object factory encountered an exception
* while attempting to create an object, and no other object factories are
* to be tried.
Yes you are right, we have to live with null.
Vyom
2-> DirTxtFactory.java same as "DirFactory.java”.
Same as above
3-> In LookupWithAnyAttrProp/LookupWithAttrProp java create a constant for
"1.2.4.1" and put one liner comment if possible. This is will help future
maintainer to understand why we are comparing with this value.
Sure, created constant for “1.2.4.1” and put comment to explain that it’s pre
defined attribute value of ‘A’, thanks
One generic comment, in most of the places i can see, you declared functions to throw
generic exception "Exception", please change it to specific exception or list
of specific exceptions if possible.
Fixed, those generic exception are generated automatically through override
method, I removed those unused throws Exception from xxxFactory.
Thanks & Regards,
Chris
Thanks,
Vyom
On Monday 30 July 2018 02:08 PM, Chris Yin wrote:
Please review the changes to add 5 JNDI tests to com/sun/jndi/dns/FactoryTests/
in OpenJDK, thanks
bug: https://bugs.openjdk.java.net/browse/JDK-8208483
<https://bugs.openjdk.java.net/browse/JDK-8208483>
webrev: http://cr.openjdk.java.net/~xyin/8208483/webrev.00/
<http://cr.openjdk.java.net/~xyin/8208483/webrev.00/>
Regards,
Chris
