michael-o commented on PR #819:
URL: https://github.com/apache/maven/pull/819#issuecomment-1274619037
> > I see two general issues/questions here:
> > I would rather have had those discussions when I asked for feedback on
the API, but I suppose it's not too late ;-)
> > See my answers / reasoning below.
>
> > * We have already briefly discussed the benefit/overrating of
`Optional`. Can you explain why it makes sense here? I mean `X != null` is as
good as here.
>
> I slightly disagree. The main differences are that when using an
`Optional`, the code carries the semantics and actually forces the caller to
deal with it, whereas just if you simply return a value (the optionals are only
used as return values), you have to get to the definition, read the javadoc, so
that you can know if you need to deal with a possible null value. There are
very often places in our code where you see both null checks and not and people
end up adding null checks, just in case.
>
> This also hurts code concision. A simple example found in the code:
>
> ```
> if ( getBaseVersionInternal() != null )
> {
> sb.append( getBaseVersionInternal() );
> }
> else
> {
> sb.append( versionRange.toString() );
> }
> ```
>
> This could be rewritten as:
>
> ```
> sb.append( getBaseVersionInternal().orElse( versionRange.toString()
) );
> ```
>
> This is actually related to the fact that the maven coding style is very
sparse. I actually find that quite difficult to work with (even if I'm getting
used to it). That may be due to the fact I'm working almost exclusively on a
laptop, so the height of it is limited, and only seing 30-40 lines of code at
the same time becomes a problem when nearly 50% of those don't really convey
any semantic (being blank lines or braces usually). It means more time to
scroll through the code back and forth to understand it. I don't like that.
Streams are another way to get around empty lines, just because often, you
simply don't need braces when using them, so you have 1 line instead of 3.
>
> So generally speaking, the more semantic carried by the code, the better.
I agree to not overuse `Optional`, I just don't think they are at this point.
That said, the use of `Nullable` / `NonNull` annotations does help with
semantics already, and I would not be opposed to removing them on requests /
results objects.
This reasoning I can follow where `Optional` adds value. I don't like like
to proclaim `Optional` to be the new `null` which is non sense. No objections
here.
> > * I have a general problem with the terms
`XBuilderRequest`/`XBuilderResult`/`XBuilderException`. I think this is
incorrectly names since it implies that there is something with the builder and
not the build itself. In other spots we use the terms
`XBuildingRequest`/`XBuildingResult`/`XBuildingException`/`XBuildingProblem` or
just `Build`. @elharo Sample:
https://github.com/apache/maven/blob/35b93b0a589752cc88105623a2ddf9e52b56c1ce/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuildingRequest.java
>
> I'm actually seeing the terms as _a request to the X builder_, _the result
returned by X builder_, _the exception thrown by X builder_. We can't use
quotes, but think about those as _XBuilder's request_, _XBuilder's result_,
_XBuilder's exception_. I actually think those make more sense than what we
have in other places. It's also easier to see the relationship between the
service and its related request/result/exception. Also, if you think each
service usually throws _its own exception_. If we move the semantic to _what
action is performed_ instead of _who does the action_, the exceptions would
have to be more shared between services. However (and that's still missing on
some right now), a bunch of exception are used to carry the _context_ in which
the exception was thrown.
>
> Another point that just came to my mind is that wording like
`DependencyCollectionRequest` is not intuitive either: the term `Collection` in
java is immediately associated to a... `java.util.Collection`, and not to the
act of collecting...
Let me crunch on this. @elharo What is your opinion on the style here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
