Just one more minor revision to expand initContext() into test method for easier read, new webrev as below, thanks
http://cr.openjdk.java.net/~xyin/8208483/webrev.04/ Regards, Chris > On 7 Aug 2018, at 5:41 PM, Chris Yin <xu.y....@oracle.com> wrote: > > Hi, Vyom > > Thanks a lot for your comment, I didn’t realize it, sure, I moved this > constant to LookupFactoryBase as you suggested and also abstract the same > verify logic into base class as ‘verifyLookupObjectAndValue’, hope that looks > better, update new webrev as below, thanks > > http://cr.openjdk.java.net/~xyin/8208483/webrev.03/ > > Regards, > Chris > >> On 7 Aug 2018, at 4:35 PM, vyom tewari <vyom.tew...@oracle.com> wrote: >> >> Hi Chris, >> >> Overall latest code looks good to me, one minor comment did you consider >> moving "private static final String ATTRIBUTE_VALUE = "1.2.4.1";" in >> "LookupFactoryBase" ? as both LookupWithAnyAttrProp & LookupWithAttrProp >> inherit "LookupFactoryBase". >> >> .Thanks, >> >> Vyom >> >> >> 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. >>> >>>> 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 >> >
