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"<[email protected]>
>>> 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
>>>> <[email protected]> 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<[email protected]>
>>>>>> 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: [email protected]
>>>>>>> For additional commands, e-mail: [email protected]
>>>>>>>
>>>>>>>
>>>>>> ---------------------------------------------------------------------
>>>>>> To unsubscribe, e-mail: [email protected]
>>>>>> For additional commands, e-mail: [email protected]
>>>>>>
>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: [email protected]
>>>>> For additional commands, e-mail: [email protected]
>>>>>
>>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: [email protected]
>>>> For additional commands, e-mail: [email protected]
>>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: [email protected]
>>> For additional commands, e-mail: [email protected]
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [email protected]
>> For additional commands, e-mail: [email protected]
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [email protected]
> For additional commands, e-mail: [email protected]
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
