Make JSON methods pass thru to the implementation.

R=roubert@google.com

Review URL: https://codereview.appspot.com/115210043

git-svn-id: http://libaddressinput.googlecode.com/svn/trunk@317 38ededc0-08b8-5190-f2ac-b31f878777ad

4 files changed, 95 insertions, 108 deletions

diff --git a/cpp/src/preload_supplier.cc b/cpp/src/preload_supplier.cc
index 3a9cdc3..26f7e2b 100644
--- a/cpp/src/preload_supplier.cc
+++ b/cpp/src/preload_supplier.cc
@@ -114,13 +114,13 @@ class Helper {
 
     for (std::vector<std::string>::const_iterator
          it = json.GetKeys().begin(); it != json.GetKeys().end(); ++it) {
-      if (!json.HasDictionaryValueForKey(*it)) {
+      const Json* value;
+      if (!json.GetDictionaryValueForKey(*it, &value)) {
         success = false;
         goto callback;
       }
-      const Json& value = json.GetDictionaryValueForKey(*it);
 
-      if (!value.GetStringValueForKey("id", &id)) {
+      if (!value->GetStringValueForKey("id", &id)) {
         success = false;
         goto callback;
       }
@@ -135,7 +135,7 @@ class Helper {
         // All rules on the COUNTRY level inherit from the default rule.
         rule->CopyFrom(Rule::GetDefault());
       }
-      rule->ParseJsonRule(value);
+      rule->ParseJsonRule(*value);
       assert(id == rule->GetId());  // Sanity check.
 
       rule_storage_->push_back(rule);
diff --git a/cpp/src/util/json.cc b/cpp/src/util/json.cc
index 4f26069..14a9d11 100644
--- a/cpp/src/util/json.cc
+++ b/cpp/src/util/json.cc
@@ -36,75 +36,97 @@ using rapidjson::Value;
 
 class Json::JsonImpl {
  public:
-  // Takes ownership of |document|.
-  explicit JsonImpl(const Document* document)
-      : document_(document), value_(document), dictionaries_() {
-    assert(value_ != NULL);
-    assert(value_->IsObject());
-    BuildKeyList();
-  }
-
-  // Does not take ownership of |value|.
-  explicit JsonImpl(const Value* value)
-      : document_(), value_(value), dictionaries_() {
-    assert(value_ != NULL);
-    assert(value_->IsObject());
-    BuildKeyList();
+  explicit JsonImpl(const std::string& json)
+      : document_(new Document),
+        value_(document_.get()),
+        dictionaries_(),
+        keys_(),
+        valid_(false) {
+    document_->Parse<kParseValidateEncodingFlag>(json.c_str());
+    valid_ = !document_->HasParseError() && document_->IsObject();
+    if (valid_) {
+      BuildKeyList();
+    }
   }
 
   ~JsonImpl() {
     for (std::map<std::string, const Json*>::const_iterator
          it = dictionaries_.begin();
-         it != dictionaries_.end(); ++it) {
+         it != dictionaries_.end();
+         ++it) {
       delete it->second;
     }
   }
 
-  // The caller does not own the result.
-  const Value::Member* FindMember(const std::string& key) {
-    return value_->FindMember(key.c_str());
+  bool valid() const { return valid_; }
+
+  const std::vector<std::string>& GetKeys() const { return keys_; }
+
+  bool GetStringValueForKey(const std::string& key, std::string* value) const {
+    assert(value != NULL);
+
+    Value::ConstMemberIterator member = value_->FindMember(key.c_str());
+    if (member == NULL || !member->value.IsString()) {
+      return false;
+    }
+
+    value->assign(member->value.GetString(),
+                  member->value.GetStringLength());
+    return true;
   }
 
-  // The caller does not own the result. The result can be NULL if there's no
-  // dictionary for |key|.
-  const Json* FindDictionary(const std::string& key) const {
-    std::map<std::string, const Json*>::const_iterator it =
+  bool GetDictionaryValueForKey(const std::string& key, const Json** value) {
+    assert(value != NULL);
+
+    std::map<std::string, const Json*>::const_iterator dict_it =
         dictionaries_.find(key);
-    return it != dictionaries_.end() ? it->second : NULL;
-  }
+    if (dict_it != dictionaries_.end()) {
+      *value = dict_it->second;
+      return true;
+    }
+
+    Value::ConstMemberIterator member = value_->FindMember(key.c_str());
+    if (member == NULL || !member->value.IsObject()) {
+      return false;
+    }
 
-  // Takes ownership of |dictionary|. Should be called only once per |key| and
-  // per |dictionary|.
-  void AddDictionary(const std::string& key, const Json* dictionary) {
+    Json* sub_dictionary = new Json;
+    sub_dictionary->impl_.reset(new JsonImpl(&member->value));
     bool inserted =
-        dictionaries_.insert(std::make_pair(key, dictionary)).second;
+        dictionaries_.insert(std::make_pair(key, sub_dictionary)).second;
     // Cannot do work inside of assert(), because the compiler can optimize it
     // away.
     assert(inserted);
     // Avoid unused variable warning when assert() is optimized away.
     (void)inserted;
-  }
 
-  const std::vector<std::string>& GetKeys() const { return keys_; }
+    *value = sub_dictionary;
+    return true;
+  }
 
  private:
+  // Does not take ownership of |value|.
+  explicit JsonImpl(const Value* value)
+      : document_(),
+        value_(value),
+        dictionaries_(),
+        keys_(),
+        valid_(true) {
+    assert(value_ != NULL);
+    assert(value_->IsObject());
+    BuildKeyList();
+  }
+
   void BuildKeyList() {
     assert(keys_.empty());
-    for (Value::ConstMemberIterator it = value_->MemberBegin();
-         it != value_->MemberEnd(); ++it) {
-      keys_.push_back(it->name.GetString());
+    for (Value::ConstMemberIterator member = value_->MemberBegin();
+         member != value_->MemberEnd(); ++member) {
+      keys_.push_back(member->name.GetString());
     }
   }
 
   // An owned JSON document. Can be NULL if the JSON document is not owned.
-  //
-  // When a JsonImpl object is constructed using a Document object, then
-  // JsonImpl is supposed to take ownership of that object, making sure to
-  // delete it in its own destructor. But when a JsonImpl object is constructed
-  // using a Value object, then that object is owned by a Member object which is
-  // owned by a Document object, and should therefore not be deleted by
-  // JsonImpl.
-  const scoped_ptr<const Document> document_;
+  const scoped_ptr<Document> document_;
 
   // A JSON document that is not owned. Cannot be NULL. Can point to document_.
   const Value* const value_;
@@ -112,8 +134,12 @@ class Json::JsonImpl {
   // Owned JSON objects.
   std::map<std::string, const Json*> dictionaries_;
 
+  // The list of keys with values in the JSON object.
   std::vector<std::string> keys_;
 
+  // True if the JSON object was parsed successfully.
+  bool valid_;
+
   DISALLOW_COPY_AND_ASSIGN(JsonImpl);
 };
 
@@ -123,13 +149,11 @@ Json::~Json() {}
 
 bool Json::ParseObject(const std::string& json) {
   assert(impl_ == NULL);
-  scoped_ptr<Document> document(new Document);
-  document->Parse<kParseValidateEncodingFlag>(json.c_str());
-  bool valid = !document->HasParseError() && document->IsObject();
-  if (valid) {
-    impl_.reset(new JsonImpl(document.release()));
+  impl_.reset(new JsonImpl(json));
+  if (!impl_->valid()) {
+    impl_.reset();
   }
-  return valid;
+  return impl_ != NULL;
 }
 
 const std::vector<std::string>& Json::GetKeys() const {
@@ -140,50 +164,13 @@ const std::vector<std::string>& Json::GetKeys() const {
 bool Json::GetStringValueForKey(const std::string& key,
                                 std::string* value) const {
   assert(impl_ != NULL);
-  assert(value != NULL);
-
-  // Member is owned by impl_.
-  const Value::Member* member = impl_->FindMember(key.c_str());
-  if (member == NULL || !member->value.IsString()) {
-    return false;
-  }
-
-  value->assign(member->value.GetString(), member->value.GetStringLength());
-  return true;
-}
-
-bool Json::HasDictionaryValueForKey(const std::string& key) const {
-  assert(impl_ != NULL);
-
-  // The value returned by FindDictionary() is owned by impl_.
-  if (impl_->FindDictionary(key) != NULL) {
-    return true;
-  }
-
-  // Member is owned by impl_.
-  const Value::Member* member = impl_->FindMember(key);
-  return member != NULL && member->value.IsObject();
+  return impl_->GetStringValueForKey(key, value);
 }
 
-const Json& Json::GetDictionaryValueForKey(const std::string& key) const {
+bool Json::GetDictionaryValueForKey(const std::string& key,
+                                    const Json** value) const {
   assert(impl_ != NULL);
-
-  // Existing_dictionary is owned by impl_.
-  const Json* existing_dictionary = impl_->FindDictionary(key);
-  if (existing_dictionary != NULL) {
-    return *existing_dictionary;
-  }
-
-  // Member is owned by impl_.
-  const Value::Member* member = impl_->FindMember(key);
-  assert(member != NULL);
-  assert(member->value.IsObject());
-
-  // Dictionary is owned by impl_.
-  Json* dictionary = new Json;
-  dictionary->impl_.reset(new JsonImpl(&member->value));
-  impl_->AddDictionary(key, dictionary);
-  return *dictionary;
+  return impl_->GetDictionaryValueForKey(key, value);
 }
 
 }  // namespace addressinput
diff --git a/cpp/src/util/json.h b/cpp/src/util/json.h
index 90565e6..1d66aeb 100644
--- a/cpp/src/util/json.h
+++ b/cpp/src/util/json.h
@@ -49,15 +49,12 @@ class Json {
   // parameter should not be NULL.
   bool GetStringValueForKey(const std::string& key, std::string* value) const;
 
-  // Returns true if the parsed JSON contains a dictionary value for |key|. The
-  // JSON object must be parsed successfully in ParseObject() before invoking
-  // this method.
-  bool HasDictionaryValueForKey(const std::string& key) const;
-
-  // Returns the dictionary value for the |key|. The |key| must be present and
-  // its value must be of the dictionary type, i.e., HasDictionaryValueForKey()
-  // must return true before invoking this method.
-  const Json& GetDictionaryValueForKey(const std::string& key) const;
+  // Returns true if the parsed JSON contains a dictionary value for |key|.
+  // Points |value| to the dictionary value of the |key|. The JSON object must
+  // be parsed successfully in ParseObject() before invoking this method. The
+  // caller does not own |value|.
+  bool GetDictionaryValueForKey(const std::string& key,
+                                const Json** value) const;
 
  private:
   class JsonImpl;
diff --git a/cpp/test/util/json_test.cc b/cpp/test/util/json_test.cc
index ff23ea6..2306ac3 100644
--- a/cpp/test/util/json_test.cc
+++ b/cpp/test/util/json_test.cc
@@ -120,16 +120,18 @@ TEST(JsonTest, NumberIsNotValid) {
 TEST(JsonTest, NoDictionaryFound) {
   Json json;
   ASSERT_TRUE(json.ParseObject("{\"key\":\"value\"}"));
-  EXPECT_FALSE(json.HasDictionaryValueForKey("key"));
+  const Json* sub_json;
+  EXPECT_FALSE(json.GetDictionaryValueForKey("key", &sub_json));
 }
 
 TEST(JsonTest, DictionaryFound) {
   Json json;
   ASSERT_TRUE(json.ParseObject("{\"key\":{\"inner_key\":\"value\"}}"));
-  ASSERT_TRUE(json.HasDictionaryValueForKey("key"));
-  const Json& sub_json = json.GetDictionaryValueForKey("key");
+  const Json* sub_json = NULL;
+  ASSERT_TRUE(json.GetDictionaryValueForKey("key", &sub_json));
+  ASSERT_TRUE(sub_json != NULL);
   std::string value;
-  EXPECT_TRUE(sub_json.GetStringValueForKey("inner_key", &value));
+  EXPECT_TRUE(sub_json->GetStringValueForKey("inner_key", &value));
   EXPECT_EQ("value", value);
 }
 
@@ -139,10 +141,11 @@ TEST(JsonTest, DictionariesHaveKeys) {
   std::vector<std::string> expected_keys(1, "key");
   EXPECT_EQ(expected_keys, json.GetKeys());
 
-  ASSERT_TRUE(json.HasDictionaryValueForKey("key"));
-  const Json& sub_json = json.GetDictionaryValueForKey("key");
+  const Json* sub_json = NULL;
+  ASSERT_TRUE(json.GetDictionaryValueForKey("key", &sub_json));
+  ASSERT_TRUE(sub_json != NULL);
   std::vector<std::string> expected_sub_keys(1, "inner_key");
-  EXPECT_EQ(expected_sub_keys, sub_json.GetKeys());
+  EXPECT_EQ(expected_sub_keys, sub_json->GetKeys());
 }
 
 }  // namespace