commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benedikt Ritter <brit...@apache.org>
Subject Re: [CSV] Naming pattern of getters and setters in CSVFormat (was: Re: [VOTE] Release Commons CSV 1.0 based on RC1)
Date Mon, 21 Jul 2014 19:26:33 GMT
2014-07-21 21:15 GMT+02:00 Gary Gregory <garydgregory@gmail.com>:

> On Mon, Jul 21, 2014 at 3:05 PM, Benedikt Ritter <britter@apache.org>
> wrote:
>
> > 2014-07-21 20:55 GMT+02:00 Gary Gregory <garydgregory@gmail.com>:
> >
> > > On Mon, Jul 21, 2014 at 2:45 PM, Benedikt Ritter <britter@apache.org>
> > > wrote:
> > >
> > > > Hey Gary,
> > > >
> > > > I like what you did with the isEscaping, isHandlingNull etc. The API
> > > looks
> > > > much cleaner to me now.
> > > >
> > > > What I don't understand is, why you added the type postfix to some of
> > the
> > > > getters. If we have getCommentStartCharacter, why don't we have
> > > > getRecordSeparatorString ?
> > > >
> > >
> > > No good reason.
> > >
> >
> > :)
> >
> > How about removing the postfixes again?
> >
> > If the requirement for additional getters returning different typs come
> up,
> > we can still add getters with postfix:
> >
> > CSV v1.0:
> >   getCommentStart()
> >
> > CSV v1.1
> >   getCommentStart()
> >   getCommentStartString() / getCommentStartAsString()
> >
> > WDYT?
> >
> >
> I'm not crazy about it because the APIs read oddly otherwise:
>
> For example getEscapeCharacter() is clear to me and means -- to me --
> returns the escape character. The only complaint I can imagine is
> verbosity. But taking it out is odd "get escape" means get escape what? Is
> escape enabled, disabled? Some attribute of escaping? So getEscapeCharacter
> is good as is IMO.
>
>
So you had a good reason ;-)

one could say, that the information what kind (of escape) is being returned
is encoded in the method signature, because it is returning a Character.
But I'm fine with leaving the type information in the method name, where it
is not clear. That said getDelimiter() is pretty clear to me, as well as
getRecordSeparator.


> getQuoteCharacter is good because we have more than one item to configure
> for quote: getQuoteMode and getQuoteCharacter. I can see that because of
> this specific property, getQuote is understandable, but I prefer
> consistency of getQuote<Attribute>.
>

Agreed.


>
> getCommentStartCharacter() is good because it is clean getCommentStart
> means what? Start could be a misinterpreted as verb, as in "comment
> starting is enabled" or "commenting is enabled" which is not what this
> property is about.
>

Emmanuel already commented on that particular method. We renamed it to
getCommentMarker(), but it looks like that has been changed again.

Let's change that back to getCommentMarker() again and leave the rest as it
is.


