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

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

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



##########
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:
       `instanceof` always return `false` if `o` is `null`.

##########
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:
       In fact,  now double checking, `!files.isEmpty` is always true. files always have data

##########
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:
       It is always <tokens.length, otherwise it would exit in the loop conditions.
   ` for ( int i = 0; i < tokens.length; i++ )`

##########
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:
       two line above it add 3 to account, so, in the worst case. would be -1 + 3 

##########
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:
       "M" and ¨C" are cover in previous checks
   
   `else if ( statusString.equals( "M" ) || statusString.equals( "R" ) || statusString.equals( "~" ) )`
   and
   `  else if ( statusString.equals( "C" ) )
           {
               status = ScmFileStatus.CONFLICT;
           }`

##########
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:
       Agree. Much better. TY

##########
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:
       there is no options that status can be null. In the worst case will be exit in the else
   
   ` else
           {
               logger.warn( "Ignoring unrecognized line: " + line );
               return;
           }`

##########
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:
       Hi @michael-o 
   Never can be null. Is controlled in the private method ` private Object getObject( Class<?> clazz, CommandParameter parameter )`




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