commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bob Smith (JIRA)" <>
Subject [jira] Updated: (SANDBOX-279) CSVStrategy has modifiable public static variables
Date Tue, 10 Feb 2009 03:12:59 GMT


Bob Smith updated SANDBOX-279:

    Attachment: CSVStrategy_Immutable.patch

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.

> CSVStrategy has modifiable public static variables
> --------------------------------------------------
>                 Key: SANDBOX-279
>                 URL:
>             Project: Commons Sandbox
>          Issue Type: Improvement
>          Components: CSV
>    Affects Versions: Nightly Builds
>            Reporter: Bob Smith
>            Priority: Minor
>         Attachments: CSVStrategy_Immutable.patch, CSVStrategyAndTests.patch
> 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).

This message is automatically generated by JIRA.
You can reply to this email to add a comment to the issue online.

View raw message