[ 
https://issues.apache.org/jira/browse/OPENNLP-1211?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Koji Sekiguchi updated OPENNLP-1211:
------------------------------------
    Description: 
I'd like to improve WindowFeatureGeneratorTest from the following perspective:

* testWindowSizeOne should check the contents of the returned features. It 
checks the length of the features only now
* most of test methods uses Assert.assertEquals(expected, actual) in opposite 
way for its arguments when checking the contents of the returned features

{code}
Assert.assertEquals(features.get(0), testSentence[testTokenIndex]);
{code}

should be

{code}
Assert.assertEquals(testSentence[testTokenIndex], features.get(0));
{code}

* Though I pointed out the arguments in assertEquals() above, I think we'd 
better use exact concrete string rather than expression such like 
testSentence[testTokenIndex] for the expected. And also, testForCorrectFeatures 
uses contains method when checking the contents of the returned features but I 
think we should avoid using contains when checking the items in a List, rather 
than writing like this:

{code}
    Assert.assertTrue(features.contains(WindowFeatureGenerator.PREV_PREFIX + 
"2" +
        testSentence[testTokenIndex - 2]));
    Assert.assertTrue(features.contains(WindowFeatureGenerator.PREV_PREFIX + 
"1" +
        testSentence[testTokenIndex - 1]));

    Assert.assertTrue(features.contains(testSentence[testTokenIndex]));

    Assert.assertTrue(features.contains(WindowFeatureGenerator.NEXT_PREFIX + 
"1" +
        testSentence[testTokenIndex + 1]));
    Assert.assertTrue(features.contains(WindowFeatureGenerator.NEXT_PREFIX + 
"2" +
        testSentence[testTokenIndex + 2]));
{code}

but I'd like to rewrite them like this:

{code}
    Assert.assertEquals("d",features.get(0));
    Assert.assertEquals("p1c",features.get(1));
    Assert.assertEquals("p2b",features.get(2));
    Assert.assertEquals("n1e",features.get(3));
    Assert.assertEquals("n2f",features.get(4));
{code}

The second form helps us to understand how WindowFeatureGenerator works and 
it's easier to read.

  was:
I'd like to improve WindowFeatureGeneratorTest from the following perspective:

* testWindowSizeOne should check the contents of the returned features. It 
checks the length of the features only now
* most of test methods uses Assert.assertEquals(expected, actual) in opposite 
way for its arguments when checking the contents of the returned features

{code}
Assert.assertEquals(features.get(0), testSentence[testTokenIndex]);
{code}

should be

{code}
Assert.assertEquals(testSentence[testTokenIndex], features.get(0));
{code}

* Though I pointed out the arguments in assertEquals() above, I think we'd 
better use exact concrete string rather than expression such like 
testSentence[testTokenIndex] for the expected. And also, testForCorrectFeatures 
uses contains method when checking the contents of the returned features but I 
think we should avoid using contains when checking the items in a List, rather 
than writing like this:

{code}
    Assert.assertTrue(features.contains(WindowFeatureGenerator.PREV_PREFIX + 
"2" +
        testSentence[testTokenIndex - 2]));
    Assert.assertTrue(features.contains(WindowFeatureGenerator.PREV_PREFIX + 
"1" +
        testSentence[testTokenIndex - 1]));

    Assert.assertTrue(features.contains(testSentence[testTokenIndex]));

    Assert.assertTrue(features.contains(WindowFeatureGenerator.NEXT_PREFIX + 
"1" +
        testSentence[testTokenIndex + 1]));
    Assert.assertTrue(features.contains(WindowFeatureGenerator.NEXT_PREFIX + 
"2" +
        testSentence[testTokenIndex + 2]));
{code}

but I'd like to rewrite them like this:

{code}
    Assert.assertTrue("d",features.get(0));
    Assert.assertTrue("p1c",features.get(1));
    Assert.assertTrue("p2b",features.get(2));
    Assert.assertTrue("n1e",features.get(3));
    Assert.assertTrue("n2f",features.get(4));
{code}

The second form helps us to understand how WindowFeatureGenerator works and 
it's easier to read.


> Improve WindowFeatureGeneratorTest
> ----------------------------------
>
>                 Key: OPENNLP-1211
>                 URL: https://issues.apache.org/jira/browse/OPENNLP-1211
>             Project: OpenNLP
>          Issue Type: Test
>          Components: Build, Packaging and Test
>    Affects Versions: 1.9.0
>            Reporter: Koji Sekiguchi
>            Priority: Trivial
>             Fix For: 1.9.1
>
>
> I'd like to improve WindowFeatureGeneratorTest from the following perspective:
> * testWindowSizeOne should check the contents of the returned features. It 
> checks the length of the features only now
> * most of test methods uses Assert.assertEquals(expected, actual) in opposite 
> way for its arguments when checking the contents of the returned features
> {code}
> Assert.assertEquals(features.get(0), testSentence[testTokenIndex]);
> {code}
> should be
> {code}
> Assert.assertEquals(testSentence[testTokenIndex], features.get(0));
> {code}
> * Though I pointed out the arguments in assertEquals() above, I think we'd 
> better use exact concrete string rather than expression such like 
> testSentence[testTokenIndex] for the expected. And also, 
> testForCorrectFeatures uses contains method when checking the contents of the 
> returned features but I think we should avoid using contains when checking 
> the items in a List, rather than writing like this:
> {code}
>     Assert.assertTrue(features.contains(WindowFeatureGenerator.PREV_PREFIX + 
> "2" +
>         testSentence[testTokenIndex - 2]));
>     Assert.assertTrue(features.contains(WindowFeatureGenerator.PREV_PREFIX + 
> "1" +
>         testSentence[testTokenIndex - 1]));
>     Assert.assertTrue(features.contains(testSentence[testTokenIndex]));
>     Assert.assertTrue(features.contains(WindowFeatureGenerator.NEXT_PREFIX + 
> "1" +
>         testSentence[testTokenIndex + 1]));
>     Assert.assertTrue(features.contains(WindowFeatureGenerator.NEXT_PREFIX + 
> "2" +
>         testSentence[testTokenIndex + 2]));
> {code}
> but I'd like to rewrite them like this:
> {code}
>     Assert.assertEquals("d",features.get(0));
>     Assert.assertEquals("p1c",features.get(1));
>     Assert.assertEquals("p2b",features.get(2));
>     Assert.assertEquals("n1e",features.get(3));
>     Assert.assertEquals("n2f",features.get(4));
> {code}
> The second form helps us to understand how WindowFeatureGenerator works and 
> it's easier to read.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Reply via email to