Velocity
  1. Velocity
  2. VELOCITY-692

have #if handle empty strings/arrays/collections/maps more conveniently

    Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.x
    • Component/s: Engine
    • Labels:
      None

      Description

      An idea from the dev list:
      -------------------------------------------------------------------------------------------------
      On Sat, Feb 7, 2009 at 3:41 PM, <serg472@gmail.com> wrote:
      > Hello,
      > I wanted to share with you a few ideas I have about new simple
      > improvements for DisplayTools. I should be able to make patches for
      > them if you are interested.
      >
      > 1. Add new method
      >
      > isEmpty(object)
      >
      > that will return true if the object is null or empty (for strings it's
      > zero length; for collections, maps and arrays it's zero size). This
      > should help with annoying null checks. (Probably a better place for
      > this method would be Engine, not Tools)

      yeah, not something for tools. would be interesting to have the
      Uberspect pretend that every non-null reference has an isEmpty()
      method, or perhaps just add 0-length strings, empty collections, empty
      maps and 0-length arrays to the list of things that #if( $foo )
      considers false.
      -------------------------------------------------------------------------------------------------

        Activity

        Hide
        Byron Foster added a comment -

        This is probably a good opportunity to address the issue discussed in VELOCITY-656, mainly removing the toString() call on objects.

        What about, #if($obj) returns true if:

        $obj is Boolean and is true
        $obj is an instance of CharSequence and length() returns a non zero value
        $obj has an isEmpty() method that returns false.

        Show
        Byron Foster added a comment - This is probably a good opportunity to address the issue discussed in VELOCITY-656 , mainly removing the toString() call on objects. What about, #if($obj) returns true if: $obj is Boolean and is true $obj is an instance of CharSequence and length() returns a non zero value $obj has an isEmpty() method that returns false.
        Hide
        Nathan Bubna added a comment -

        No, you still need to return true if $obj is a non-null object that doesn't return null (or empty string in this case) when calling toString(). So really, if we changed this (which i suspect we could now that #if( $obj == $null ) works right), then it would be false if:

        $obj is null
        $obj is a false Boolean
        $obj has length() that returns > 0
        $obj has isEmpty() that returns true
        $obj toString() returns string w/length > 0

        (false rules seemed easier to write than true)

        Show
        Nathan Bubna added a comment - No, you still need to return true if $obj is a non-null object that doesn't return null (or empty string in this case) when calling toString(). So really, if we changed this (which i suspect we could now that #if( $obj == $null ) works right), then it would be false if: $obj is null $obj is a false Boolean $obj has length() that returns > 0 $obj has isEmpty() that returns true $obj toString() returns string w/length > 0 (false rules seemed easier to write than true)
        Hide
        Sergiy Kovalchuk added a comment -

        >$obj has length() that returns > 0
        >$obj has isEmpty() that returns true

        Sorry, do you mean checking through reflection if any object has length() or isEmpty() methods?

        Show
        Sergiy Kovalchuk added a comment - >$obj has length() that returns > 0 >$obj has isEmpty() that returns true Sorry, do you mean checking through reflection if any object has length() or isEmpty() methods?
        Hide
        Nathan Bubna added a comment -

        Yep.

        Show
        Nathan Bubna added a comment - Yep.
        Hide
        Sergiy Kovalchuk added a comment -

        I think it is pretty unexpected for a user. It basically means that you are not allowed to use length() or isEmpty() methods in your objects for your needs. If I have length or empty fields returning false or null or " ", it doesn't necessary mean that the whole object should be treated as null or empty. Plus "length" and "empty" are pretty common names, nobody expects that they will become reserved one day for checking if the whole object is empty. Lets say I have Rectangle object with "height" and "length" fields, or Train object with isEmpty method, it doesn't mean that rectangle with length not set or train that is not loaded are equal to null (maybe not the best examples, but you got the idea)

        Even returning false if obj.toString()==null (not to mention length==0) caught me by surprise. I never thought that it is doing this check until saw it in the code. What if I have toString method returning only one field of an object that can be null or ""? I would spend a lot of time trying to figure out why does this object is treated as null in #if statements. At least such behavior should be noted in documentation with big red letters, because it is pretty unexpected I think. toString() should be used however you want and nobody else should rely on it in my opinion.

        I think much safer would be just to check specifically CharSequence, Collection, Map, array (anything else?) for their length/size.

        Show
        Sergiy Kovalchuk added a comment - I think it is pretty unexpected for a user. It basically means that you are not allowed to use length() or isEmpty() methods in your objects for your needs. If I have length or empty fields returning false or null or " ", it doesn't necessary mean that the whole object should be treated as null or empty. Plus "length" and "empty" are pretty common names, nobody expects that they will become reserved one day for checking if the whole object is empty. Lets say I have Rectangle object with "height" and "length" fields, or Train object with isEmpty method, it doesn't mean that rectangle with length not set or train that is not loaded are equal to null (maybe not the best examples, but you got the idea) Even returning false if obj.toString()==null (not to mention length==0) caught me by surprise. I never thought that it is doing this check until saw it in the code. What if I have toString method returning only one field of an object that can be null or ""? I would spend a lot of time trying to figure out why does this object is treated as null in #if statements. At least such behavior should be noted in documentation with big red letters, because it is pretty unexpected I think. toString() should be used however you want and nobody else should rely on it in my opinion. I think much safer would be just to check specifically CharSequence, Collection, Map, array (anything else?) for their length/size.
        Hide
        Sergiy Kovalchuk added a comment -

        On second thought the whole checking for emptiness in #if statement feature looks to me now like not a good idea after all.

        It is nice to be able to write:
        #if($list)
        ...list has elements, display...
        #else
        empty or null, nothing to display...
        #end

        But you won't be able to check specifically for null now, because null and empty are now equal (both false), plus this will not be backward compatible. In the old code you could write:
        #if($list == $null)
        ...list is null... (but also empty now which is unexpected)
        #else if($list.size() == 0)
        ...list has no elements (this will never execute now)
        #else
        ..list is not empty
        #end

        (Until I have bad understanding of how it works)

        What about new operator #ifEmpty() (or #ifNotEmpty())?
        So instead of:
        #if(#list && $list.size() > 0)
        you can write:
        #ifNotEmpty($list)
        ...not empty
        #else
        ...empty
        #end

        Kinda mess

        Show
        Sergiy Kovalchuk added a comment - On second thought the whole checking for emptiness in #if statement feature looks to me now like not a good idea after all. It is nice to be able to write: #if($list) ...list has elements, display... #else empty or null, nothing to display... #end But you won't be able to check specifically for null now, because null and empty are now equal (both false), plus this will not be backward compatible. In the old code you could write: #if($list == $null) ...list is null... (but also empty now which is unexpected) #else if($list.size() == 0) ...list has no elements (this will never execute now) #else ..list is not empty #end (Until I have bad understanding of how it works) What about new operator #ifEmpty() (or #ifNotEmpty())? So instead of: #if(#list && $list.size() > 0) you can write: #ifNotEmpty($list) ...not empty #else ...empty #end Kinda mess
        Hide
        Nathan Bubna added a comment -

        You could always do #if( $foo == $null ) to check for null instead of empty. Personally, i see #if as a shortcut for "falsey" values, and it doesn't seem that odd to me to include empty objects in the "falsey" category. Personally, i think if $rectangle.length() equals 0, i don't see why that should be considered any less empty or falsey than an empty string. If we're going to add "emptiness" to the things that #if considers false, then i would much prefer to use reflection and add all empty things, rather than pick and choose. So for me, the question is simply whether to have the list of falsey things be just:

        null/undefined
        false

        or

        null/undefined
        false
        empty

        heck, i wouldn't even object to treating the number 0 and the string "false" as falsey also. if someone wants to be specific about what falsiness they are checking on, they should always be specific and do:

        #if( $foo == $null )
        #if( $foo == false )
        #if( $foo.length() == 0 )
        #if( $foo.isEmpty() )

        Using #if( $foo ) is always a fuzzy shortcut. I think it's perfectly legitimate to have it be a shortcut for checking emptiness too.

        Show
        Nathan Bubna added a comment - You could always do #if( $foo == $null ) to check for null instead of empty. Personally, i see #if as a shortcut for "falsey" values, and it doesn't seem that odd to me to include empty objects in the "falsey" category. Personally, i think if $rectangle.length() equals 0, i don't see why that should be considered any less empty or falsey than an empty string. If we're going to add "emptiness" to the things that #if considers false, then i would much prefer to use reflection and add all empty things, rather than pick and choose. So for me, the question is simply whether to have the list of falsey things be just: null/undefined false or null/undefined false empty heck, i wouldn't even object to treating the number 0 and the string "false" as falsey also. if someone wants to be specific about what falsiness they are checking on, they should always be specific and do: #if( $foo == $null ) #if( $foo == false ) #if( $foo.length() == 0 ) #if( $foo.isEmpty() ) Using #if( $foo ) is always a fuzzy shortcut. I think it's perfectly legitimate to have it be a shortcut for checking emptiness too.
        Hide
        Sergiy Kovalchuk added a comment -

        >if someone wants to be specific about what falsiness they are checking on, they should always be specific and do:
        >#if( $foo == $null )
        >#if( $foo == false )
        >#if( $foo.length() == 0 )
        >#if( $foo.isEmpty() )

        Yes, but if I understand it correctly if #if operator will check for emptiness, all those expressions will be equal. You can't use #if( $foo == $null ) anymore because it will be the same as #if( $foo.length() == 0 ) (if an object has length) and so on.

        That's why I thought about separated operator for checking for emptiness - so you still can check for null or for length/false/empty specifically if you want using regular #if, and it won't break backward compatibility. Also when a user calls operator named something like #ifEmpty it is clear what he wants, and in this case reflection for empty/length/size methods won't be unexpected.

        Unless I don't get the point and you still will be able to check for nulls even with this empty addition? ($null in Velocity is just a regular variable, not a special keyword, isn't it?)

        Show
        Sergiy Kovalchuk added a comment - >if someone wants to be specific about what falsiness they are checking on, they should always be specific and do: >#if( $foo == $null ) >#if( $foo == false ) >#if( $foo.length() == 0 ) >#if( $foo.isEmpty() ) Yes, but if I understand it correctly if #if operator will check for emptiness, all those expressions will be equal. You can't use #if( $foo == $null ) anymore because it will be the same as #if( $foo.length() == 0 ) (if an object has length) and so on. That's why I thought about separated operator for checking for emptiness - so you still can check for null or for length/false/empty specifically if you want using regular #if, and it won't break backward compatibility. Also when a user calls operator named something like #ifEmpty it is clear what he wants, and in this case reflection for empty/length/size methods won't be unexpected. Unless I don't get the point and you still will be able to check for nulls even with this empty addition? ($null in Velocity is just a regular variable, not a special keyword, isn't it?)
        Hide
        Nathan Bubna added a comment -

        No, #if( $foo == $null ) only checks to see if the RHS equals the LHS. This is different from #if( $foo ) which tries to evaluate $foo to a boolean value. That evaluation process is not used when there are two sides and an operator between them.

        Show
        Nathan Bubna added a comment - No, #if( $foo == $null ) only checks to see if the RHS equals the LHS. This is different from #if( $foo ) which tries to evaluate $foo to a boolean value. That evaluation process is not used when there are two sides and an operator between them.
        Hide
        Nathan Bubna added a comment -

        Sorry, hit submit prematurely. To finish that last paragraph...

        The syntax differentiates the operation. There is no need for a separate directive. At that point, we should just forget it and leave people to do #if( $foo.isEmpty() ), rather than do it for them.

        And yes, it is true there would be a backwards compatibility issue, but it is relatively small. I don't expect many people are (or should be) relying upon #if( $foo ) to distinguish empty lists/strings from non-empty ones. But we would certainly have to make this clear in the changelog and documentation.

        And yes, $null is just a regular variable, albeit with the benefit of (approximately) never being defined, as people don't tend to refer to actual objects with the key "null". They could, but that would be stupid.

        Show
        Nathan Bubna added a comment - Sorry, hit submit prematurely. To finish that last paragraph... The syntax differentiates the operation. There is no need for a separate directive. At that point, we should just forget it and leave people to do #if( $foo.isEmpty() ), rather than do it for them. And yes, it is true there would be a backwards compatibility issue, but it is relatively small. I don't expect many people are (or should be) relying upon #if( $foo ) to distinguish empty lists/strings from non-empty ones. But we would certainly have to make this clear in the changelog and documentation. And yes, $null is just a regular variable, albeit with the benefit of (approximately) never being defined, as people don't tend to refer to actual objects with the key "null". They could, but that would be stupid.
        Hide
        Byron Foster added a comment -

        To clarify my original comment, yes, I meant that #if would also return true if the object is non-null and the other conditions did not fail.

        Anyway, I wonder if including both length() and isEmpty() too wide a net. I propose testing length() only if the object is of type CharSequence, this prevents the call to toString(), but provides similar functionality. In fact, my real interest in this issue is to remove the toString() method call . As far as testing for the general emptiness of containers I could go either way. One is the Python way, the other is the Ruby way, both have merit.

        However, I noticed that equals and not-equals tests also call toString() as in #if($a == $b). So at least this is consistent. Maybe a discussion for another Issue.

        Show
        Byron Foster added a comment - To clarify my original comment, yes, I meant that #if would also return true if the object is non-null and the other conditions did not fail. Anyway, I wonder if including both length() and isEmpty() too wide a net. I propose testing length() only if the object is of type CharSequence, this prevents the call to toString(), but provides similar functionality. In fact, my real interest in this issue is to remove the toString() method call . As far as testing for the general emptiness of containers I could go either way. One is the Python way, the other is the Ruby way, both have merit. However, I noticed that equals and not-equals tests also call toString() as in #if($a == $b). So at least this is consistent. Maybe a discussion for another Issue.
        Hide
        Nathan Bubna added a comment -

        So, i'm still of the opinion that having "empty" values be treated as false by #if( $foo ) is a good idea. so i thought i'd restart this conversation by reminding people that VTL is a display language; it is not meant to be a full scripting language. so, while an empty array or empty map may not be "falsey" to a script, the lack of anything in the array or map to display makes it quite falsey to a display language, no?

        So, to recap, i propose that we change #if in 2.0 to treat the following values as false:

        $obj that is null
        $obj that is boolean false
        $obj that returns false from a getAsBoolean method
        $obj that is empty string ""
        $obj that is empty array
        $obj that returns true from an isEmpty() method (i.e. Collection and Map implementations)
        $obj that returns null or empty string from a getAsString method
        $obj that returns 0 from a length() or size() method
        $obj that returns empty string from toString() method

        i also propose that the order above be the order in which things are checked. This means that while we will still check toString(), it will not ever be called on any maps or lists or any object that bothers to implement a getAsBoolean, getAsString or isEmpty() method. Again to be more clear, if the object has a getAsBoolean(), isEmpty(), getAsString(), length(), or size() method, then we don't keep looking further. Once we find one of these methods, we return false if the result is falsey or true otherwise. This makes it easy for users to avoid expensive calls to toString() or even getAsString().

        For those who want to distinguish between false and falsey or null and empty, they will have to use #if( $obj == $null ) or #if( $obj == false ) to be explicit about what they want to know. So, there is no loss of functionality involved here. It is merely a change to make empty values be treated as false by #if( $foo ) calls and simultaneously make it easy to avoid expensive toString() calls when using #if to test references.

        Thoughts, tirades, minor objections?

        p.s. the getAsString and getAsBoolean support will probably be checked in this evening.

        Show
        Nathan Bubna added a comment - So, i'm still of the opinion that having "empty" values be treated as false by #if( $foo ) is a good idea. so i thought i'd restart this conversation by reminding people that VTL is a display language; it is not meant to be a full scripting language. so, while an empty array or empty map may not be "falsey" to a script, the lack of anything in the array or map to display makes it quite falsey to a display language, no? So, to recap, i propose that we change #if in 2.0 to treat the following values as false: $obj that is null $obj that is boolean false $obj that returns false from a getAsBoolean method $obj that is empty string "" $obj that is empty array $obj that returns true from an isEmpty() method (i.e. Collection and Map implementations) $obj that returns null or empty string from a getAsString method $obj that returns 0 from a length() or size() method $obj that returns empty string from toString() method i also propose that the order above be the order in which things are checked. This means that while we will still check toString(), it will not ever be called on any maps or lists or any object that bothers to implement a getAsBoolean, getAsString or isEmpty() method. Again to be more clear, if the object has a getAsBoolean(), isEmpty(), getAsString(), length(), or size() method, then we don't keep looking further. Once we find one of these methods, we return false if the result is falsey or true otherwise. This makes it easy for users to avoid expensive calls to toString() or even getAsString(). For those who want to distinguish between false and falsey or null and empty, they will have to use #if( $obj == $null ) or #if( $obj == false ) to be explicit about what they want to know. So, there is no loss of functionality involved here. It is merely a change to make empty values be treated as false by #if( $foo ) calls and simultaneously make it easy to avoid expensive toString() calls when using #if to test references. Thoughts, tirades, minor objections? p.s. the getAsString and getAsBoolean support will probably be checked in this evening.
        Hide
        Sergiu Dumitriu added a comment -

        How about 0, either as a Number with that value, or as the result of a getAsNumber method? JavaScript considers 0 as falsey as well.

        Why not put the length() and size() check before getAsString, since length/size are closer in meaning to isEmpty, while getAsString is closer to toString?

        Show
        Sergiu Dumitriu added a comment - How about 0, either as a Number with that value, or as the result of a getAsNumber method? JavaScript considers 0 as falsey as well. Why not put the length() and size() check before getAsString, since length/size are closer in meaning to isEmpty, while getAsString is closer to toString?
        Hide
        Nathan Bubna added a comment -

        No, the order should be about frequency of types, not closeness in meaning to each other. And for that very reason, i've revamped the order (see below). As for 0, i've struggled with that because i know many people might expect 0 to be treated as it is in javascript. However, this is not a scripting language but a display language, and 0 is often a very important thing to display. My inclination is to leave it as non-empty for now. Besides, checking to see if a number is zero takes more processing than you'd think. Anyway, here's the order i am implementing for now...

        #if( $obj ) will be false if:

        $obj is null
        $obj is boolean false
        $obj returns false from getAsBoolean()
        $obj is empty string (CharSequence w/length 0)
        $obj returns true from isEmpty()
        $obj is array of length 0
        $obj returns null from getAsString()
        $obj returns empty string from getAsString()
        $obj returns null from getAsNumber()
        $obj returns 0 from length() or size()
        $obj returns empty string from toString()

        Type-wise, this prioritizes those most likely to be used in #if( $obj) (by my completely subjective experience), as such: booleans, strings, maps/collections, and arrays. then it gives getAsString(), getAsNumber(), length() and size() all opportunities, falling back on toString() as the last resort. So, if you have a custom object likely to end up alone in an #if directive, you'll get the best performance by implementing a getAsBoolean() method, or at least, isEmpty().

        Also, i might add that organizing things this way and considering the various types likely to end up in #if, the length()/size() testing feels a bit unnecessary. What do you guys think?

        Show
        Nathan Bubna added a comment - No, the order should be about frequency of types, not closeness in meaning to each other. And for that very reason, i've revamped the order (see below). As for 0, i've struggled with that because i know many people might expect 0 to be treated as it is in javascript. However, this is not a scripting language but a display language, and 0 is often a very important thing to display. My inclination is to leave it as non-empty for now. Besides, checking to see if a number is zero takes more processing than you'd think. Anyway, here's the order i am implementing for now... #if( $obj ) will be false if: $obj is null $obj is boolean false $obj returns false from getAsBoolean() $obj is empty string (CharSequence w/length 0) $obj returns true from isEmpty() $obj is array of length 0 $obj returns null from getAsString() $obj returns empty string from getAsString() $obj returns null from getAsNumber() $obj returns 0 from length() or size() $obj returns empty string from toString() Type-wise, this prioritizes those most likely to be used in #if( $obj) (by my completely subjective experience), as such: booleans, strings, maps/collections, and arrays. then it gives getAsString(), getAsNumber(), length() and size() all opportunities, falling back on toString() as the last resort. So, if you have a custom object likely to end up alone in an #if directive, you'll get the best performance by implementing a getAsBoolean() method, or at least, isEmpty(). Also, i might add that organizing things this way and considering the various types likely to end up in #if, the length()/size() testing feels a bit unnecessary. What do you guys think?
        Hide
        Nathan Bubna added a comment -

        Ok, the code is in. And i did drop support for length() and size(), since i really can't see that being used or even very useful.

        Show
        Nathan Bubna added a comment - Ok, the code is in. And i did drop support for length() and size(), since i really can't see that being used or even very useful.

          People

          • Assignee:
            Unassigned
            Reporter:
            Nathan Bubna
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development