[GitHub] [maven-scm] michael-o commented on a change in pull request #124: Simplify conditions with the same result and avoid extra validations.

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

michael-o commented on a change in pull request #124:
URL: https://github.com/apache/maven-scm/pull/124#discussion_r702321513



##########
File path: maven-scm-providers/maven-scm-provider-integrity/src/main/java/org/apache/maven/scm/provider/integrity/Sandbox.java
##########
@@ -99,7 +99,7 @@ public static String formatFilePatterns( String pattern )
                 {
                     sb.append( tkn );
                 }
-                sb.append( i < tokens.length ? "," : "" );
+                sb.append(",");

Review comment:
       Please explain

##########
File path: maven-scm-providers/maven-scm-provider-integrity/src/main/java/org/apache/maven/scm/provider/integrity/Member.java
##########
@@ -247,10 +247,7 @@ public boolean equals( Object o )
     {
         if ( o instanceof Member )
         {
-            if ( null != o )
-            {
-                return ( (Member) o ).getMemberName().equals( this.getMemberName() );
-            }
+            return ( (Member) o ).getMemberName().equals( this.getMemberName() );

Review comment:
       Assumption is tht if o is instance of `Member` it cannot be null?

##########
File path: maven-scm-providers/maven-scm-providers-svn/maven-scm-provider-svnexe/src/main/java/org/apache/maven/scm/provider/svn/svnexe/command/status/SvnStatusConsumer.java
##########
@@ -169,20 +169,9 @@ else if ( statusString.equals( "I" ) )
             //Parse the second column
             statusString = line.substring( 1, 1 );
 
-            if ( statusString.equals( "M" ) )
-            {
-                status = ScmFileStatus.MODIFIED;
-            }
-            else if ( statusString.equals( "C" ) )
-            {
-                status = ScmFileStatus.CONFLICT;
-            }
-            else
-            {
-                //The line isn't a status line, ie something like 'Performing status on external item at...'
-                //or a status defined in next columns
-                return;
-            }
+            //The line isn't a status line, ie something like 'Performing status on external item at...'
+            //or a status defined in next columns
+            return;

Review comment:
       Please explain

##########
File path: maven-scm-providers/maven-scm-providers-git/maven-scm-provider-gitexe/src/main/java/org/apache/maven/scm/provider/git/gitexe/command/status/GitStatusConsumer.java
##########
@@ -203,7 +203,7 @@ else if ( ( matcher = RENAMED_PATTERN.matcher( line ) ).find() )
         }
 
         // If the file isn't a file; don't add it.
-        if ( !files.isEmpty() && status != null )
+        if ( !files.isEmpty() )

Review comment:
       The else block does not cover status. Please explain.

##########
File path: maven-scm-api/src/main/java/org/apache/maven/scm/CommandParameters.java
##########
@@ -53,11 +53,6 @@ public String getString( CommandParameter parameter )
     {
         Object object = getObject( String.class, parameter );
 
-        if ( object == null )
-        {
-            throw new ScmException( "Missing parameter: '" + parameter.getName() + "'." );
-        }
-

Review comment:
       Well, this will change to NPE. Please explain.

##########
File path: maven-scm-providers/maven-scm-provider-integrity/src/main/java/org/apache/maven/scm/provider/integrity/command/mkdir/IntegrityMkdirCommand.java
##########
@@ -66,7 +66,7 @@ public MkdirScmResult executeMkdirCommand( ScmProviderRepository repository, Scm
         {
             dirPath = fit.next().getPath().replace( '\\', '/' );
         }
-        if ( null == dirPath || dirPath.length() == 0 )
+        if (dirPath.length() == 0 )

Review comment:
       Space missing. `isEmpty()`?

##########
File path: maven-scm-providers/maven-scm-provider-jazz/src/main/java/org/apache/maven/scm/provider/jazz/JazzScmProvider.java
##########
@@ -222,7 +222,7 @@ public ScmProviderRepository makeProviderScmRepository( String scmUrl, char deli
             // So we will set port to zero.
             int protocolIndex = jazzUrl.indexOf( ":" ) + 3;     // The +3 accounts for the "://"
             int pathIndex = jazzUrl.indexOf( "/", protocolIndex + 1 );
-            if ( ( protocolIndex != -1 ) && ( pathIndex != -1 ) )
+            if ( pathIndex != -1 )

Review comment:
       Please explain




-- 
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