Commons CSV
  1. Commons CSV
  2. CSV-49

CSVStrategy has modifiable public static variables

    Details

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

      Description

      The public static variables in CSVStrategy are not final and DEFAULT_STRATEGY, EXCEL_STRATEGY, and TDF_STRATEGY can be modified using setter methods. I would recommend making them all final and using an unmodifiable subclass for the CSVStrategy variables.

      For this change to work, the main CSVStrategy constructor would have to be changed to set the class fields directly instead of using the setters since the unmodifiable subclass will have overwritten all of the setters to thrown UnsupportedOperationExceptions. And I think a copy constructor would also be a good addition to allow easily copying one of these (or any) CSVStrategy objects if any modifications have to be made to them (as an alternative to the clone method).

      1. CSVStrategy_Immutable.patch
        19 kB
        Bob Smith
      2. CSVStrategyAndTests.patch
        11 kB
        Bob Smith

        Activity

        Hide
        Emmanuel Bourg added a comment -

        The static fields are now final and the class is immutable. I followed the suggestion of Stephen Colebourne to use chained withXXX setters cloning the instance on the fly.

        Show
        Emmanuel Bourg added a comment - The static fields are now final and the class is immutable. I followed the suggestion of Stephen Colebourne to use chained withXXX setters cloning the instance on the fly.
        Hide
        Emmanuel Bourg added a comment -

        +1 for refactoring the class and make it immutable with a builder pattern.

        Show
        Emmanuel Bourg added a comment - +1 for refactoring the class and make it immutable with a builder pattern.
        Hide
        Bob Smith added a comment -

        If its still ok to make a big API change, then I think that would be a great idea. And if CSVStrategy is immutable then there would be no need to ever copy a CSVStrategy (so the clone method could go away and a copy constructor would not have to be added).

        If that change is made, then it might be nice to use the Builder pattern and get rid of all the normal constructors. That would keep it from loosing the flexibility that the mutable version had and it also makes it possible to add new values to CSVStrategy in the future without having to add more constructors. And I think it would make it easier to tell what parameter each each value goes with (especially with so many boolean variables).

        I attached a patch that includes these changes. Surprisingly, there weren't that many changes needed to the rest of the classes. A deprecated constructor in CSVParser needed to be changed to use a CSVStrategy.Builder and a few of the CSVParser test cases needed similar changes. I also got rid of the CSVParser test cases for the getters/setters of the CSVStrategy methods, but I suppose they could be modified to test the Builder inetead.

        Show
        Bob Smith added a comment - If its still ok to make a big API change, then I think that would be a great idea. And if CSVStrategy is immutable then there would be no need to ever copy a CSVStrategy (so the clone method could go away and a copy constructor would not have to be added). If that change is made, then it might be nice to use the Builder pattern and get rid of all the normal constructors. That would keep it from loosing the flexibility that the mutable version had and it also makes it possible to add new values to CSVStrategy in the future without having to add more constructors. And I think it would make it easier to tell what parameter each each value goes with (especially with so many boolean variables). I attached a patch that includes these changes. Surprisingly, there weren't that many changes needed to the rest of the classes. A deprecated constructor in CSVParser needed to be changed to use a CSVStrategy.Builder and a few of the CSVParser test cases needed similar changes. I also got rid of the CSVParser test cases for the getters/setters of the CSVStrategy methods, but I suppose they could be modified to test the Builder inetead.
        Hide
        Sebb added a comment -

        +1

        I'm not sure there is a use-case for changing the CSV Strategy once it has been created.

        I would make all the fields final, and only allow them to be set via the constructor. This would make the class immutable, and therefore thread-safe.

        The deprecated constructor could also be dispensed with.

        Show
        Sebb added a comment - +1 I'm not sure there is a use-case for changing the CSV Strategy once it has been created. I would make all the fields final, and only allow them to be set via the constructor. This would make the class immutable, and therefore thread-safe. The deprecated constructor could also be dispensed with.
        Hide
        Bob Smith added a comment - - edited

        I attached a patch for the changes described in the issue report. It includes the changes to CSVStrategy, some comments on the variables that were changed to indicate that they are not modifiable, some test cases to make sure these fields are not modifiable, and changes the CSVParserTest to stop attempting to modify the field.

        Show
        Bob Smith added a comment - - edited I attached a patch for the changes described in the issue report. It includes the changes to CSVStrategy, some comments on the variables that were changed to indicate that they are not modifiable, some test cases to make sure these fields are not modifiable, and changes the CSVParserTest to stop attempting to modify the field.

          People

          • Assignee:
            Unassigned
            Reporter:
            Bob Smith
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development