asu-ke-web-services / search-api

Search API for documents, data, research, people, etc
MIT License
2 stars 1 forks source link

Warnings about for-loops #37

Closed iajohns1 closed 8 years ago

iajohns1 commented 8 years ago

I use a for-loop to test the individual contents of the array returned by tagger_service() in ner-tagger.php, but I get a warning. Is there a better way to write this test?

I also get this warning when testing a function that contains a FOR loop as well even if the test itself does not contain a for-loop. What conventions should I follow here?

idmontie commented 8 years ago

Code snippet please.

iajohns1 commented 8 years ago

1st Warning (ner-tagger.php)

  public function results_to_keywords( $tagger_results ) {
    if ( $tagger_results === null ) {
      return null;
    }
    // FIRST WARNING OCCURS BELOW
    for ( $count = 0; $count < count( $tagger_results ); $count++ ) {
      $keywords[] = new Keyword( $tagger_results[ $count ][0], $tagger_results[ $count ][1], 1.0 / ( $count + 1 ) );
    }
    return $keywords;
  }

2nd Warning (NerTaggerSpec.php)

function it_should_return_nested_array() {
    // Test a single function call instead of several
    $test_result = $this->tagger_service( 'test' );
    $test_result->shouldBeArray();
    // SECOND WARNING OCCURS BELOW
    for ( $count = 0; $count < count( $test_result ); $count++ ) {
      $test_result[ $count ]->shouldBeArray();
      $test_result[ $count ]->shouldHaveCount( 2 );
      $test_result[ $count ][0]->shouldBeString();
      $test_result[ $count ][1]->shouldBeString();
    }
    // Tests only for dummy return values
    $test_result->shouldHaveCount( 6 );
    $test_result[0][0]->shouldReturn( 'Phoenix' );
    $test_result[0][1]->shouldReturn( 'LOCATION' );
  }
idmontie commented 8 years ago

Use foreach instead of for ( $i = 0; $i < $count; $i++) type loops.

Also, I don't like loops in tests, but @rraub and I will disagree on that.

iajohns1 commented 8 years ago

Is it acceptable to just test the first component in the array instead of the whole thing?

rraub commented 8 years ago

Ok, I see whats going on. Yeah the issue is your calling count() within the loop conditional and I would do what @idmontie say's and use a foreach. Foreach is much easier to read and less logic on your end.

For your tests, you can do a equality checks like you had:

    $test_result->shouldHaveCount( 6 );
    $test_result[0][0]->shouldReturn( 'Phoenix' );
    $test_result[0][1]->shouldReturn( 'LOCATION' );

Depending on your test case, there may be a good application for looping over the results and having test assertions on each one.