cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From miguelaferreira <...@git.apache.org>
Subject [GitHub] cloudstack pull request: CLOUDSTACK-9074: Support shared networkin...
Date Tue, 01 Dec 2015 10:45:42 GMT
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.
---

Mime
View raw message