commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CLI-284) Inconsistency in constraints for creating an instance of class Option using builder pattern or constructor
Date Sun, 10 Jun 2018 00:03:00 GMT

    [ https://issues.apache.org/jira/browse/CLI-284?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16507206#comment-16507206
] 

ASF GitHub Bot commented on CLI-284:
------------------------------------

Github user kinow commented on the issue:

    https://github.com/apache/commons-cli/pull/25
  
    Hi @sfuhrm thanks for the pull request. I had a look at the CLI-284 issue in JIRA, and
also checked out the pull request locally to take a look in Eclipse.
    
    I believe we have some tests failing due to this change. See the Travis-CI build log,
or try running `mvn clean test` locally. I got the following in my environment:
    
    ```
    Tests run: 410, Failures: 0, Errors: 55, Skipped: 54
    ```
    
    The only other comment I have is about the duplicated code that we have now.
    
    `Option`'s constructor checks for either `opt` or `longOpt` being present. But so does
`Option$Builder#build()`.
    
    What about moving the 
    
    ```java
    if (opt == null && longOpt == null)
    {
        throw new IllegalArgumentException("Either opt or longOpt must be specified");
    }
    ```
    
    check to `OptionValidator`? Maybe that way we avoid having the same if in two places,
and running the risk of someday changing one without changing the other (though tests should
catch it).
    
    Thanks!


> Inconsistency in constraints for creating an instance of class Option using builder pattern
or constructor
> ----------------------------------------------------------------------------------------------------------
>
>                 Key: CLI-284
>                 URL: https://issues.apache.org/jira/browse/CLI-284
>             Project: Commons CLI
>          Issue Type: Bug
>            Reporter: Dilraj Singh
>            Assignee: Bruno P. Kinoshita
>            Priority: Major
>         Attachments: CLI284.patch
>
>
> Builder pattern for creating an instance of class *Option* ensures that at least one
of the representation (short and long representation) for the option is not null. It throws
an *IllegalArgumentException* in-case program tries to build an instance with both the representations
as null. Consider the following code snippet for reference.
> {code:java}
> public static void main(String[] args) {
>   Option.Builder builder = Option.builder();
>   Option op = builder.build();
> } 
> {code}
> This piece of code fails with the following exception
> {noformat}
> Exception in thread "main" java.lang.IllegalArgumentException: Either opt or longOpt
must be specified
> {noformat}
> But if we try to create an instance of Option by calling its constructor, It allows the
creation with both opt and longOpt for Option as null. Consider the following code snippet
for reference
> {code:java}
> public static void main(String[] args) {
>   Option op = new Option(null, null);
>   System.out.format("Short Representation: %s\n" +
>   "Long Representation: %s", op.getOpt(), 
>   op.getLongOpt());
> }
> {code}
> Output:
> {noformat}
> Short Representation: null
> Long Representation: null
> {noformat}
> Calling a method on an instance with both opt and longOpt as null will lead to undesired
null pointer exception. For example, calling
> {code:java}
>  op.getId() {code}
> will throw a null pointer exception, thus violating the method contract.
> So as to fix this we need to make sure that whenever a constructor is invoked it has
a non-null argument value for at least one of the Option representation. 



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Mime
View raw message