>
> Gary
>
>
>
>
> > Benedikt
> >
> >
> > >
> > > Gary
> > >
> > >
> > > >
> > > > We're very close...
> > > >
> > > > Benedikt
> > > >
> > > >
> > > > 2014-07-21 19:18 GMT+02:00 Gary Gregory <garydgregory@gmail.com>:
> > > >
> > > > > On Mon, Jul 21, 2014 at 12:35 PM, Benedikt Ritter <
> > britter@apache.org>
> > > > > wrote:
> > > > >
> > > > > > Please take another look at CSV in rev. 1612344. I have:
> > > > > >
> > > > > > - renamed the ignoreEmptyHeaders property to
> > allowMissingColumnNames
> > > > > > - prefixed everything that is a pure getter with "get"
> > > > > > - renamed Quote/QuotePolicy consistently to QuoteMode
> > > > > >
> > > > >
> > > > > Good changes!
> > > > >
> > > > >
> > > > > > Is this the final API we can agree upon?
> > > > > >
> > > > >
> > > > > Not quite, these names are still bad IMO:
> > > > >
> > > > > org.apache.commons.csv.CSVFormat.isHandlingNull()
> > > > >
> > > > > Terrible name, isNullStringSet() is simpler and expresses what the
> > code
> > > > > does. FWIW, It is also the same kind of name JAXB generates for
> this
> > > code
> > > > > pattern.
> > > > >
> > > > > org.apache.commons.csv.CSVFormat.isQuoting()
> > > > >
> > > > > isFoo should return a foo ivar. This is not the case here.
> > > > >
> > > > > org.apache.commons.csv.CSVFormat.isCommentingEnabled() return a
> > > computed,
> > > > > so the name is good.
> > > > >
> > > > > Same for org.apache.commons.csv.CSVFormat.isEscaping() but why does
> > one
> > > > > have the "Enabled" postfix and not the other? The names should be
> > > > > consistent, I say remove "Enabled".
> > > > >
> > > > > So to summarize, looking at all of:
> > > > >
> > > > > isCommentingEnabled()
> > > > > isEscaping()
> > > > > isHandlingNull()
> > > > > isQuoting()
> > > > >
> > > > > they all do the same thing: test an ivar for null.
> > > > >
> > > > > So the simple thing to do is to make sure we have a good ivar name
> > and
> > > > use
> > > > > the JAXB inspired pattern: ivar foo has a test method called
> > isFooSet()
> > > > >
> > > > > commentStart : Character
> > > > > escape : Character
> > > > > quoteChar : Character
> > > > >
> > > > > First, why is one name postFixed with Char and not the others? That
> > > > should
> > > > > be consistent. And why not use the full name "Character" in the
> > > postfix?
> > > > > Let's do that and match the method names.
> > > > >
> > > > > This also gives us room for String versions later,
> > > > getCommentStartString()
> > > > > for example, without breaking BC.
> > > > >
> > > > > For the with* methods, having the Character postfix does not make
> > > sense,
> > > > > since the type of the argument is in the signature. So
> > > > > withQuoteChar(char|Character) should be come
> > withQuote(char|Character).
> > > > >
> > > > > Now, that's better :-)
> > > > >
> > > > > My only question now is should "char delimiter" be "char
> > > delimiterChar"?
> > > > >
> > > > > See revision 1612352.
> > > > >
> > > > > Gary
> > > > >
> > > > >
> > > > > >
> > > > > > br,
> > > > > > Benedikt
> > > > > >
> > > > > >
> > > > > > 2014-07-20 14:04 GMT+02:00 Gary Gregory <garydgregory@gmail.com
> >:
> > > > > >
> > > > > > > I like using all "get" methods and no "is" methods. It
is
> simpler
> > > and
> > > > > > > makes the getters easier to access as a group with code
> > completion
> > > > IMO.
> > > > > > The
> > > > > > > with methods do not behave like Java bean method so I do
not
> > thing
> > > we
> > > > > > need
> > > > > > > to worry about that. Unless we want to register immutability...
> > > > > > >
> > > > > > > Gary
> > > > > > >
> > > > > > > <div>-------- Original message --------</div><div>From:
> Benedikt
> > > > > Ritter <
> > > > > > > britter@apache.org> </div><div>Date:07/20/2014
 04:02
