Replace Get* methods returning unique_ptr with reference alternative
Existing code created temporarily objects and returned them to the
client. It was not efficient and error-prone as client code could
retrieve pointers to internal objects without keeping unique_ptr alive.
Change-Id: I9e17c8d9f66dfc9f52ce9ffc9a31992b16b00461
Reviewed-on: https://weave-review.googlesource.com/1672
Reviewed-by: Alex Vakulenko <avakulenko@google.com>
diff --git a/src/base_api_handler.cc b/src/base_api_handler.cc
index 3d22a10..1423dd1 100644
--- a/src/base_api_handler.cc
+++ b/src/base_api_handler.cc
@@ -99,10 +99,10 @@
bool discovery_enabled{settings.local_discovery_enabled};
bool pairing_enabled{settings.local_pairing_enabled};
- auto parameters = command->GetParameters();
- parameters->GetString("localAnonymousAccessMaxRole", &anonymous_access_role);
- parameters->GetBoolean("localDiscoveryEnabled", &discovery_enabled);
- parameters->GetBoolean("localPairingEnabled", &pairing_enabled);
+ const auto& parameters = command->GetParameters();
+ parameters.GetString("localAnonymousAccessMaxRole", &anonymous_access_role);
+ parameters.GetBoolean("localDiscoveryEnabled", &discovery_enabled);
+ parameters.GetBoolean("localPairingEnabled", &pairing_enabled);
AuthScope auth_scope{AuthScope::kNone};
if (!StringToEnum(anonymous_access_role, &auth_scope)) {
@@ -144,10 +144,10 @@
std::string description{settings.description};
std::string location{settings.location};
- auto parameters = command->GetParameters();
- parameters->GetString("name", &name);
- parameters->GetString("description", &description);
- parameters->GetString("location", &location);
+ const auto& parameters = command->GetParameters();
+ parameters.GetString("name", &name);
+ parameters.GetString("description", &description);
+ parameters.GetString("location", &location);
device_info_->UpdateDeviceInfo(name, description, location);
command->Complete({}, nullptr);
diff --git a/src/base_api_handler_unittest.cc b/src/base_api_handler_unittest.cc
index 357385d..15a575a 100644
--- a/src/base_api_handler_unittest.cc
+++ b/src/base_api_handler_unittest.cc
@@ -84,7 +84,8 @@
}
std::unique_ptr<base::DictionaryValue> GetBaseState() {
- auto state = state_manager_->GetState();
+ std::unique_ptr<base::DictionaryValue> state{
+ state_manager_->GetState().DeepCopy()};
std::set<std::string> result;
for (base::DictionaryValue::Iterator it{*state}; !it.IsAtEnd();
it.Advance()) {
diff --git a/src/commands/cloud_command_proxy.cc b/src/commands/cloud_command_proxy.cc
index 91db18c..3d472c7 100644
--- a/src/commands/cloud_command_proxy.cc
+++ b/src/commands/cloud_command_proxy.cc
@@ -43,7 +43,7 @@
void CloudCommandProxy::OnResultsChanged() {
std::unique_ptr<base::DictionaryValue> patch{new base::DictionaryValue};
patch->Set(commands::attributes::kCommand_Results,
- command_instance_->GetResults().release());
+ command_instance_->GetResults().CreateDeepCopy());
QueueCommandUpdate(std::move(patch));
}
@@ -57,7 +57,7 @@
void CloudCommandProxy::OnProgressChanged() {
std::unique_ptr<base::DictionaryValue> patch{new base::DictionaryValue};
patch->Set(commands::attributes::kCommand_Progress,
- command_instance_->GetProgress().release());
+ command_instance_->GetProgress().CreateDeepCopy());
QueueCommandUpdate(std::move(patch));
}
diff --git a/src/commands/command_instance.cc b/src/commands/command_instance.cc
index aa71b0e..8328299 100644
--- a/src/commands/command_instance.cc
+++ b/src/commands/command_instance.cc
@@ -94,16 +94,16 @@
return origin_;
}
-std::unique_ptr<base::DictionaryValue> CommandInstance::GetParameters() const {
- return std::unique_ptr<base::DictionaryValue>(parameters_.DeepCopy());
+const base::DictionaryValue& CommandInstance::GetParameters() const {
+ return parameters_;
}
-std::unique_ptr<base::DictionaryValue> CommandInstance::GetProgress() const {
- return std::unique_ptr<base::DictionaryValue>(progress_.DeepCopy());
+const base::DictionaryValue& CommandInstance::GetProgress() const {
+ return progress_;
}
-std::unique_ptr<base::DictionaryValue> CommandInstance::GetResults() const {
- return std::unique_ptr<base::DictionaryValue>(results_.DeepCopy());
+const base::DictionaryValue& CommandInstance::GetResults() const {
+ return results_;
}
const Error* CommandInstance::GetError() const {
diff --git a/src/commands/command_instance.h b/src/commands/command_instance.h
index e7a6eca..60620a1 100644
--- a/src/commands/command_instance.h
+++ b/src/commands/command_instance.h
@@ -54,9 +54,9 @@
const std::string& GetName() const override;
Command::State GetState() const override;
Command::Origin GetOrigin() const override;
- std::unique_ptr<base::DictionaryValue> GetParameters() const override;
- std::unique_ptr<base::DictionaryValue> GetProgress() const override;
- std::unique_ptr<base::DictionaryValue> GetResults() const override;
+ const base::DictionaryValue& GetParameters() const override;
+ const base::DictionaryValue& GetProgress() const override;
+ const base::DictionaryValue& GetResults() const override;
const Error* GetError() const override;
bool SetProgress(const base::DictionaryValue& progress,
ErrorPtr* error) override;
diff --git a/src/commands/command_instance_unittest.cc b/src/commands/command_instance_unittest.cc
index 1d32cd9..fb8fe84 100644
--- a/src/commands/command_instance_unittest.cc
+++ b/src/commands/command_instance_unittest.cc
@@ -82,8 +82,8 @@
EXPECT_EQ("robot.speak", instance.GetName());
EXPECT_EQ(Command::Origin::kCloud, instance.GetOrigin());
EXPECT_JSON_EQ("{'phrase': 'iPityDaFool', 'volume': 5}",
- *instance.GetParameters());
- EXPECT_JSON_EQ("{'foo': 239}", *instance.GetResults());
+ instance.GetParameters());
+ EXPECT_JSON_EQ("{'foo': 239}", instance.GetResults());
CommandInstance instance2{"base.reboot",
Command::Origin::kLocal,
@@ -118,7 +118,7 @@
EXPECT_EQ("abcd", instance->GetID());
EXPECT_EQ("robot.jump", instance->GetName());
EXPECT_JSON_EQ("{'height': 53, '_jumpType': '_withKick'}",
- *instance->GetParameters());
+ instance->GetParameters());
}
TEST_F(CommandInstanceTest, FromJson_ParamsOmitted) {
@@ -126,7 +126,7 @@
auto instance = CommandInstance::FromJson(json.get(), Command::Origin::kCloud,
dict_, nullptr, nullptr);
EXPECT_EQ("base.reboot", instance->GetName());
- EXPECT_JSON_EQ("{}", *instance->GetParameters());
+ EXPECT_JSON_EQ("{}", instance->GetParameters());
}
TEST_F(CommandInstanceTest, FromJson_NotObject) {
diff --git a/src/device_manager.cc b/src/device_manager.cc
index c0b8e9a..52b2882 100644
--- a/src/device_manager.cc
+++ b/src/device_manager.cc
@@ -134,7 +134,7 @@
return state_manager_->SetProperties(dict, error);
}
-std::unique_ptr<base::Value> DeviceManager::GetStateProperty(
+const base::Value* DeviceManager::GetStateProperty(
const std::string& name) const {
return state_manager_->GetProperty(name);
}
@@ -145,7 +145,7 @@
return state_manager_->SetProperty(name, value, error);
}
-std::unique_ptr<base::DictionaryValue> DeviceManager::GetState() const {
+const base::DictionaryValue& DeviceManager::GetState() const {
return state_manager_->GetState();
}
diff --git a/src/device_manager.h b/src/device_manager.h
index ccf8778..6c3df05 100644
--- a/src/device_manager.h
+++ b/src/device_manager.h
@@ -52,12 +52,11 @@
ErrorPtr* error) override;
bool SetStateProperties(const base::DictionaryValue& dict,
ErrorPtr* error) override;
- std::unique_ptr<base::Value> GetStateProperty(
- const std::string& name) const override;
+ const base::Value* GetStateProperty(const std::string& name) const override;
bool SetStateProperty(const std::string& name,
const base::Value& value,
ErrorPtr* error) override;
- std::unique_ptr<base::DictionaryValue> GetState() const override;
+ const base::DictionaryValue& GetState() const override;
void Register(const std::string& ticket_id,
const DoneCallback& callback) override;
GcdState GetGcdState() const override;
diff --git a/src/device_registration_info.cc b/src/device_registration_info.cc
index 5c8625a..0c914b7 100644
--- a/src/device_registration_info.cc
+++ b/src/device_registration_info.cc
@@ -485,8 +485,7 @@
if (!commands)
return nullptr;
- std::unique_ptr<base::DictionaryValue> state = state_manager_->GetState();
- CHECK(state);
+ const base::DictionaryValue& state = state_manager_->GetState();
std::unique_ptr<base::DictionaryValue> resource{new base::DictionaryValue};
if (!GetSettings().cloud_id.empty())
@@ -507,7 +506,7 @@
}
resource->Set("channel", channel.release());
resource->Set("commandDefs", commands.release());
- resource->Set("state", state.release());
+ resource->Set("state", state.DeepCopy());
return resource;
}
diff --git a/src/privet/cloud_delegate.cc b/src/privet/cloud_delegate.cc
index efb69bd..c771366 100644
--- a/src/privet/cloud_delegate.cc
+++ b/src/privet/cloud_delegate.cc
@@ -243,9 +243,8 @@
void OnStateChanged() {
state_.Clear();
- auto state = state_manager_->GetState();
- CHECK(state);
- state_.MergeDictionary(state.get());
+ const auto& state = state_manager_->GetState();
+ state_.MergeDictionary(&state);
NotifyOnStateChanged();
}
diff --git a/src/states/state_manager.cc b/src/states/state_manager.cc
index b1e70a3..b170b86 100644
--- a/src/states/state_manager.cc
+++ b/src/states/state_manager.cc
@@ -27,14 +27,17 @@
callback.Run(); // Force to read current state.
}
-std::unique_ptr<base::DictionaryValue> StateManager::GetState() const {
- std::unique_ptr<base::DictionaryValue> dict{new base::DictionaryValue};
- for (const auto& pair : packages_) {
- auto pkg_value = pair.second->GetValuesAsJson();
- CHECK(pkg_value);
- dict->SetWithoutPathExpansion(pair.first, pkg_value.release());
+const base::DictionaryValue& StateManager::GetState() const {
+ if (!cached_dict_valid_) {
+ cached_dict_.Clear();
+ for (const auto& pair : packages_) {
+ cached_dict_.SetWithoutPathExpansion(
+ pair.first, pair.second->GetValuesAsJson().DeepCopy());
+ }
+ cached_dict_valid_ = true;
}
- return dict;
+
+ return cached_dict_;
}
bool StateManager::SetProperty(const std::string& name,
@@ -46,8 +49,7 @@
return result;
}
-std::unique_ptr<base::Value> StateManager::GetProperty(
- const std::string& name) const {
+const base::Value* StateManager::GetProperty(const std::string& name) const {
auto parts = SplitAtFirst(name, ".", true);
const std::string& package_name = parts.first;
const std::string& property_name = parts.second;
@@ -93,6 +95,8 @@
if (!package->SetPropertyValue(property_name, value, error))
return false;
+ cached_dict_valid_ = false;
+
std::unique_ptr<base::DictionaryValue> prop_set{new base::DictionaryValue};
prop_set->Set(full_property_name, value.DeepCopy());
state_change_queue_->NotifyPropertiesUpdated(timestamp, std::move(prop_set));
diff --git a/src/states/state_manager.h b/src/states/state_manager.h
index 55faf76..c48a57c 100644
--- a/src/states/state_manager.h
+++ b/src/states/state_manager.h
@@ -39,11 +39,11 @@
bool LoadStateDefinitionFromJson(const std::string& json, ErrorPtr* error);
bool SetProperties(const base::DictionaryValue& dict, ErrorPtr* error);
bool SetPropertiesFromJson(const std::string& json, ErrorPtr* error);
- std::unique_ptr<base::Value> GetProperty(const std::string& name) const;
+ const base::Value* GetProperty(const std::string& name) const;
bool SetProperty(const std::string& name,
const base::Value& value,
ErrorPtr* error);
- std::unique_ptr<base::DictionaryValue> GetState() const;
+ const base::DictionaryValue& GetState() const;
// Returns the recorded state changes since last time this method has been
// called.
@@ -80,6 +80,9 @@
std::vector<base::Closure> on_changed_;
+ mutable base::DictionaryValue cached_dict_;
+ mutable bool cached_dict_valid_ = false;
+
DISALLOW_COPY_AND_ASSIGN(StateManager);
};
diff --git a/src/states/state_manager_unittest.cc b/src/states/state_manager_unittest.cc
index f655e84..6899646 100644
--- a/src/states/state_manager_unittest.cc
+++ b/src/states/state_manager_unittest.cc
@@ -109,7 +109,7 @@
},
'device': {}
})";
- EXPECT_JSON_EQ(expected, *mgr_->GetState());
+ EXPECT_JSON_EQ(expected, mgr_->GetState());
}
TEST_F(StateManagerTest, LoadStateDefinition) {
@@ -128,7 +128,7 @@
'power': {},
'device': {}
})";
- EXPECT_JSON_EQ(expected, *mgr_->GetState());
+ EXPECT_JSON_EQ(expected, mgr_->GetState());
}
TEST_F(StateManagerTest, Startup) {
@@ -170,7 +170,7 @@
'battery_level': 44
}
})";
- EXPECT_JSON_EQ(expected, *manager.GetState());
+ EXPECT_JSON_EQ(expected, manager.GetState());
}
TEST_F(StateManagerTest, SetPropertyValue) {
@@ -189,7 +189,7 @@
'state_property': 'Test Value'
}
})";
- EXPECT_JSON_EQ(expected, *mgr_->GetState());
+ EXPECT_JSON_EQ(expected, mgr_->GetState());
}
TEST_F(StateManagerTest, SetPropertyValue_Error_NoName) {
@@ -227,7 +227,7 @@
ASSERT_TRUE(SetPropertyValue("device.state_property",
base::StringValue{"Test Value"}, nullptr));
std::vector<StateChange> expected_state;
- const std::string expected_val =
+ const std::string expected_val =
"{'device': {'state_property': 'Test Value'}}";
expected_state.emplace_back(
timestamp_,
@@ -259,7 +259,7 @@
},
'device': {}
})";
- EXPECT_JSON_EQ(expected, *mgr_->GetState());
+ EXPECT_JSON_EQ(expected, mgr_->GetState());
}
TEST_F(StateManagerTest, GetProperty) {
diff --git a/src/states/state_package.cc b/src/states/state_package.cc
index 55650b6..b0ea199 100644
--- a/src/states/state_package.cc
+++ b/src/states/state_package.cc
@@ -39,11 +39,11 @@
return true;
}
-std::unique_ptr<base::DictionaryValue> StatePackage::GetValuesAsJson() const {
- return std::unique_ptr<base::DictionaryValue>(values_.DeepCopy());
+const base::DictionaryValue& StatePackage::GetValuesAsJson() const {
+ return values_;
}
-std::unique_ptr<base::Value> StatePackage::GetPropertyValue(
+const base::Value* StatePackage::GetPropertyValue(
const std::string& property_name,
ErrorPtr* error) const {
const base::Value* value = nullptr;
@@ -55,7 +55,7 @@
return nullptr;
}
- return std::unique_ptr<base::Value>(value->DeepCopy());
+ return value;
}
bool StatePackage::SetPropertyValue(const std::string& property_name,
diff --git a/src/states/state_package.h b/src/states/state_package.h
index 7632145..4092948 100644
--- a/src/states/state_package.h
+++ b/src/states/state_package.h
@@ -43,13 +43,12 @@
// "message": "Printer low on cyan ink"
// }
// }
- std::unique_ptr<base::DictionaryValue> GetValuesAsJson() const;
+ const base::DictionaryValue& GetValuesAsJson() const;
// Gets the value for a specific state property. |property_name| must not
// include the package name as part of the property name.
- std::unique_ptr<base::Value> GetPropertyValue(
- const std::string& property_name,
- ErrorPtr* error) const;
+ const base::Value* GetPropertyValue(const std::string& property_name,
+ ErrorPtr* error) const;
// Sets the value for a specific state property. |property_name| must not
// include the package name as part of the property name.
bool SetPropertyValue(const std::string& property_name,
diff --git a/src/states/state_package_unittest.cc b/src/states/state_package_unittest.cc
index c116b37..d09625a 100644
--- a/src/states/state_package_unittest.cc
+++ b/src/states/state_package_unittest.cc
@@ -131,7 +131,7 @@
})";
EXPECT_JSON_EQ(expected, GetTypes(package));
- EXPECT_JSON_EQ("{}", *package.GetValuesAsJson());
+ EXPECT_JSON_EQ("{}", package.GetValuesAsJson());
}
TEST(StatePackage, AddValuesFromJson_OnEmpty) {
@@ -148,7 +148,7 @@
'iso': 200,
'light': true
})";
- EXPECT_JSON_EQ(expected, *package.GetValuesAsJson());
+ EXPECT_JSON_EQ(expected, package.GetValuesAsJson());
}
TEST_F(StatePackageTest, AddSchemaFromJson_AddMore) {
@@ -200,7 +200,7 @@
'iso': 200,
'light': true
})";
- EXPECT_JSON_EQ(expected, *package_->GetValuesAsJson());
+ EXPECT_JSON_EQ(expected, package_->GetValuesAsJson());
}
TEST_F(StatePackageTest, AddValuesFromJson_AddMore) {
@@ -222,7 +222,7 @@
'iso': 200,
'light': true
})";
- EXPECT_JSON_EQ(expected, *package_->GetValuesAsJson());
+ EXPECT_JSON_EQ(expected, package_->GetValuesAsJson());
}
TEST_F(StatePackageTest, AddSchemaFromJson_Error_Redefined) {
@@ -283,7 +283,7 @@
'iso': 200,
'light': true
})";
- EXPECT_JSON_EQ(expected, *package_->GetValuesAsJson());
+ EXPECT_JSON_EQ(expected, package_->GetValuesAsJson());
}
} // namespace weave
diff --git a/src/test/mock_command.cc b/src/test/mock_command.cc
deleted file mode 100644
index 65c2bab..0000000
--- a/src/test/mock_command.cc
+++ /dev/null
@@ -1,29 +0,0 @@
-// Copyright 2015 The Weave Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include <weave/test/mock_command.h>
-
-#include <memory>
-#include <string>
-
-#include <base/values.h>
-#include <weave/test/unittest_utils.h>
-
-namespace weave {
-namespace test {
-
-std::unique_ptr<base::DictionaryValue> MockCommand::GetParameters() const {
- return CreateDictionaryValue(MockGetParameters());
-}
-
-std::unique_ptr<base::DictionaryValue> MockCommand::GetProgress() const {
- return CreateDictionaryValue(MockGetProgress());
-}
-
-std::unique_ptr<base::DictionaryValue> MockCommand::GetResults() const {
- return CreateDictionaryValue(MockGetResults());
-}
-
-} // namespace test
-} // namespace weave
