[
https://issues.apache.org/jira/browse/STORM-1228?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15173934#comment-15173934
]
ASF GitHub Bot commented on STORM-1228:
---------------------------------------
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/1160#issuecomment-190779747
I would prefer to see getFields inlined everywhere. We are not saving
enough code to really make it worth it.
```
@Test(expected = IndexOutOfBoundsException.class)
public void getThrowsWhenOutOfBoundsTest() {
Fields fields = getFields(); // only has two items
fields.get(2);
}
```
requires the comment `// only has two items` but
```
@Test(expected = IndexOutOfBoundsException.class)
public void getThrowsWhenOutOfBoundsTest() {
Fields fields = new Fields("a", "b");
fields.get(2);
}
```
is much more readable and needs no comment. If getFields were a large
method that required a lot of setup then it would be OK, but for this it is
small enough I think it should be inlined.
As for the NPE. I don't really want to touch that code just yet. If you
want to file a JIRA with a reproducible use case we can look at it and see what
ramifications a change might have.
> port backtype.storm.fields-test to java
> ----------------------------------------
>
> Key: STORM-1228
> URL: https://issues.apache.org/jira/browse/STORM-1228
> Project: Apache Storm
> Issue Type: New Feature
> Components: storm-core
> Reporter: Robert Joseph Evans
> Assignee: Alessandro Bellina
> Labels: java-migration, jstorm-merger
>
> This is a test that should be simple to move to JUnit
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