> >  (GMT-05:00)
> > > > > > > </div><div>To: Commons Developers List <dev@commons.apache.org
> >
> > > > > > > </div><div>Subject: Re: [CSV] Naming pattern
of getters and
> > setters
> > > > in
> > > > > > > CSVFormat (was: Re: [VOTE] Release Commons CSV 1.0 based
on
> RC1)
> > > > > > </div><div>
> > > > > > > </div>using "get" for methods that return booleans
is very
> > uncommon
> > > > > > imho...
> > > > > > >
> > > > > > > how about leaving all the gramme stuff out and use:
> > > > > > >
> > > > > > > void withSkipEmptyHeaders(boolean)
> > > > > > > boolean isSkipEmptyHeaders
> > > > > > >
> > > > > > > that would
> > > > > > > - restore symmetry between getter and setter
> > > > > > > - almost follow JavaBean conventions (except for the "with")
> > > > > > >
> > > > > > > br,
> > > > > > > Benedikt
> > > > > > >
> > > > > > >
> > > > > > > 2014-07-20 8:00 GMT+02:00 Dipanjan Laha <dipanjan21@gmail.com
> >:
> > > > > > >
> > > > > > > > Although i am not familiar with CSV's codebase, imho
"get" is
> > > more
> > > > > > > straight
> > > > > > > > forward, so +1 to Gary's suggestion.
> > > > > > > >
> > > > > > > > On Saturday, 19 July 2014, Gary Gregory <
> > garydgregory@gmail.com>
> > > > > > wrote:
> > > > > > > >
> > > > > > > > > On Sat, Jul 19, 2014 at 12:14 PM, Emmanuel Bourg
<
> > > > > ebourg@apache.org
> > > > > > > > > <javascript:;>> wrote:
> > > > > > > > >
> > > > > > > > > > Le 19/07/2014 13:48, Gary Gregory a écrit
:
> > > > > > > > > >
> > > > > > > > > > > Can we go back to use "get"?
> > > > > > > > > >
> > > > > > > > > > We are running in circles Gary, Benedikt
and I, if others
> > > could
> > > > > > weigh
> > > > > > > > in
> > > > > > > > > > that would help.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Circles, back and forth, to and fro, call it
what you will.
> > IMO
> > > > > this
> > > > > > is
> > > > > > > > the
> > > > > > > > > nature of the kind of development we do. Decentralized,
no
> > > water
> > > > > > > cooler,
> > > > > > > > no
> > > > > > > > > white board, all emails, leads to this development
style,
> > which
> > > > is
> > > > > > what
> > > > > > > > we
> > > > > > > > > have to live with.
> > > > > > > > >
> > > > > > > > > In this case, it seems we had to try the code
several ways
> > and
> > > > see
> > > > > it
> > > > > > > > > before we can decide. In an office, we might
have decided
> in
> > > pair
> > > > > > > > > programming in 5 minutes, this is not what we
have. That or
> > > > > architect
> > > > > > > > would
> > > > > > > > > have created some coding edict that imposes coding
style.
> > > > > > > > >
> > > > > > > > > So this circling is all OK by me ;-)
> > > > > > > > >
> > > > > > > > > Gary
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Emmanuel Bourg
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > >
> > ---------------------------------------------------------------------
> > > > > > > > > > To unsubscribe, e-mail:
> dev-unsubscribe@commons.apache.org
> > > > > > > > > <javascript:;>
> > > > > > > > > > For additional commands, e-mail:
> > dev-help@commons.apache.org
> > > > > > > > > <javascript:;>
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > E-Mail: garydgregory@gmail.com <javascript:;>
|
> > > > > ggregory@apache.org
> > > > > > > > > <javascript:;>
> > > > > > > > > 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
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > http://people.apache.org/~britter/
> > > > > > > http://www.systemoutprintln.de/
> > > > > > > http://twitter.com/BenediktRitter
> > > > > > > http://github.com/britter
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > http://people.apache.org/~britter/
> > > > > > http://www.systemoutprintln.de/
> > > > > > http://twitter.com/BenediktRitter
> > > > > > http://github.com/britter
> > > > > >
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > 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
> > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > http://people.apache.org/~britter/
> > > > http://www.systemoutprintln.de/
> > > > http://twitter.com/BenediktRitter
> > > > http://github.com/britter
> > > >
> > >
> > >
> > >
> > > --
> > > 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
> > >
> >
> >
> >
> > --
> > http://people.apache.org/~britter/
> > http://www.systemoutprintln.de/
> > http://twitter.com/BenediktRitter
> > http://github.com/britter
> >
>
>
>
> --
> 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
>



-- 
http://people.apache.org/~britter/
http://www.systemoutprintln.de/
http://twitter.com/BenediktRitter
http://github.com/britter

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