Solr
  1. Solr
  2. SOLR-2920

Refactor frequent conditional use of DefaultSolrParams into SolrParams.combine(p,d)

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.6, 4.0-ALPHA
    • Component/s: None
    • Labels:
      None

      Description

      I had a small bug in use of DefaultSolrParams in my code because I didn't check for non-existent defaults. I noticed through the Solr codebase that is code pattern is very common:

          if( defaults != null ) {
            params = new DefaultSolrParams( params, defaults );
          }
      

      Instead, I refactored this logic into a new SolrParams.combine(p,d) method and made it so that nobody refers to DefaultSolrParams. I did similarly for AppendedSolrParams.

        Activity

        Hide
        David Smiley added a comment -

        The attached patch was done against trunk r1205366

        Show
        David Smiley added a comment - The attached patch was done against trunk r1205366
        Hide
        Hoss Man added a comment -

        David:

        +1 to the refactoring ... thta's a great idea to help eliminate bugs

        -0 to the names SolrParams.combine and SolrParams.combineAppended .. "combine" doesn't really make it clear what it's doing, it sounds more like a merge then wrapping in a defaults.

        What do you think about putting each of these static methods in their respective classes?... "params = DefaultSolrParams.wrap(params,defaults)" and "params = AppendedSolrParamswrap(params,extra)" ?

        (BTW: can you config your IDE to not reorder import statements? makes patches a lot harder to review)

        Show
        Hoss Man added a comment - David: +1 to the refactoring ... thta's a great idea to help eliminate bugs -0 to the names SolrParams.combine and SolrParams.combineAppended .. "combine" doesn't really make it clear what it's doing, it sounds more like a merge then wrapping in a defaults. What do you think about putting each of these static methods in their respective classes?... "params = DefaultSolrParams.wrap(params,defaults)" and "params = AppendedSolrParamswrap(params,extra)" ? (BTW: can you config your IDE to not reorder import statements? makes patches a lot harder to review)
        Hide
        David Smiley added a comment -

        Thanks for noticing my issue, Hoss.

        "combine" vs "merge" seem similar to me. "wrap" (if on DefaultSolrParams) or "wrapDefaults" (if not) sounds clearer to me.

        I started this by putting a method in the respective class it came from, but then I noticed that SolrParams already has some handy static utility methods. Then it occurred to me that it would be convenient if the caller didn't need to know about all the classes in this package – just SolrParams with some static factory methods. It's just one class to import too. Extending this line of reasoning suggests more refactoring would be in order to create a MapSolrParams. What do you think?

        RE import statements: It would be nice if there was a standard the project could agree on so we could just configure our IDEs consistently. In the mean time, I'll try to reduce import statement modifications.

        Show
        David Smiley added a comment - Thanks for noticing my issue, Hoss. "combine" vs "merge" seem similar to me. "wrap" (if on DefaultSolrParams) or "wrapDefaults" (if not) sounds clearer to me. I started this by putting a method in the respective class it came from, but then I noticed that SolrParams already has some handy static utility methods. Then it occurred to me that it would be convenient if the caller didn't need to know about all the classes in this package – just SolrParams with some static factory methods. It's just one class to import too. Extending this line of reasoning suggests more refactoring would be in order to create a MapSolrParams. What do you think? RE import statements: It would be nice if there was a standard the project could agree on so we could just configure our IDEs consistently. In the mean time, I'll try to reduce import statement modifications.
        Hide
        David Smiley added a comment -

        Anybody have any opinion here? I'd love to get this issue to a resolution.

        Show
        David Smiley added a comment - Anybody have any opinion here? I'd love to get this issue to a resolution.
        Hide
        Hoss Man added a comment -

        I started this by putting a method in the respective class it came from, but then I noticed that SolrParams already has some handy static utility methods. Then it occurred to me that it would be convenient if the caller didn't need to know about all the classes in this package

        Ah ... good point. SolrParams already has static methods like this, and no reason to increase the import count.

        SolrParams.wrapAppended and SolrParams.wrapDefaults are a good call.

        Show
        Hoss Man added a comment - I started this by putting a method in the respective class it came from, but then I noticed that SolrParams already has some handy static utility methods. Then it occurred to me that it would be convenient if the caller didn't need to know about all the classes in this package Ah ... good point. SolrParams already has static methods like this, and no reason to increase the import count. SolrParams.wrapAppended and SolrParams.wrapDefaults are a good call.
        Hide
        David Smiley added a comment -

        I attached a new patch using wrapDefaults and wrapAppended, and I took care to retain the import statements according to Lucene/Solr's strict standards

        I think it's still an open-question if we want to more aggressively take the SolrParams factory-method approach and create some methods that return a MapSolrParams, ModifiableSolrParams, MultiMapSolrParams, and RequiredSolrParams. Personally, I really like the idea of SolrParams being a one-stop shop for these.

        Show
        David Smiley added a comment - I attached a new patch using wrapDefaults and wrapAppended, and I took care to retain the import statements according to Lucene/Solr's strict standards I think it's still an open-question if we want to more aggressively take the SolrParams factory-method approach and create some methods that return a MapSolrParams, ModifiableSolrParams, MultiMapSolrParams, and RequiredSolrParams. Personally, I really like the idea of SolrParams being a one-stop shop for these.
        Hide
        Hoss Man added a comment -

        David: thanks for the updated, reading the diff something wonky jumped out at me though...

        in SingleFragListBuilder & SimpleFragListBuilder the "params" are no longer wrapped in defaults at all - is that intentional? was that dead code before? .. or is that just a accidental deletion?

        Show
        Hoss Man added a comment - David: thanks for the updated, reading the diff something wonky jumped out at me though... in SingleFragListBuilder & SimpleFragListBuilder the "params" are no longer wrapped in defaults at all - is that intentional? was that dead code before? .. or is that just a accidental deletion?
        Hide
        David Smiley added a comment -

        Those params weren't used at all – dead code.

        Show
        David Smiley added a comment - Those params weren't used at all – dead code.
        Hide
        Hoss Man added a comment -

        yep yep ... totally get it now. they don't use any of the params they are given, so no reason to waste the cycles wrapping the defaults.

        just in case though, i added a comment about the defaults in case those methods ever do start using defaults.

        Committed revision 1213474. - trunk

        ...testing the merge back to 3x now.

        Show
        Hoss Man added a comment - yep yep ... totally get it now. they don't use any of the params they are given, so no reason to waste the cycles wrapping the defaults. just in case though, i added a comment about the defaults in case those methods ever do start using defaults. Committed revision 1213474. - trunk ...testing the merge back to 3x now.
        Hide
        Hoss Man added a comment -

        Committed revision 1213484. - 3x

        David: thanks for the suggestion & the following through with patch!

        Show
        Hoss Man added a comment - Committed revision 1213484. - 3x David: thanks for the suggestion & the following through with patch!

          People

          • Assignee:
            David Smiley
            Reporter:
            David Smiley
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development