groovy
  1. groovy
  2. GROOVY-4253

findResult object/collection/map enhancement patch

    Details

    • Type: Improvement Improvement
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.7.5, 1.8-beta-2
    • Component/s: None
    • Labels:
      None
    • Testcase included:
      yes
    • Patch Submitted:
      Yes
    • Number of attachments :
      1

      Description

      Groovy find and findAll take the result of the passed in closure and use groovy truth to either find the first element in the collection/map or filter the collection/map.

      There are many situations situations where what you actually want to find or collect is the result of the closure, rather than test original elements and then reprocessing them all again.

      The attached patch (with javadocs & unit tests) adds 2 methods to groovy objects, collections, and maps; enhanced versions of the existing groovy find and findAll methods:

      <object/collection/map>.findResult(closure)
      <object/collection/map>.findAllResults(closure)
      

      findResult and findAllresults are valuable for shortening/clarifying code in many situations, but they're the most valuable when the closure calls a relatively expensive method (or one that isn't idempotent). When calling the method again with the results of a find/findAll is impractical.

      I had a discussion today on twitter with Robert Fletcher (http://twitter.com/rfletcherEW/status/16410798707) that spurred this patch, though there have been many times in the past that I've wanted this functionality and ended up writing the 4+ lines of boilerplate code to work around it being missing.

      Details:

      The object/collection version of findAll iterates through the object and passes each value to the closure. The first closure result that is non-null (including false) will short circuit evaluation and return the value, example:

      // works on lists/collections
      assert "RAB" == ["foo", "bar", "baz"].findResult {
          def uCaseReversed = it.reverse().toUpperCase()  // proxy for expensive method that we don't want to do more often than necessary
          if (uCaseReversed ==~ /..B/) return uCaseReversed
      }
      
      // and objects with iterators like ranges
      assert "Found 4" == (1..10).findResult {
          if (it > 3) return "Found $it"
      }
      

      The map version of findResult is slightly different as what it returns is a Map.Entry with the key equal to the key of the map entry that generated a non-null value from the closure:

      // closure has 0/1 parameters, it passes the current Map.Entry in as it iterates
      [a: 1, b: 2, c: 3, d: 4].findResult { 
          if (it.value > 2) return "I found ${it.key}:${it.value}"
      }.with {
          assert "c" == it.key
          assert "I found c:3" == it.value
      }
      
      // closure has 2 parameters, it passes the key and value in as it iterates
      [a: 1, b: 2, c: 3, d: 4].findResult { key, value ->
          if (value > 2) return "I found ${key}:${value}"
      }.with {
          assert "c" == it.key
          assert "I found c:3" == it.value
      }
      

      findAllResults will spin through the whole iterator, but will collect only those results from the closure that are non-null, so it can both collect and filter in one step:

      assert ["Found 4", "Found 5"] == (1..5).findAllResults {
          if (it > 3) return "Found $it"
      }
      

      The map version returns a map that contains the keys that had a non-null result with the result of the closure:

      def origMap = [a:1, b:3, c:5]
      origMap.findAllResults { if (it.value >= 3) return "Found ${it.key}:${it.value}" }.with { mappedResults ->
          assert 2 == mappedResults.size()
          assert "Found b:3" == mappedResults.b
          assert "Found c:5" == mappedResults.c
      }
      

      Other groovy iterators (like collect and inject) are not able to easily fulfill this role. Collect will not do any filtering, but instead returns a full list with nulls that need to be removed. Inject can sort of be made to fill this role, but it's often more work than it's worth as you need to manage the list creation/appending and the additional closure parameter. There isn't an equivalent of collect/inject on Maps.

      I'd love it if this could be added to 1.7.4, but 1.8 would work too .

        Issue Links

          Activity

          Hide
          Ted Naleid added a comment - - edited

          After some discussion on twitter, the map version of this might be a little screwy. I was trying to mimic the existing map.find and map.findAll in what they returned, but the user would have access to both the key and value in the closure so if they really wanted a Map.Entry they could create it for themselves. This current version probably violates the principle of least surprise.

          If others agree, I could refactor the patch tomorrow to switch the map version to match the object/collection version.

          There's also been some discussion about the name findResult and findAllResults. I'm open to other suggestions, it was the best thing that I could think of that implied both finding and transforming. I'm not aware of a similar construct in other languages (I checked ruby/python/clojure/scala and didn't see anything) that might give a common name that is used for this kind of method.

          Show
          Ted Naleid added a comment - - edited After some discussion on twitter, the map version of this might be a little screwy. I was trying to mimic the existing map.find and map.findAll in what they returned, but the user would have access to both the key and value in the closure so if they really wanted a Map.Entry they could create it for themselves. This current version probably violates the principle of least surprise. If others agree, I could refactor the patch tomorrow to switch the map version to match the object/collection version. There's also been some discussion about the name findResult and findAllResults. I'm open to other suggestions, it was the best thing that I could think of that implied both finding and transforming. I'm not aware of a similar construct in other languages (I checked ruby/python/clojure/scala and didn't see anything) that might give a common name that is used for this kind of method.
          Hide
          blackdrag blackdrag added a comment - - edited

          I am wondering about the usage a bit.. say we have a list of n entries and we define f(liston.find

          {f(it)!=null}

          ), having the same result as your findResult, then the major difference for this is, that the function f is applied one more time here, than in with findResult. Looking at O(n) vs. O(n+1) function calls, I see no reason to have this. It makes more sense if the result will be in the first elements, then you have a low n, and the n+1 makes a difference. But still the biggest ddifference would be O(1) vs O(2). Usually, if f is very expensive, then I split f into two parts, one that tells me if f should be applied and one that does the actual transformation.

          So what I would like to see are some (nonabstract) use cases as of how this method will be a real help to someone. You said there had been some twitter discussions... then it should be easy to give a few examples that helps us to see the true value of this method.

          Show
          blackdrag blackdrag added a comment - - edited I am wondering about the usage a bit.. say we have a list of n entries and we define f(liston.find {f(it)!=null} ), having the same result as your findResult, then the major difference for this is, that the function f is applied one more time here, than in with findResult. Looking at O(n) vs. O(n+1) function calls, I see no reason to have this. It makes more sense if the result will be in the first elements, then you have a low n, and the n+1 makes a difference. But still the biggest ddifference would be O(1) vs O(2). Usually, if f is very expensive, then I split f into two parts, one that tells me if f should be applied and one that does the actual transformation. So what I would like to see are some (nonabstract) use cases as of how this method will be a real help to someone. You said there had been some twitter discussions... then it should be easy to give a few examples that helps us to see the true value of this method.
          Hide
          Ted Naleid added a comment -

          Jochen, you're correct that calling

          f( list.find { f(it) != null } )
          

          is equivalent in many cases and that this

          list.findResult { f(it) }
          

          is mostly just a more readable version. The situations where it really matters is when f() is either expensive (or of unknown expense), or if calling f() has side effects that are not safe to repeat in a second call (not idempotent).

          One type of problem that findResult is useful for is when implementing a chain of responsibility type pattern, where the collection we're iterating over is a list of closures/objects where each of them is given a chance to handle a request till one of them finds an answer.

          This is the problem that spurred this patch in the first place on twitter. Rob Fletcher was implementing an url rewriting using a set of rules, each of which could potentially handle the url and return the rewritten url (http://bit.ly/anf4EL). The return value from each rule is the rewritten rule. Here was his solution:

          String rewrite (String url) {
              for (rule in rules) {
                  def rewrittenUrl = rule.handle(url)
                  if (rewrittenUrl) return rewrittenUrl
              }
              return url
          }
          

          With this patch, he would have been able to use:

          String rewrite (String url) {
              rules.findResult { rule.handle(url) } ?: url
          }
          

          Which I think is much more readable and maintainable.

          Here's another example from the grails code base that is currently required to use a "for" loop with a local variable

          // from: BuildSettings.groovy
          
          private getFirstPropertyValue(List<String> propertyNames, Properties props, String defaultValue) {
              def value
              for(String p in propertyNames ) {
                  value = getValueFromSystemOrBuild(p, props)
                  if(value!=null) break
              }
              return value ?: defaultValue
          }
          

          With findResult, it could be turned into:

          private getFirstPropertyValue(List<String> propertyNames, Properties props, String defaultValue) {
              propertyNames.findResult { getValueFromSystemOrBuild(it, props)  } ?: defaultValue
          }
          

          From the groovy codebase, using the map version of findAllResults this:

          // ConfigObject.groovy
          private convertValuesToString(props) {
              def newProps = [:]
              for(e in props) {
                  newProps[e.key] = e.value?.toString()
              }
              return newProps
          }
          

          becomes:

          private convertValuesToString(props) {
              props.findAllResults { it.value?.toString() }
          }
          

          A few other examples that come to mind that could use findResult or findAllResults are:

          • a List of paths/file handles, grep each file in the list with a regular expression with wildcards, return the file and matched text in the first one hit
          • You have a grails service that can take an url, download the web page and extract contact information. With a list of URLs, you could use urls.findAllResults { webService.extractContactInfo(url) }

            to get all the contact info aff all the web pages

          • implementing grails-like UrlMappings, similar to the rewrite rules above where the first url mapping that matches returns the controller/action to forward to urlMappings.findResult { it.handle(url) }
          Show
          Ted Naleid added a comment - Jochen, you're correct that calling f( list.find { f(it) != null } ) is equivalent in many cases and that this list.findResult { f(it) } is mostly just a more readable version. The situations where it really matters is when f() is either expensive (or of unknown expense), or if calling f() has side effects that are not safe to repeat in a second call (not idempotent). One type of problem that findResult is useful for is when implementing a chain of responsibility type pattern, where the collection we're iterating over is a list of closures/objects where each of them is given a chance to handle a request till one of them finds an answer. This is the problem that spurred this patch in the first place on twitter. Rob Fletcher was implementing an url rewriting using a set of rules, each of which could potentially handle the url and return the rewritten url ( http://bit.ly/anf4EL ). The return value from each rule is the rewritten rule. Here was his solution: String rewrite (String url) { for (rule in rules) { def rewrittenUrl = rule.handle(url) if (rewrittenUrl) return rewrittenUrl } return url } With this patch, he would have been able to use: String rewrite (String url) { rules.findResult { rule.handle(url) } ?: url } Which I think is much more readable and maintainable. Here's another example from the grails code base that is currently required to use a "for" loop with a local variable // from: BuildSettings.groovy private getFirstPropertyValue(List<String> propertyNames, Properties props, String defaultValue) { def value for(String p in propertyNames ) { value = getValueFromSystemOrBuild(p, props) if(value!=null) break } return value ?: defaultValue } With findResult, it could be turned into: private getFirstPropertyValue(List<String> propertyNames, Properties props, String defaultValue) { propertyNames.findResult { getValueFromSystemOrBuild(it, props) } ?: defaultValue } From the groovy codebase, using the map version of findAllResults this: // ConfigObject.groovy private convertValuesToString(props) { def newProps = [:] for(e in props) { newProps[e.key] = e.value?.toString() } return newProps } becomes: private convertValuesToString(props) { props.findAllResults { it.value?.toString() } } A few other examples that come to mind that could use findResult or findAllResults are: a List of paths/file handles, grep each file in the list with a regular expression with wildcards, return the file and matched text in the first one hit You have a grails service that can take an url, download the web page and extract contact information. With a list of URLs, you could use urls.findAllResults { webService.extractContactInfo(url) } to get all the contact info aff all the web pages implementing grails-like UrlMappings, similar to the rewrite rules above where the first url mapping that matches returns the controller/action to forward to urlMappings.findResult { it.handle(url) }
          Hide
          blackdrag blackdrag added a comment -

          Don't get me wrong, I am not pro or contra here, but I need to understand the issue fully before giving my ok for this.

          Ok, now the picture becomes more clear....I agree mostly now, only the map example...

          private convertValuesToString(props) {
            return props.inject([:]) {e -> it[e.key] = e.value?.toString()}
          }
          

          Or maybe too complicated? Maybe the findAllResult method should be called different... like... map? Or we stay with inject....

          Anyway... bak to findResult... From the use case I see a lot of ?:, maybe this should be part of the method then? maybe

          String rewrite(String url) {
            rules.findResult(url) { it.handle(url) }
          }

          would be even better. findResult would then take an parameter with default value null.

          Show
          blackdrag blackdrag added a comment - Don't get me wrong, I am not pro or contra here, but I need to understand the issue fully before giving my ok for this. Ok, now the picture becomes more clear....I agree mostly now, only the map example... private convertValuesToString(props) { return props.inject([:]) {e -> it[e.key] = e.value?.toString()} } Or maybe too complicated? Maybe the findAllResult method should be called different... like... map? Or we stay with inject.... Anyway... bak to findResult... From the use case I see a lot of ?:, maybe this should be part of the method then? maybe String rewrite( String url) { rules.findResult(url) { it.handle(url) } } would be even better. findResult would then take an parameter with default value null.
          Hide
          Ted Naleid added a comment -

          Good points, I think you're probably right that inject is good enough and there isn't as much of a need for findAllResults.

          Inject can do everything that findAllResults can, without running f() extra times. I do think findAllResults is a little clearer in what's going on:

          def list = [1, 2, 3, 4]
          assert(["1", "2"] == list.inject([]) { newList, val -> if (val < 3) newList << "$val"; newList })
          // versus
          assert(["1", "2"] == list.findAllResults { if (it < 3) "$it" })
          

          But inject has more flexibility as it can return things other than lists (and it's already in the code base . It could be added in later if we find there is demand for it if findResult gets added and people think that findAllResults is a natural and expected extension.

          findResult is really the more important and, I think, valuable part of the patch. I think your suggestion to add an optional default parameter is a good one as I do think that many of the use cases for findResult would want a default value.

          I'll modify the patch tonight to remove findAllResults, add a optional default to find result, and change the map version of findResult to just return the non-null result of the closure (as per my first comment above) rather than a Map.Entry.

          Show
          Ted Naleid added a comment - Good points, I think you're probably right that inject is good enough and there isn't as much of a need for findAllResults. Inject can do everything that findAllResults can, without running f() extra times. I do think findAllResults is a little clearer in what's going on: def list = [1, 2, 3, 4] assert ([ "1" , "2" ] == list.inject([]) { newList, val -> if (val < 3) newList << "$val" ; newList }) // versus assert ([ "1" , "2" ] == list.findAllResults { if (it < 3) "$it" }) But inject has more flexibility as it can return things other than lists (and it's already in the code base . It could be added in later if we find there is demand for it if findResult gets added and people think that findAllResults is a natural and expected extension. findResult is really the more important and, I think, valuable part of the patch. I think your suggestion to add an optional default parameter is a good one as I do think that many of the use cases for findResult would want a default value. I'll modify the patch tonight to remove findAllResults, add a optional default to find result, and change the map version of findResult to just return the non-null result of the closure (as per my first comment above) rather than a Map.Entry.
          Hide
          Ted Naleid added a comment -

          updated patch that removes findAllResults, changes map version to return just the result instead of a Map.Entry and adds the ability to pass a default parameter

          Show
          Ted Naleid added a comment - updated patch that removes findAllResults, changes map version to return just the result instead of a Map.Entry and adds the ability to pass a default parameter
          Hide
          Ted Naleid added a comment -

          I've finished the modifications mentioned above. The latest patch ("findResult_with_defaultResult.patch") has these changes from the original:

          • removed findAllResults as inject can fulfill the role for the most part, this could be added later if desired.
          • modified the map version of findResult so that it returns the closure result rather than a Map.Entry with the value as the closure result, this is more intuitive and someone could always construct a Map.Entry to return if that's what they want, example:
          assert "I found c:3" == [a: 1, c:3].findResult { 
              if (it.value > 2) return "I found ${it.key}:${it.value}"
          }
          
          • added an optional defaultResult parameter
          assert "default" == [1, 2, 3].findResult("default") { 
              if (it == 8) return "never get here"
          }
          

          All tests have been updated to exercise the new code and are passing. I'd be interested to hear any feedback and if there are other changes that people feel would make this even more valuable.

          Show
          Ted Naleid added a comment - I've finished the modifications mentioned above. The latest patch ("findResult_with_defaultResult.patch") has these changes from the original: removed findAllResults as inject can fulfill the role for the most part, this could be added later if desired. modified the map version of findResult so that it returns the closure result rather than a Map.Entry with the value as the closure result, this is more intuitive and someone could always construct a Map.Entry to return if that's what they want, example: assert "I found c:3" == [a: 1, c:3].findResult { if (it.value > 2) return "I found ${it.key}:${it.value}" } added an optional defaultResult parameter assert " default " == [1, 2, 3].findResult( " default " ) { if (it == 8) return "never get here" } All tests have been updated to exercise the new code and are passing. I'd be interested to hear any feedback and if there are other changes that people feel would make this even more valuable.
          Hide
          Paul King added a comment -

          It would be nice to consider whether there is a more general solution to this. One that also fixed GROOVY-3526 would be good.

          Show
          Paul King added a comment - It would be nice to consider whether there is a more general solution to this. One that also fixed GROOVY-3526 would be good.
          Hide
          Paul King added a comment -

          BTW: Great to have a patch with tests and doco!

          Show
          Paul King added a comment - BTW: Great to have a patch with tests and doco!
          Hide
          Guillaume Laforge added a comment -

          Thanks for this implementation!

          Show
          Guillaume Laforge added a comment - Thanks for this implementation!
          Hide
          Andre added a comment - - edited

          actually can we change the name of the issue (remove findAllResults), because only findResult has been added, itīs misleading and appears in the release notes that way
          my 2ct: inject is more powerful and can do everything that findAllResults can do and more, but findAllResults is so much more intuitive - imho it should be added, doesnt do harm and you would always think: now there is find, findAll, findResult ... so where is findAllResults?

          Show
          Andre added a comment - - edited actually can we change the name of the issue (remove findAllResults), because only findResult has been added, itīs misleading and appears in the release notes that way my 2ct: inject is more powerful and can do everything that findAllResults can do and more, but findAllResults is so much more intuitive - imho it should be added, doesnt do harm and you would always think: now there is find, findAll, findResult ... so where is findAllResults?
          Hide
          Uri Moszkowicz added a comment -

          I know this issue is closed but I tend to agree that findAllResults would be useful. It does look like inject() could achieve the same but I wouldn't have thought to use it. This is one case where the implementation doesn't match the mental model.

          Show
          Uri Moszkowicz added a comment - I know this issue is closed but I tend to agree that findAllResults would be useful. It does look like inject() could achieve the same but I wouldn't have thought to use it. This is one case where the implementation doesn't match the mental model.
          Hide
          Uri Moszkowicz added a comment -

          Interestingly, I also found this question on StackOverlow:

          http://stackoverflow.com/questions/5789795/collection-findallresults-in-groovy-1-7-5

          I have another example where inject is even more verbose:

          def text = "file1: could not\nline2\nfile2: could not\nline3\n"
          println text.readLines().inject([]) { l, v -> l << v.find(~/(\S+): could not/)

          { it[1] } }.grep { it != null }

          contrasted with

          println text.readLines().findResults { it.find(~/(\s+): could not/) { it[1] }

          }

          Show
          Uri Moszkowicz added a comment - Interestingly, I also found this question on StackOverlow: http://stackoverflow.com/questions/5789795/collection-findallresults-in-groovy-1-7-5 I have another example where inject is even more verbose: def text = "file1: could not\nline2\nfile2: could not\nline3\n" println text.readLines().inject([]) { l, v -> l << v.find(~/(\S+): could not/) { it[1] } }.grep { it != null } contrasted with println text.readLines().findResults { it.find(~/(\s+): could not/) { it[1] } }
          Hide
          Paul King added a comment -

          In case there is further interest, I created GROOVY-4899

          Show
          Paul King added a comment - In case there is further interest, I created GROOVY-4899

            People

            • Assignee:
              Guillaume Laforge
              Reporter:
              Ted Naleid
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: