commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gary Gregory <garydgreg...@gmail.com>
Subject Re: [csv] CSVRecord and Map<String, String>
Date Wed, 22 Jan 2014 16:07:09 GMT
On Wed, Jan 22, 2014 at 11:03 AM, <adrian.crum@sandglass-software.com>wrote:

> There is no question your use case is convenient, but it violates the
> contract that a CSV record is read-only. Calling put() on a CSV record is a
> mutating operation.
>
> So, is a CSV record read-only or not?


That the heart of the matter indeed.

For the sake of getting 1.0 out the door, we can leave it read-only.

If we want to make it read-write, we can do that later, in 1.1 or 2.0.
Whether or not this is done while preserving BC will be a different matter.

Gary


>
>
> -Adrian
>
> Quoting Gary Gregory <garydgregory@gmail.com>:
>
>  On Wed, Jan 22, 2014 at 10:04 AM, <adrian.crum@sandglass-software.com
>> >wrote:
>>
>>  I disagree. If the CSV record is read-only, then the List and Map
>>> representations of the CSV record should be read-only too. Making those
>>> representations writable is confusing - it gives the impression you are
>>> modifying the CSV record when in fact you are not.
>>>
>>> I don't see how it restricts their usability.
>>>
>>>
>> I guess I should have started with another use-case: take a CSV, remove a
>> column, and compute a new column.
>>
>> Right now I can do (to try, add this to CSVRecordTest):
>>
>>     @Test
>>     public void testRemoveAndAddColumns() throws IOException {
>>         // do:
>>         final CSVPrinter printer = new CSVPrinter(new StringBuilder(),
>> CSVFormat.DEFAULT);
>>         final Map<String, String> map = recordWithHeader.toMap();
>>         map.remove("OldColumn");
>>         map.put("NewColumn", "NewValue");
>>         // check:
>>         final ArrayList<String> list = new ArrayList<String>(map.values()
>> );
>>         Collections.sort(list);
>>         printer.printRecord(list);
>>         Assert.assertEquals("A,B,C,NewValue" +
>> CSVFormat.DEFAULT.getRecordSeparator(), printer.getOut().toString());
>>     }
>>
>> I would have to create a wasteful temp Map if the Map I get from the
>> record
>> is read-only. This code is bad enough.
>>
>> If the record is a Map, there is no song and dance at all:
>>
>>         // do:
>>         final CSVPrinter printer = new CSVPrinter(new StringBuilder(),
>> CSVFormat.DEFAULT);
>>         recordWithHeader.remove("OldColumn");
>>         recordWithHeader.put("NewColumn", "NewValue");
>>         // check:
>>         printer.printRecord(recordWithHeader);
>>         Assert.assertEquals("A,B,C,NewValue" +
>> CSVFormat.DEFAULT.getRecordSeparator(), printer.getOut().toString());
>>
>>
>> If you want a special kind of Map (a blinking read-only map ;), you can
>> call putIn(Map).
>>
>> Gary
>>
>>
>>> -Adrian
>>>
>>> Quoting Gary Gregory <garydgregory@gmail.com>:
>>>
>>>  On Wed, Jan 22, 2014 at 9:40 AM, <adrian.crum@sandglass-software.com>
>>>
>>>> wrote:
>>>>
>>>>  CORRECTION: The patch in the Jira issue is a simplified version of
>>>> Gary's
>>>>
>>>>> implementation.
>>>>>
>>>>> I started with the design I proposed - an inner class Map
>>>>> implementation
>>>>> backed by CSVRecord, but then implementing entrySet() and such became
>>>>> complicated. So I changed it to the inner class being backed by a
>>>>> HashMap,
>>>>> then reduced that to a simplified version of Gray's implementation.
>>>>>
>>>>> I apologize for the confusion.
>>>>>
>>>>>
>>>>>  I do not see the point of making the new objects (list, map)
>>>> read-only.
>>>> Since the objects are not connected to the record, there should be no
>>>> expectancy of synchronizing the map with the record. There are "to"
>>>> methods
>>>> after all. This just restricts the usability of the objects.
>>>>
>>>> Gary
>>>>
>>>>
>>>>  -Adrian
>>>>>
>>>>>
>>>>> Quoting Adrian Crum <adrian.crum@sandglass-software.com>:
>>>>>
>>>>>  I agree with Emmanuel. We don't want CSVRecord to devolve into a god
>>>>>
>>>>>  class that tries to be everything to everybody. "Do only one thing,
>>>>>> and
>>>>>> do
>>>>>> it really well" is the design pattern I prefer.
>>>>>>
>>>>>> I created a patch for the design I proposed a few days ago. Please
>>>>>> consider using it.
>>>>>>
>>>>>> https://issues.apache.org/jira/browse/CSV-104
>>>>>>
>>>>>>
>>>>>> Adrian Crum
>>>>>> Sandglass Software
>>>>>> www.sandglass-software.com
>>>>>>
>>>>>> On 1/22/2014 8:01 AM, Emmanuel Bourg wrote:
>>>>>>
>>>>>>  Le 21/01/2014 15:08, Gary Gregory a écrit :
>>>>>>
>>>>>>>
>>>>>>>  This kind of complication is unnecessary IMO, why not just have
the
>>>>>>>
>>>>>>>  record
>>>>>>>> implement Map?
>>>>>>>>
>>>>>>>>
>>>>>>>>  Because a record is neither a List nor a Map.
>>>>>>>
>>>>>>> A map decorator isn't that complicated, and the work has already
been
>>>>>>> done in [configuration]. It would be trivial to adapt the code
to
>>>>>>> [csv].
>>>>>>>
>>>>>>> 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
>>>>>
>>>>>
>>>>>
>>>>>
>>>> --
>>>> E-Mail: garydgregory@gmail.com | ggregory@apache.org
>>>> Java Persistence with Hibernate, Second Edition<http://www.manning.
>>>> com/bauer3/>
>>>> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
>>>> Spring Batch in Action <http://www.manning.com/templier/>
>>>>
>>>> Blog: http://garygregory.wordpress.com
>>>> Home: http://garygregory.com/
>>>> Tweet! http://twitter.com/GaryGregory
>>>>
>>>>
>>>>
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>
>>>
>>>
>>
>> --
>> E-Mail: garydgregory@gmail.com | ggregory@apache.org
>> Java Persistence with Hibernate, Second Edition<http://www.manning.
>> com/bauer3/>
>> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
>> Spring Batch in Action <http://www.manning.com/templier/>
>> Blog: http://garygregory.wordpress.com
>> Home: http://garygregory.com/
>> Tweet! http://twitter.com/GaryGregory
>>
>>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>


-- 
E-Mail: garydgregory@gmail.com | ggregory@apache.org
Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message