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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
