elek commented on pull request #1206:
URL: https://github.com/apache/hadoop-ozone/pull/1206#issuecomment-666305595
Thanks the update @sadanand48. Yes it looks better (and significant better)
for me.
I realized two other NITs in the test.
1. `@Test(expected = IllegalArgumentException.class)`
When you use complex test cases (which multiple assertions) it's not safe to
use the `expected` attribute. Actually this test passes even if the method
*always* returns with IllegalArgumentException as the remaining asserstions
will be ignored in this case.
For example the last line, is ignored in the current patch.
```
Assert.assertEquals(0, keyNames.size());
```
It's never called.
I think it's more safe to check if that specific call throws an exception or
not:
```
try {
getKeyNames(dbScanner, "keyTable", keyNames);
Assert.fail("Illegal argument exception is expected");
} catch (IllegalArgumentException ex) {
}
```
2. Is more minor, but the readability can be improved with using
```
List<String> keyNames = getKeyNames(dbScanner, "keyTable");
```
instead of
```
getKeyNames(dbScanner, "keyTable", keyNames);
```
I know that it's possible to pass `keyNames` as reference and reset and
modify the content, but the behavior seems to be more obvious (easier to read)
if you just return with the selected values.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
