Re: RFR - JDK-8203442 String::transform (Code Review) (was: RFR - JDK-8203442 String::transform (Code Review))

[Stephen Colebourne]

I'm very uncomfortable about adding this method as is.

Firstly, as Peter says (in a different thread), naming this method
`map()` is inconsistent with `Stream` and `Optional`. Adding `map()`
to `String` would be:
  public String map(Function);
Not that such a method would be particularly useful!

Secondly, the premise of the method is effectively an alternative to
the language feature "extension methods". While the motivation in
https://bugs.openjdk.java.net/browse/JDK-8203703 and
https://dzone.com/articles/making-java-fluent-api-more-flexible makes
some sense, it is clearly equally applicable to all types - `String`
is not special here. An isolated change here just looks odd and ill
thought through - a big mistake for the core `String` class.

My preference is for the change to not proceed without consideration
of the broader context. The change should not be added without a
commitment to also add the same method to Stream and Optional as a
minimum, something that would make the addition part of a more
consistent broader design. Doing this would also mandate a different
method name.

FWIW, were such a method added to java.time.* as the OP suggested, it
would enable pluggable conversions:

  java.util.Date ju = LocalDate.now()
.plusDays(2)
.apply(MyUtility::convertToOldDate);

Clearly there is some value to the above in terms of abstraction.
However, there would also be problems making the best choice of types.
Does it go on the concrete LocalDate or the interface Temporal. Its a
problem just like String vs CharSequence.

TLDR, this change needs to be part of a broader context to be
acceptable (something that would inform the best name). Without that
broader context, I'm against this change.

Stephen


On Mon, 12 Nov 2018 at 14:05, Jim Laskey  wrote:
>
> updated webrev: 
> http://cr.openjdk.java.net/~jlaskey/8203442/webrev-02/index.html 
> <http://cr.openjdk.java.net/~jlaskey/8203442/webrev-02/index.html>
> > On Sep 21, 2018, at 7:42 AM, Remi Forax  wrote:
> >
> > - Mail original -
> >> De: "Alan Bateman" 
> >> À: "Jim Laskey" , "core-libs-dev" 
> >> 
> >> Envoyé: Vendredi 21 Septembre 2018 12:22:42
> >> Objet: Re: RFR - JDK-8203442 String::transform (Code Review)
> >
> >> On 18/09/2018 18:52, Jim Laskey wrote:
> >>> Please review the code for String::transform. The goal is to provide a 
> >>> String
> >>> instance method to allow function application of custom transformations 
> >>> applied
> >>> to an instance of String.
> >>>
> >>> webrev: http://cr.openjdk.java.net/~jlaskey/8203442/webrev/index.html
> >>> jbs: https://bugs.openjdk.java.net/browse/JDK-8203442
> >>> csr: https://bugs.openjdk.java.net/browse/JDK-8203703
> >> I hate to bring up naming but if I have a Stream or
> >> Optional then I'll use the "map" method to apply the mapping
> >> function. Has this already been through the naming bike shed and it
> >> settled on "transform"? Maybe a different name was chosen after looking
> >> at code that would use it in stream operations? Maybe "transform" was
> >> used as the argument to the function is always text? I'm not interested
> >> in re-opening any discussions, just trying to see if options are
> >> captured in a mail somewhere.
> >>
> >> I'm also wondering if this is general enough to be defined by
> >> CharSequence. Retrofitting CharSequence is always problematic and never
> >> done lightly but I'm just wondering if it has been discussed and
> >> dismissed. I don't think it could be done at a later time, at least not
> >> without changing the input type parameter so that the mapping function
> >> is Function rather than Function.
> >
> > the main issue is that a lot of utility methods take a String as parameter, 
> > not a CharSequence :(
> > and Function is a supertype not a subtype of Function > super CharSequence, R>.
> >
> >>
> >> -Alan
> >
> > Rémi
>

Re: RFR - JDK-8203442 String::transform (Code Review) (was: RFR - JDK-8203442 String::transform (Code Review))

[Andrej Golovnin]

Hi Jim,

> updated webrev: 
> http://cr.openjdk.java.net/~jlaskey/8203442/webrev-02/index.html 
> 

src/java.base/share/classes/java/lang/String.java

2983  * @param   class of the result

Maybe "the type of the result" would be better.

Best regards,
Andrej Golovnin

RFR - JDK-8203442 String::transform (Code Review) (was: RFR - JDK-8203442 String::transform (Code Review))

[Jim Laskey]

updated webrev: 
http://cr.openjdk.java.net/~jlaskey/8203442/webrev-02/index.html 
<http://cr.openjdk.java.net/~jlaskey/8203442/webrev-02/index.html>




> On Sep 21, 2018, at 7:42 AM, Remi Forax  wrote:
> 
> - Mail original -
>> De: "Alan Bateman" 
>> À: "Jim Laskey" , "core-libs-dev" 
>> 
>> Envoyé: Vendredi 21 Septembre 2018 12:22:42
>> Objet: Re: RFR - JDK-8203442 String::transform (Code Review)
> 
>> On 18/09/2018 18:52, Jim Laskey wrote:
>>> Please review the code for String::transform. The goal is to provide a 
>>> String
>>> instance method to allow function application of custom transformations 
>>> applied
>>> to an instance of String.
>>> 
>>> webrev: http://cr.openjdk.java.net/~jlaskey/8203442/webrev/index.html
>>> jbs: https://bugs.openjdk.java.net/browse/JDK-8203442
>>> csr: https://bugs.openjdk.java.net/browse/JDK-8203703
>> I hate to bring up naming but if I have a Stream or
>> Optional then I'll use the "map" method to apply the mapping
>> function. Has this already been through the naming bike shed and it
>> settled on "transform"? Maybe a different name was chosen after looking
>> at code that would use it in stream operations? Maybe "transform" was
>> used as the argument to the function is always text? I'm not interested
>> in re-opening any discussions, just trying to see if options are
>> captured in a mail somewhere.
>> 
>> I'm also wondering if this is general enough to be defined by
>> CharSequence. Retrofitting CharSequence is always problematic and never
>> done lightly but I'm just wondering if it has been discussed and
>> dismissed. I don't think it could be done at a later time, at least not
>> without changing the input type parameter so that the mapping function
>> is Function rather than Function.
> 
> the main issue is that a lot of utility methods take a String as parameter, 
> not a CharSequence :(
> and Function is a supertype not a subtype of Function super CharSequence, R>.
> 
>> 
>> -Alan
> 
> Rémi