[GitHub] [maven] gnodet commented on pull request #819: [MNG-7553] Introduce SettingsBuilder and ToolchainsBuilder services

[GitBox <gi...@apache.org>]

gnodet commented on PR #819:
URL: https://github.com/apache/maven/pull/819#issuecomment-1274257056

   > 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.
   
   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.
   
   > * 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 right now), a bunch of exception are used to carry the _context_ in which the exception was thrown.  So you end up with untyped `source` fields or similar things.
   


-- 
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-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org