Thank you, Vyom Regards, Chris
> On 8 Aug 2018, at 3:12 PM, vyom tewari <vyom.tew...@oracle.com> wrote: > > Hi Chris, > > Latest code webrev(.03) looks good to me, expending initContext() makes tests > more readable :) . > > Thanks, > > Vyom > > > On Wednesday 08 August 2018 09:11 AM, Chris Yin wrote: >> 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 >
