commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jacopo Cappellato <jacopo.cappell...@gmail.com>
Subject Re: [csv] Proposal to remove setter methods from CSVStrategy
Date Sun, 30 Jan 2011 08:04:14 GMT
Hi Adrian,

yes this is what I was thinking when I mentioned the factory method.

Jacopo

On Jan 30, 2011, at 8:06 AM, Adrian Crum wrote:

> 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
> 


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


Mime
View raw message