commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: [csv] Why does CSVFormat provide a validate() method instead of validating parameters passed to its constructor?
Date Wed, 14 Mar 2012 21:41:26 GMT
On 14 March 2012 21:25, Benedikt Ritter <beneritter@googlemail.com> wrote:
> Am 14. März 2012 22:16 schrieb sebb <sebbaz@gmail.com>:
>> On 14 March 2012 21:02, Emmanuel Bourg <ebourg@apache.org> wrote:
>>> Le 14/03/2012 21:52, Benedikt Ritter a écrit :
>>>
>>>
>>>> the subject of this mail is pretty self-explanatory. Why do we need a
>>>> package private validate() method, given the fact, that users can not
>>>> create custom instances (constructor is package private)? You could
>>>> even argue, that no validation is needed at all, since we are in
>>>> control of what formats can be created. However I'd say, the
>>>> validation makes sure that we do not unintentionally create invalid
>>>> formats. But then validate() can be made private and called at the end
>>>> of the constructor.
>>>
>>>
>>> Because the format could temporarily be in an invalid state. Something like
>>> this would break (a bit far fetched, I agree):
>>>
>>> CSVFormat.DEFAULT.withDelimiter('#').withCommentStart('/');
>>
>> In which case, reversing the order would work.
>> We would just have to document this as a restriction.
>>
>> The problem with leaving the validation until later is that it
>> decouples the cause and effect.
>> This makes it a bit harder to debug.
>>
>> It's also simpler if there is a single validation method.
>> At present some of the setters also perform validation.
>>
>
> I agree with you on this. However, I think it would be better to tie
> validation to the object creation. Maybe the Builder Pattern like
> shown in Effective Java p. 14-15 is a reasonable solution for this
> case? It would be a bit more verbose, but we can be sure that
> everything will be validated.
>
>> BTW, we should probably reject delimiter == DISABLED.
>>
>> Also, the DISABLED constant needs to be public (or available via a
>> public getter) otherwise it's not possible to disable all but the
>> delimiter.
>> Using a getter would allow the constant to be changed if it became necessary.
>>
>
> Only if we remove Serializable...

I'd forgotten about that lurking demon.

It would still be possible to change the constant, but it would cause
a compatibility break for serialised instances.
Far less likely to cause problems than a break in binary compat.

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