Make JSON methods pass thru to the implementation.

Make JSON methods pass thru to the implementation.

R=roubert@google.com

Committed: https://code.google.com/p/libaddressinput/source/detail?r=317

[Rouslan Solomakhin, 2014-07-23 17:56:03]

Fredrik: PTAL. This patch helps to clarify what JSON implementation is doing. I
found this helpful when improving performance here in other patches.

[roubert (google), 2014-07-23 20:19:21]

https://codereview.appspot.com/115210043/diff/20001/cpp/src/util/json.cc
File cpp/src/util/json.cc (right):

https://codereview.appspot.com/115210043/diff/20001/cpp/src/util/json.cc#newc...
cpp/src/util/json.cc:79: return GetSubDictionary(key) != NULL;
By implementing HasDictionaryValueForKey() in terms of GetSubDictionary(), you
just made it significantly more expensive than it was before.

This does actually matter in preload_supplier.cc where OnRetrieved() does an
early exit if HasDictionaryValueForKey() returns false.

You might decide that the ability to do this early exit is such a small
optimization that it isn't worth the added code complexity, but if you decide
that, then I think you ought to merge HasDictionaryValueForKey() with
GetDictionaryValueForKey() into one single function to not give the false
impression that such an optimization is possible.

https://codereview.appspot.com/115210043/diff/20001/cpp/src/util/json.cc#newc...
cpp/src/util/json.cc:139: mutable std::map<std::string, const Json*>
dictionaries_;
Is this now mutable to be able to declare GetSubDictionary() const? If so, then
I think that's a slightly strange design decision and would instead prefer that
these methods on JsonImpl were not declared const, to clearly show that they do
manipulate the state of this object. The Json object holds a pointer to a
non-const JsonImpl object, so there is no issue with having const Json methods
calling non-const JsonImpl methods (like how AddDictionary() was called before).

[Rouslan Solomakhin, 2014-07-23 23:47:54]

Fredrik: PTAL Patch Set 3.

https://codereview.appspot.com/115210043/diff/20001/cpp/src/util/json.cc
File cpp/src/util/json.cc (right):

https://codereview.appspot.com/115210043/diff/20001/cpp/src/util/json.cc#newc...
cpp/src/util/json.cc:79: return GetSubDictionary(key) != NULL;
On 2014/07/23 20:19:20, roubert wrote:
> By implementing HasDictionaryValueForKey() in terms of GetSubDictionary(), you
> just made it significantly more expensive than it was before.
> 
> This does actually matter in preload_supplier.cc where OnRetrieved() does an
> early exit if HasDictionaryValueForKey() returns false.
> 
> You might decide that the ability to do this early exit is such a small
> optimization that it isn't worth the added code complexity, but if you decide
> that, then I think you ought to merge HasDictionaryValueForKey() with
> GetDictionaryValueForKey() into one single function to not give the false
> impression that such an optimization is possible.

Merged GetDictionary and HasDictionary into one.

https://codereview.appspot.com/115210043/diff/20001/cpp/src/util/json.cc#newc...
cpp/src/util/json.cc:139: mutable std::map<std::string, const Json*>
dictionaries_;
On 2014/07/23 20:19:21, roubert wrote:
> Is this now mutable to be able to declare GetSubDictionary() const? If so,
then
> I think that's a slightly strange design decision and would instead prefer
that
> these methods on JsonImpl were not declared const, to clearly show that they
do
> manipulate the state of this object. The Json object holds a pointer to a
> non-const JsonImpl object, so there is no issue with having const Json methods
> calling non-const JsonImpl methods (like how AddDictionary() was called
before).

Removed the mutable keyword.

[roubert (google), 2014-07-24 00:31:52]

LGTM

https://codereview.appspot.com/115210043/diff/60001/cpp/src/preload_supplier.cc
File cpp/src/preload_supplier.cc (right):

https://codereview.appspot.com/115210043/diff/60001/cpp/src/preload_supplier....
cpp/src/preload_supplier.cc:117: const Json* value = NULL;
Initializing this to NULL is only really useful if you have an assert(value !=
NULL) after the call to GetDictionaryValueForKey(). If you think that this code
should trust GetDictionaryValueForKey() to always set a non-NULL value before
returning true, then this initialization is unnecessary as well.

https://codereview.appspot.com/115210043/diff/60001/cpp/src/util/json.cc
File cpp/src/util/json.cc (right):

https://codereview.appspot.com/115210043/diff/60001/cpp/src/util/json.cc#newc...
cpp/src/util/json.cc:90: *value = NULL;
It's more common for this kind of interface to just not touch the output
parameter at all upon failure.

https://codereview.appspot.com/115210043/diff/60001/cpp/test/util/json_test.cc
File cpp/test/util/json_test.cc (right):

https://codereview.appspot.com/115210043/diff/60001/cpp/test/util/json_test.c...
cpp/test/util/json_test.cc:123: const Json* sub_json = NULL;
The initialization here doesn't serve any purpose, does it?

[Rouslan Solomakhin, 2014-07-24 00:49:20]

Thank you for the review. Committing.

https://codereview.appspot.com/115210043/diff/60001/cpp/src/preload_supplier.cc
File cpp/src/preload_supplier.cc (right):

https://codereview.appspot.com/115210043/diff/60001/cpp/src/preload_supplier....
cpp/src/preload_supplier.cc:117: const Json* value = NULL;
On 2014/07/24 00:31:51, roubert wrote:
> Initializing this to NULL is only really useful if you have an assert(value !=
> NULL) after the call to GetDictionaryValueForKey(). If you think that this
code
> should trust GetDictionaryValueForKey() to always set a non-NULL value before
> returning true, then this initialization is unnecessary as well.

SGTM. Removed " = NULL".

https://codereview.appspot.com/115210043/diff/60001/cpp/src/util/json.cc
File cpp/src/util/json.cc (right):

https://codereview.appspot.com/115210043/diff/60001/cpp/src/util/json.cc#newc...
cpp/src/util/json.cc:90: *value = NULL;
On 2014/07/24 00:31:51, roubert wrote:
> It's more common for this kind of interface to just not touch the output
> parameter at all upon failure.

Done.

https://codereview.appspot.com/115210043/diff/60001/cpp/test/util/json_test.cc
File cpp/test/util/json_test.cc (right):

https://codereview.appspot.com/115210043/diff/60001/cpp/test/util/json_test.c...
cpp/test/util/json_test.cc:123: const Json* sub_json = NULL;
On 2014/07/24 00:31:51, roubert wrote:
> The initialization here doesn't serve any purpose, does it?

Removed.

[Rouslan Solomakhin, 2014-07-24 00:49:41]

Committed patchset #4 manually as r317 (presubmit successful).