Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.2
    • Component/s: None
    • Labels:
      None
    • Environment:

      Operating System: other
      Platform: Other

      Description

      This is the initial implementation of the new VariableFormat class as discussed
      on the dev list.

      VariableFormat provides several methods dealing with variable interpolation.

        Activity

        Hide
        Henri Yandell added a comment -

        Apologies for spam - changed my mind about the previous change.

        Show
        Henri Yandell added a comment - Apologies for spam - changed my mind about the previous change.
        Hide
        Henri Yandell added a comment -

        This thread is complete - was only left open due to a desire to have 100% in
        Clover and I think we can take care of that in another issue if need be.

        Show
        Henri Yandell added a comment - This thread is complete - was only left open due to a desire to have 100% in Clover and I think we can take care of that in another issue if need be.
        Hide
        ggregory@seagullsw.com added a comment -

        Maybe Clover thinks there are 2 code paths?
        (1) Go in the while loop.
        (2) Don't go in the while loop at all.

        Show
        ggregory@seagullsw.com added a comment - Maybe Clover thinks there are 2 code paths? (1) Go in the while loop. (2) Don't go in the while loop at all.
        Hide
        Oliver Heger added a comment -

        (In reply to comment #10)
        > In the case of:
        >
        > while ((tok = parser.nextToken(data)) != null) {
        >
        > I wonder if the issue is that the tok is never null, so the while expression
        > never evaluates to false, which is why we get a "?" in Clover.

        But that tok becomes null is the end condition of the while loop. There are no
        further exit points. I don't understand why clover has problems with statements
        of this type.

        Show
        Oliver Heger added a comment - (In reply to comment #10) > In the case of: > > while ((tok = parser.nextToken(data)) != null) { > > I wonder if the issue is that the tok is never null, so the while expression > never evaluates to false, which is why we get a "?" in Clover. But that tok becomes null is the end condition of the while loop. There are no further exit points. I don't understand why clover has problems with statements of this type.
        Hide
        ggregory@seagullsw.com added a comment -

        I've brought in this rework in SVN with a notable change: The class
        VariableFormatter.Token has been removed in favor of re-using
        java.text.FieldPosition.

        Show
        ggregory@seagullsw.com added a comment - I've brought in this rework in SVN with a notable change: The class VariableFormatter.Token has been removed in favor of re-using java.text.FieldPosition.
        Hide
        ggregory@seagullsw.com added a comment -

        In the case of:

        while ((tok = parser.nextToken(data)) != null) {

        I wonder if the issue is that the tok is never null, so the while expression
        never evaluates to false, which is why we get a "?" in Clover.

        Show
        ggregory@seagullsw.com added a comment - In the case of: while ((tok = parser.nextToken(data)) != null) { I wonder if the issue is that the tok is never null, so the while expression never evaluates to false, which is why we get a "?" in Clover.
        Hide
        ggregory@seagullsw.com added a comment -

        FYI and in case someone else is: I am looking at, integrating and testing this
        current batch set of patches.

        Show
        ggregory@seagullsw.com added a comment - FYI and in case someone else is: I am looking at, integrating and testing this current batch set of patches.
        Hide
        Oliver Heger added a comment -

        Created an attachment (id=15934)
        Updated test class for VariableFormatter

        Minor modifications at the current test class and new test cases for the new
        char[] methods.

        One word about the test coverage reported by clover: This tool seems to have
        problems with combined assignments and comparisons as in

        while((tok = nextToken()) != null)

        This lowers the reported coverage a bit, but I think statements like the one
        above are a common and elegant practise, aren't they?

        Show
        Oliver Heger added a comment - Created an attachment (id=15934) Updated test class for VariableFormatter Minor modifications at the current test class and new test cases for the new char[] methods. One word about the test coverage reported by clover: This tool seems to have problems with combined assignments and comparisons as in while((tok = nextToken()) != null) This lowers the reported coverage a bit, but I think statements like the one above are a common and elegant practise, aren't they?
        Hide
        Oliver Heger added a comment -

        Created an attachment (id=15933)
        New implementation of VariableFormatter

        This patch contains some modifications of VariableFormatter with the purpose to
        make the code easier to understand and more integrated into the rest of the
        text package.

        The main idea of this patch is to separate the logic for detecting variables
        from the processing of variables. There is now a new nested class
        VariableParser, which scans the input data for text and variable tokens. This
        class is used in the main interpolation method and called in a loop to obtain
        all tokens and process them according to their type.

        This approach IMO makes customization of this class easier. ATM the involved
        helper classes and methods are protected allowing a sub class to override some
        of them. This way e.g. completely different mechanisms for detecting variables
        or escaping could be implemented. If you think that the API becomes too
        complex, the involved classes and methods could be made private.

        There are the following further modifications and improvements:

        • The main interpolation method now operates on a char[] and can therefor
          easier be used from other classes in this package.
        • StrTokenizer.Matcher is used for detecting variable tokens. This is a
          "natural" way of dealing with char[].
        • The escaping mechanism can now be implemented much easier. No need for
          escape() and unescape() methods or for escaping variable end tokens.
        • StrBuilder is used internally for constructing result strings instead of
          StringBuffer.

        WDYT?

        Show
        Oliver Heger added a comment - Created an attachment (id=15933) New implementation of VariableFormatter This patch contains some modifications of VariableFormatter with the purpose to make the code easier to understand and more integrated into the rest of the text package. The main idea of this patch is to separate the logic for detecting variables from the processing of variables. There is now a new nested class VariableParser, which scans the input data for text and variable tokens. This class is used in the main interpolation method and called in a loop to obtain all tokens and process them according to their type. This approach IMO makes customization of this class easier. ATM the involved helper classes and methods are protected allowing a sub class to override some of them. This way e.g. completely different mechanisms for detecting variables or escaping could be implemented. If you think that the API becomes too complex, the involved classes and methods could be made private. There are the following further modifications and improvements: The main interpolation method now operates on a char[] and can therefor easier be used from other classes in this package. StrTokenizer.Matcher is used for detecting variable tokens. This is a "natural" way of dealing with char[]. The escaping mechanism can now be implemented much easier. No need for escape() and unescape() methods or for escaping variable end tokens. StrBuilder is used internally for constructing result strings instead of StringBuffer. WDYT?
        Hide
        ggregory@seagullsw.com added a comment -

        I've Retro-fitted the VariableResolver interface into the VariableFormatter
        class and provided a Map-backed VariableResolver implementation. I did not
        change everything to statics.

        Show
        ggregory@seagullsw.com added a comment - I've Retro-fitted the VariableResolver interface into the VariableFormatter class and provided a Map-backed VariableResolver implementation. I did not change everything to statics.
        Hide
        ggregory@seagullsw.com added a comment -

        Thanks Oliver, I'll take a look over the next couple days. I must say that I am
        not sure I like the "all static" method approach since it prevents subclassing
        as a means of customization. Perhaps I'll try retro-fitting the VariableResolver
        interface only and see what that looks and feels like. G.

        Show
        ggregory@seagullsw.com added a comment - Thanks Oliver, I'll take a look over the next couple days. I must say that I am not sure I like the "all static" method approach since it prevents subclassing as a means of customization. Perhaps I'll try retro-fitting the VariableResolver interface only and see what that looks and feels like. G.
        Hide
        Oliver Heger added a comment -

        Created an attachment (id=15663)
        Patch for VariableResolverTest

        Update for the test class.

        Show
        Oliver Heger added a comment - Created an attachment (id=15663) Patch for VariableResolverTest Update for the test class.
        Hide
        Oliver Heger added a comment -

        Created an attachment (id=15662)
        Patch for VariableResolver

        This is a patch for VariableResolver (or almost a complete new implementation)
        that re-introduces the VariableResolver interface and makes all methods static.

        This is a first shot only to get a feeling how this approach looks like.
        Further convenience methods could be added (e.g. for System properties
        interpolation). That the map based VariableResolver is provided as a static
        method is just an idea; other implementations are possible.

        Show
        Oliver Heger added a comment - Created an attachment (id=15662) Patch for VariableResolver This is a patch for VariableResolver (or almost a complete new implementation) that re-introduces the VariableResolver interface and makes all methods static. This is a first shot only to get a feeling how this approach looks like. Further convenience methods could be added (e.g. for System properties interpolation). That the map based VariableResolver is provided as a static method is just an idea; other implementations are possible.
        Hide
        Oliver Heger added a comment -

        Created an attachment (id=15580)
        Test class for VariableFormat

        Show
        Oliver Heger added a comment - Created an attachment (id=15580) Test class for VariableFormat
        Hide
        Oliver Heger added a comment -

        Created an attachment (id=15579)
        Source code of VariableFormat

        Show
        Oliver Heger added a comment - Created an attachment (id=15579) Source code of VariableFormat

          People

          • Assignee:
            Unassigned
            Reporter:
            Oliver Heger
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development