Use visitor for json creation of locations #170
Conversation
| /** A singleton instance of this visitor. */ | ||
| public static final LocationToJsonVisitor INSTANCE = new LocationToJsonVisitor(); |
There was a problem hiding this comment.
Why make this a singleton? Since there is no state in the object, and allocating the object should be cheap, I think this is overkill / premature optimization.
There was a problem hiding this comment.
Thank you for noticing this. I thought since there is not state, we should make it singleton. But as you said, this is cheap and not a good optimization overall. I fixed it here af081c1.
| INDEX | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") |
There was a problem hiding this comment.
Why do we need these unchecked suppressions?
There was a problem hiding this comment.
This is required for putting object of any type in JSON. The JSONObject implements raw type of Map and every call to put is risky in view of javac. The only way to resolve this warning is to change the json dependency to org.json from com.googlecode.json-simple:json-simple since it is compiled with a very old javac (the latest release is for 2012). We have an open PR that is not landed yet as it has a low priority #123 that replaces the dependency with org.json and can get rid of all @SuppressWarnings("unchecked").
| public JSONObject visit(Location location) { | ||
| return location.accept(this, null); | ||
| } |
There was a problem hiding this comment.
This method is unnecessary, right? At any caller you could just call Location.accept directly with a new visitor? I think that is standard and we can remove this method
There was a problem hiding this comment.
Yes exactly, removed it 6f245da.
This PR updates the logic for json creation from
Locationinstances using the visitor pattern architecture added in #169