commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Adrian Crum <adrian.c...@sandglass-software.com>
Subject Re: [csv] Proposal to remove setter methods from CSVStrategy
Date Sun, 30 Jan 2011 07:06:39 GMT
 From my perspective, if DEFAULT_STRATEGY, EXCEL_STRATEGY, or TDF_STRATEGY are mutable, then
copies of them should be returned in a factory method. Ideally, a selector would be passed
to a factory method:

CSVStrategy strategy = CSVStrategy.getInstance(CSVStrategy.DEFAULT_STRATEGY);


-Adrian


On 1/29/2011 8:51 PM, Jacopo Cappellato wrote:
> Hi Gary,
>
> I initially proposed the change when I noticed the following code in one of the automated
tests of common-csv:
>
>      CSVStrategy strategy = (CSVStrategy)CSVStrategy.DEFAULT_STRATEGY.clone();
>      strategy.setCommentStart('#');
>      TestCSVParser parser = new TestCSVParser(new StringReader(code), strategy);
>
> and I realized the importance, in a calling method that alters the default settings,
of using "clone".
> But apart from this I don't have real world use cases that describe issues around this.
>
> Jacopo
>
>
> On Jan 29, 2011, at 9:49 PM, Gary Gregory wrote:
>
>> Are people really using these classes in an MT way?  These seem like the kind of
classes you do not share between threads. My op only though.
>>
>> Gary
>>
>> On Jan 29, 2011, at 12:27, "Simone Tripodi"<simonetripodi@apache.org>  wrote:
>>
>>> Hi all guys!!! :)
>>> I'd suggest to apply Jacopo's suggestion, making strategies' fields as
>>> final would help classes becoming thread-safe; to fix the issue of
>>> construction described by Stephen, it would be useful adding builder
>>> classes (optionally implemented as static inner classes).
>>> WDYT? HTH, that's just my opinion,
>>> Simo
>>>
>>> http://people.apache.org/~simonetripodi/
>>> http://www.99soft.org/
>>>
>>>
>>>
>>> On Sat, Jan 29, 2011 at 2:33 PM, Jacopo Cappellato
>>> <jacopo.cappellato@gmail.com>  wrote:
>>>> Thank you Stephen,
>>>>
>>>> I understand this; alternatively we could implement a factory method that
clones DEFAULT_STRATEGY, EXCEL_STRATEGY, TDF_STRATEGY; but I will definitely postpone any
decision, it is not a big deal for me and we can wait for additional feedback.
>>>>
>>>> Jacopo
>>>>
>>>> On Jan 29, 2011, at 1:16 PM, Stephen Colebourne wrote:
>>>>
>>>>> Traditionally, many commons projects has taken no position on the
>>>>> correct way to instantiate objects, whether via constructor or bean
>>>>> methods. This was to allow construction from tools such as Velocity.
>>>>> It is also still easier to construct using Spring via bean methods
>>>>> rather than the constructor.
>>>>>
>>>>> These days, immutability and concurrency are more important, so this
>>>>> change may be applicable, buts its good to understand the history and
>>>>> wide use cases of classes like these.
>>>>>
>>>>> Stephen
>>>>>
>>>>>
>>>>> On 29 January 2011 12:04, Jacopo Cappellato<jacopo.cappellato@gmail.com>
 wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> in my opinion all the setter methods should be removed from the CSVStrategy
class: in this way the fields will only be set using the constructors and they will become
readonly.
>>>>>> The main issue I see with the current implementation is that a calling
method can modify the values of the fields of the following static objects declared in CSVStrategy
(changing the default behavior for all subsequent code that uses for example CSVStrategy.DEFAULT_STRATEGY):
>>>>>>
>>>>>>    public static CSVStrategy DEFAULT_STRATEGY = new CSVStrategy(',',
'"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
>>>>>>                                                                 true,
false, true);
>>>>>>    public static CSVStrategy EXCEL_STRATEGY   = new CSVStrategy(',',
'"', COMMENTS_DISABLED, ESCAPE_DISABLED, false,
>>>>>>                                                                 false,
false, false);
>>>>>>    public static CSVStrategy TDF_STRATEGY     = new CSVStrategy('\t',
'"', COMMENTS_DISABLED, ESCAPE_DISABLED, true,
>>>>>>                                                                 true,
false, true);
>>>>>>
>>>>>> What do you think?
>>>>>>
>>>>>> Jacopo
>>>>>>
>>>>>>
>>>>>> ---------------------------------------------------------------------
>>>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>>>
>>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>
>>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message