Got it, thank you Vyom, and I added some javadoc to LookupFactoryBase (per comments from another thread) in new webrev as below
http://cr.openjdk.java.net/~xyin/8208483/webrev.02/ Thanks & Regards, Chris > On 6 Aug 2018, at 8:38 PM, vyom tewari <vyom.tew...@oracle.com> wrote: > > > > 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
