Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Issue #497 and add a testcase #699

Merged
merged 4 commits into from Jun 2, 2021

Conversation

Pigdrum
Copy link
Contributor

@Pigdrum Pigdrum commented Apr 25, 2021

The method getPropertyKeys() in class JsonOrgJsonProvider doesn't check empty jsonObject. #497

@Pigdrum Pigdrum changed the title Fix Issue #497 and add a test Fix Issue #497 and add a testcase Apr 25, 2021
@@ -148,12 +148,16 @@ public boolean isMap(Object obj) {
JSONObject jsonObject = toJsonObject(obj);
Copy link

@lodenrogue lodenrogue May 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could probably simplify this method with streams (Sorry for any syntax errors. I'm writing this in the browser):

JSONObject jsonObject  = toJsonObject(obj);
try {
    if(Objects.isNull(jsonObject.names()) return new ArrayList<>();
    
    return jsonObject.names().toList().stream()
        .map(Object::toString)
        .collect(Collectors.toList());

} catch(JSONException e) {
// Rest of the code
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, thanks! I have seen all the suggestions you mentioned, and I will change it as soon as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find that the return type of jsonObject.names() is JsonArray, which doesn't have method like toList. But the jsonObject has another method keySet which return the set of key that is needed.
(PS: the implementation of jsonObject.names() is that it iteratively adds the value in keySet into JsonArray

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://stleary.github.io/JSON-java/org/json/JSONObject.html#names--
https://stleary.github.io/JSON-java/org/json/JSONArray.html#toList--

JSONObject::names should return a JSONArray
JSONArray should have a method called toList() according to their documentation.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I updated the code snippet. Instead of casting (String) name you can do Object::toString

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I find that the version of org.Json in JsonPath is 20140107, where JSONArray has no method toList.toList is introduced in version 20160807. What could I do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check my latest two commits, thanks!
The code of method .name() is as follows, I think it is not necessary to use .name(). Just using .keySet() will be fine.

public Iterator keys() {
        return this.keySet().iterator();
    }

public JSONArray names() {
        JSONArray ja = new JSONArray();
        Iterator keys = this.keys();
        while (keys.hasNext()) {
            ja.put(keys.next());
        }
        return ja.length() == 0 ? null : ja;
    }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is my first time to contribute to the open source community, sincerely thanks for your suggestions and approval !

@@ -76,4 +78,23 @@ public void read_book_length() {
assertThat(result).isEqualTo(4);
}

@Test
public void Issue497() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change method names to something more descriptive. I don't want to have to go to github to find out what this issue is.

}

@Test
public void Issue497_2() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here bud.

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

Successfully merging this pull request may close these issues.

None yet

3 participants