[GitHub] [maven-checkstyle-plugin] bmhm commented on issue #17: [MCHECKSTYLE-385] rework code to use a Violation.java class.

[GitBox]

bmhm commented on issue #17: [MCHECKSTYLE-385] rework code to use a 
Violation.java class.
URL: 
https://github.com/apache/maven-checkstyle-plugin/pull/17#issuecomment-565035024
 
 
   Another alternative to the `null` Pattern is to use Java 8's `Optional`s 
like so:
   
   ```patch
   Index: src/main/java/org/apache/maven/plugins/checkstyle/Violation.java
   <+>UTF-8
   ===================================================================
   --- src/main/java/org/apache/maven/plugins/checkstyle/Violation.java 
(revision e80fe9d690ade24ac0370a82f7a3c7fbfd11e264)
   +++ src/main/java/org/apache/maven/plugins/checkstyle/Violation.java (date 
1576161274328)
   @@ -20,6 +20,7 @@
     */
    
    import java.util.Objects;
   +import java.util.Optional;
    import java.util.StringJoiner;
    
    /**
   @@ -28,15 +29,13 @@
    public class Violation
    {
    
   -  public static final String NO_COLUMN = "-1";
   -
      private final String source;
    
      private final String file;
    
      private final String line;
    
   -  private String column = NO_COLUMN;
   +  private /* Nullable */ String column = null;
    
      private final String severity;
    
   @@ -98,16 +97,16 @@
        return line;
      }
    
   -  protected String getColumn( )
   +  protected Optional<String> getColumn( )
      {
   -    return column;
   +    return Optional.ofNullable( column );
      }
    
      protected void setColumn( /* Nullable */ String column )
      {
        if ( null == column || column.length() < 1 )
        {
   -      this.column = NO_COLUMN;
   +      this.column = null;
          return;
        }
    
   Index: 
src/main/java/org/apache/maven/plugins/checkstyle/CheckstyleViolationCheckMojo.java
   <+>UTF-8
   ===================================================================
   --- 
src/main/java/org/apache/maven/plugins/checkstyle/CheckstyleViolationCheckMojo.java
      (revision e80fe9d690ade24ac0370a82f7a3c7fbfd11e264)
   +++ 
src/main/java/org/apache/maven/plugins/checkstyle/CheckstyleViolationCheckMojo.java
      (date 1576161399620)
   @@ -31,6 +31,7 @@
    import java.util.Collections;
    import java.util.List;
    import java.util.Map;
   +import java.util.Optional;
    
    import org.apache.maven.artifact.Artifact;
    import org.apache.maven.model.Dependency;
   @@ -712,10 +713,11 @@
                .filter( violation -> !ignore( ignores, violation.getSource() ) 
)
                .forEach( violation ->
                {
   +                final Optional<String> column = violation.getColumn();
                    final String message = String.format( "%s:[%s%s] (%s) %s: 
%s",
                        violation.getFile(),
                        violation.getLine(),
   -                    ( Violation.NO_COLUMN.equals( violation.getColumn() ) ) 
? "" : ( ',' + violation.getColumn() ),
   +                    column.isPresent() ? ',' + column.orElse( "-1" ) : "",
                        violation.getCategory(),
                        violation.getRuleName(),
                        violation.getMessage() );
   ```
   
   The "-1" cannot happen if we pull out the column variable first. Alternative 
to `.orElse("-1")` is to use `.orElseThrow(IllegalStateException::new)` 
(because this is an illegal state). The Java 10's  `orElseThrow()` does throw 
another exception, which is `NoSuchElementException`. But if you pull out the 
Optional as a variable and check it, it's state cannot be modified anymore -- 
hence the impossible `IllegalStateException`.
   
   Just for offering an alternative.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services