-
Notifications
You must be signed in to change notification settings - Fork 148
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
Removing apache commons & Guava dependency #60
Conversation
This commit contains:
|
Maybe we could further optimize the lcs algorithm. I am gonna take a look at this: https://github.com/fillumina/LCS |
Sure @LouizFC |
Hi @LouizFC
|
class InternalUtils { | ||
|
||
static List<JsonNode> toList(ArrayNode input) { | ||
List<JsonNode> toReturn = new ArrayList<JsonNode>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably initialize with size hint (i.e parameter specifying capacity)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
|
||
private static List<JsonNode> reverse(List<JsonNode> list) { | ||
List<JsonNode> toReturn = new LinkedList<JsonNode>(); | ||
for (int i = list.size() - 1; i >= 0; i--) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would probably be better to forward-iterate the list (the interface makes no guarantees about time complexity on indexed retrieval/update) and build up an ArrayList (which does).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use Collections.reverse, which guarantees linear time complexity instead of this method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That requires a clone first, not sure it'll end up being any faster...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inside InternalUtils.longestCommonSubsequence, we could use Collections.reverse ( it does in-place reversal ) before the return statement.
By your last comment, did you mean that sir?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible, but may be slightly less readable. Bottom line, we don't have a benchmark suite yet, so the whole discussion is kind of moot :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am using @cyber-front test-suite ( shared sometime back with both of us ) for benchmarking where it creates 65,536 random objects and performed a series of 256 updates on each.
/* | ||
* Merging remove & add to move operation | ||
*/ | ||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why multi-line comments? Just makes everything more difficult...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, will make it Single line comment
Preconditions.checkArgument(second.isArray(), "LCS can only work on JSON arrays"); | ||
|
||
return ListUtils.longestCommonSubsequence(Lists.newArrayList(first), Lists.newArrayList(second)); | ||
return InternalUtils.longestCommonSubsequence(InternalUtils.toList((ArrayNode) first), InternalUtils.toList((ArrayNode) second)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slight change in error semantics, parameters no longer checked for whether or not they are arrays. Intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the caller already checks whether both inputs( source & target) are of Array type before calling this method, i thought to remove the additional checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, but I checked and couldn't see the aforementioned check at the caller. Anyway it's only for clarity of errors when the API is misused, so not a critical point regardless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, caller of compareArray() methods checks the data-type of source & target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's all good now. We removed the dependencies and gained a little perfomance boost. My only issue was running the tests: I runned all tests, but some were failing in the beggining. I could not find in where in the .json file the test data was. I think adding a little more information when the tests fail (like file name and "message" field) may be beneficial. I can submit a PR for this later if wanted, since junit offers a message in the "assertEquals" method
…e Constants final
@@ -147,11 +147,11 @@ public void replace(List<String> path, JsonNode value) { | |||
if (isNullOrEmpty(fieldToReplace) && path.size() == 1) | |||
target = value; | |||
else if (parentNode.isObject()) | |||
((ObjectNode) parentNode).put(fieldToReplace, value); | |||
((ObjectNode) parentNode).set(fieldToReplace, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the change? Is this semantically equivalent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old method is long deprecated in jackson api. But yes, the new one is equivalent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that you commented, this change shouldn't be in this commit, it passed without me noticing, sorry. I will let you decide if this should pass or not
This change got in to this commit without I noticing it. I will roll it back for now and wait for feedback.
@LouizFC |
This reverts commit 7973f70.
* removing apache commons dependency * Moved getArrayNodeRepresentation to PathUtils. Removed EncodePathFunction * Removed guava. * making 0.3.10-SNAPSHOT -> 0.3.11-SNAPSHOT & making few methods private * relacing reverse() with colelctions.reverse() and other review comments * commenting out print statements in tests * Renamed getArrayNodeRepresentation to getPathRepresentation. Also made Constants final * Rolled back a api change. This change got in to this commit without I noticing it. I will roll it back for now and wait for feedback.
You are welcome @vishwakarma. Thank you too for letting me take part in this, it was a pleasure. I look forward for helping more in the future, if you want, of course. |
@LouizFC , Your are open to propose any change in this project or to even start a new project. I would be available. |
@vishwakarma I have added my email in to my github profile. Thank you for your interest. |
This commit contains --