Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Return source as String + Cleanup #249

Merged
merged 3 commits into from
Nov 2, 2015
Merged

Return source as String + Cleanup #249

merged 3 commits into from
Nov 2, 2015

Conversation

nicoruti
Copy link
Contributor

Adds method JestResult#getSourceString() (incl. tests in JestResultTest)

In addition:

  • cleaned up some unused imports and variables
  • make use of org.junit instead of junit.assert.
  • JestHttpClient needs not to implement JestClient, as it already extends AbstractJestClient which implements it.

nicoruti added 2 commits October 21, 2015 22:27
- Unused imports
- Unused variables
- Duplicate entries in pom.xml
incl. test cases
@kramer
Copy link
Member

kramer commented Nov 1, 2015

Hi @nicoruti ,

Sorry for the late reply. I think naming the method getSourceAsString would be more in line with the existing methods getSourceAsObject and getSourceAsObjectList. Also going through the regular parser methods when converting to string seems unnecessary (especially for that ES_METADATA_ID field) so why not do something like this:

    public String getSourceAsString() {
        String[] keys = getKeys();
        if(!isSucceeded || jsonObject == null || keys == null || keys.length == 0 || !jsonObject.has(keys[0])) {
            return null;
        }

        JsonElement obj = jsonObject.get(keys[0]);
        for (int i = 1; i < keys.length; i++) {
            obj = ((JsonObject) obj).get(keys[i]);
        }
        return obj.toString();
    }

@nicoruti
Copy link
Contributor Author

nicoruti commented Nov 2, 2015

Hi @kramer

Thanks for your remarks. Yes, the method getSourceAsString fits the naming convention. I changed it as you proposed. I also used your code snippet, as it's more performant than the 'original'.

kramer pushed a commit that referenced this pull request Nov 2, 2015
Add `JestResult.getSourceAsString` and overall cleanup
@kramer kramer merged commit 7426b86 into searchbox-io:master Nov 2, 2015
@kramer
Copy link
Member

kramer commented Nov 2, 2015

Thank you for the contribution @nicoruti !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants