dialogflow / dialogflow-android-client

Android SDK for Dialogflow
https://dialogflow.com
Apache License 2.0
575 stars 270 forks source link

[OBSOLETE] GoogleRecognitionServiceImpl: Sort results by rate, solving #61 #62

Closed PLNech closed 6 years ago

PLNech commented 7 years ago

Sorts the recognitionResults in place, using an Optimized Bubble Sort.

I wrote a test, but not being sure if you want to test ai.api.services I didn't create a new TestCase. Let me know if you want one wrapping the test code:

private static final ArrayList<String> texts = new ArrayList<>(Arrays.asList("9", "2", "5", "4"));
private static final Float[] rates = new Float[]{0.9f, 0.2f, 0.5f, 0.4f};

    @Test
    public void sortTextWithRates() throws Exception {
        sortResultsByRate(texts, rates);
        Assert.assertEquals("9", texts.get(0));
        Assert.assertEquals("5", texts.get(1));
        Assert.assertEquals("4", texts.get(2));
        Assert.assertEquals("2", texts.get(3));
    }
PLNech commented 7 years ago

@xVir, let me know if this looks good to you 🙂

PLNech commented 6 years ago

@xVir any update/feedback on this?

xVir commented 6 years ago

Hi @PLNech Thank you for the pull request!

But what do you think about use Arrays.sort for sorting? https://stackoverflow.com/questions/13201573/sorting-an-array-using-comparator

PLNech commented 6 years ago

Hi @xVir, thanks for your reply!

Good feedback, but what would you put in compare(String o1, String o2) to have a pairwise comparison based on the rates? I could write:

@Override public int compare(String o1, String o2) {
    return rates[recognitionResults.indexOf(o2)] - rates[recognitionResults.indexOf(o1)];
}

But doing an index lookup for each comparison does not sound very efficient.

xVir commented 6 years ago

Ok, Thanks for your answer! Could you please add this unit test to the source code?

Thanks!

PLNech commented 6 years ago

Hi @xVir, thanks for your answer.

I meant to add the test as-is, but as the tests don't have the same package name I can't test a protected method from ai.api.test.XXXTest. I'm left with a few options:

  1. Extract sortResultsByRate to a SortHelper class and write an SortHelperTest
  2. Refactor sortResultsByRate as a public method
  3. Use reflection to test sortResultsByRate as a private method

Considering 1 and 2 would increase bloat in the project, I considered using reflection in one test case is an acceptable tradeoff (in my testing it still ran in an acceptable time, 0.024s). Let me know if you have a better alternative!

PLNech commented 6 years ago

Closing and reopening as #77 as since the project renaming I can't add new commits to this PR.