[GitHub] cloudstack pull request: CLOUDSTACK-9074: Support shared networkin...

[miguelaferreira <gi...@git.apache.org>]

Github user miguelaferreira commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/1094#discussion_r46263781
  
    --- Diff: api/src/com/cloud/network/Networks.java ---
    @@ -251,6 +252,10 @@ public static URI fromString(String candidate) {
                     if (com.cloud.dc.Vlan.UNTAGGED.equalsIgnoreCase(candidate)) {
                         return Native.toUri(candidate);
                     }
    +                if (UuidUtils.validateUUID(candidate)){
    +                    //Supporting lswitch uuid as vlan id: set broadcast_uri = null and add row on nicira_nvp_router_map with logicalrouter_uuid = candidate
    +                    return null;
    --- End diff --
    
    Ok, I understand the intention now. However, you could make that intention clear in the code. 
    
    You use exception handling to deal with expected behaviour because you implement the expected behaviour in the `catch`block. Given the state-of-the-practive in this project I would say that is normal, and I accept what you did.
    
    Still, if you would like to make it better, you could create a method `private boolean isVlanId(String candidate)` that has the `try...catch` in there (with the same construction you have now) but returns `true`or `false`. Then in the `fromString` method you can call the new method, and instead of having your logic in the `catch` block you will have it in a `else` block. This will make clear that the code is not handling an exceptional (error) case.
    It would look a bit like this:
    ```java
    (...)
    
    private boolean isVlanId(String candidate) {
      try {
        Long.parseLong(candidate);
        return true;
      } catch (NumberFormatException nfe) {
        return false;
      }
    }
    
    (...)
    
    if(isVlanId(candidate)) {
      return Vlan.toUri(candidate);
    } else {
      (...)
    }
    
    (...)
    ```
    
    In addition, instead of returning `null` (because that is a very bad practice, and opens up an entire category of errors, a.k.a. null dereferencing) you could return Optional<URI> and then in the method that calls `fromString` you need to handle the case of an existing URI or a non-existing URI. Optional has an [API](https://code.google.com/p/guava-libraries/wiki/UsingAndAvoidingNullExplained) for that.
    The idea behind this recommendation is that we express the fact that we might have an URI or not, in a safer and more meaningful way that returning `null`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---