On Mon, Feb 6, 2017 at 9:43 AM, Michael Osipov <micha...@apache.org> wrote: ...
> 4) check for empty objects by class: >> - return whether the collection is empty for a Collection object >> >> 5) check object for an isEmpty() method, if so return whether it >> returned false >> > > Why explicitly check for collection if you do #isEmpty()? > I'd either add Map to the explicit check or drop collection altogether and > rely on #isEmpty() solely. makes sense to me. > 7) check object for a length() or size() method, if so return whether it >> returns 0, but I agree with Alex Fedotov that we could skip those >> methods if we already took care for strings and collections. >> > > What happened to array#length? You completely missed that out. > I would not drop #length() and #size(), you'd ultimately fail with > javax.naming.directory.Attribute#size() or > javax.naming.directory.Attributes#size(). > There are likely more examples to have this. i don't think it hurts to keep them, most users won't often get that far, i think. 8) If directive.if.tostring.check = false, stop here and return true (*) >> >> 9) check object for a toString() method, and return whether the string >> is non-null and non-empty >> >> The 6th step won't be reached very often... >> >> (*) the old configuration parameter was directive.if.tostring.nullcheck, >> but for consistency with the new behavior regarding empty strings, my >> proposal is to rename it like this. I don't consider that backward >> compatibility is important since all collections are handled above in >> the chain. >> > > 9) is somewhat confusing because #toString() is never null unless you > override it and return a custom string. Moreover, how will a #toString() > guarantee you that an object is logically empty? It can't, see > javax.naming.directory.SearchResult#getAttributes(). > > At best, this would be false by default in 2.0. ... I still back this check. Velocity is for templating, text output, i.e. a display language. Velocity-specific classes (including some VelocityTools) have had cause in the past to return null or empty strings specifically because of this check and that toString() is regularly central to rendering objects. While it cannot guarantee emptiness for every object out there, it is a sensible check. The goal here is not perfection, but to catch common cases, and due to history, i believe this is a common one.
