[3007] A few more review changes.

src/bin/d2/ncr_msg.cc

@@ -58,11 +58,12 @@ NameChangeRequest::NameChangeRequest()
     dhcid_(), lease_expires_on_(), lease_length_(0), status_(ST_NEW) {
 }
 
-NameChangeRequest::NameChangeRequest(NameChangeType change_type,
-            bool forward_change, bool reverse_change,
-            const std::string& fqdn, const std::string & ip_address,
-            const D2Dhcid& dhcid, const ptime& lease_expires_on,
-            uint32_t lease_length)
+NameChangeRequest::NameChangeRequest(const NameChangeType change_type,
+            const bool forward_change, const bool reverse_change,
+            const std::string& fqdn, const std::string& ip_address,
+            const D2Dhcid& dhcid,
+            const boost::posix_time::ptime& lease_expires_on,
+            const uint32_t lease_length)
     : change_type_(change_type), forward_change_(forward_change),
     reverse_change_(reverse_change), fqdn_(fqdn), ip_address_(ip_address),
     dhcid_(dhcid), lease_expires_on_(new ptime(lease_expires_on)),
@@ -74,7 +75,7 @@ NameChangeRequest::NameChangeRequest(NameChangeType change_type,
 }
 
 NameChangeRequestPtr
-NameChangeRequest::fromFormat(NameChangeFormat format,
+NameChangeRequest::fromFormat(const NameChangeFormat format,
                               isc::util::InputBuffer& buffer) {
     // Based on the format requested, pull the marshalled request from
     // InputBuffer and pass it into the appropriate format-specific factory.
@@ -114,12 +115,11 @@ NameChangeRequest::fromFormat(NameChangeFormat format,
 }
 
 void
-NameChangeRequest::toFormat(NameChangeFormat format,
+NameChangeRequest::toFormat(const NameChangeFormat format,
                             isc::util::OutputBuffer& buffer) const {
     // Based on the format requested, invoke the appropriate format handler
     // which will marshal this request's contents into the OutputBuffer.
-    switch (format)
-    {
+    switch (format) {
     case FMT_JSON: {
         // Invoke toJSON to create a JSON text of this request's contents.
         std::string json = toJSON();
@@ -223,7 +223,7 @@ NameChangeRequest::toJSON() const  {
 
 void
 NameChangeRequest::validateContent() {
-    //@TODO This is an initial implementation which provides a minimal amount
+    //@todo This is an initial implementation which provides a minimal amount
     // of validation.  FQDN, DHCID, and IP Address members are all currently
     // strings, these may be replaced with richer classes.
     if (fqdn_ == "") {
@@ -271,7 +271,7 @@ NameChangeRequest::getElement(const std::string& name,
 }
 
 void
-NameChangeRequest::setChangeType(NameChangeType value) {
+NameChangeRequest::setChangeType(const NameChangeType value) {
     change_type_ = value;
 }
 
@@ -299,7 +299,7 @@ NameChangeRequest::setChangeType(isc::data::ConstElementPtr element) {
 }
 
 void
-NameChangeRequest::setForwardChange(bool value) {
+NameChangeRequest::setForwardChange(const bool value) {
     forward_change_ = value;
 }
 
@@ -320,7 +320,7 @@ NameChangeRequest::setForwardChange(isc::data::ConstElementPtr element) {
 }
 
 void
-NameChangeRequest::setReverseChange(bool value) {
+NameChangeRequest::setReverseChange(const bool value) {
     reverse_change_ = value;
 }
 
@@ -385,7 +385,8 @@ NameChangeRequest::getLeaseExpiresOnStr() const {
     return (to_iso_string(*lease_expires_on_));
 }
 
-void NameChangeRequest::setLeaseExpiresOn(const std::string&  value) {
+void 
+NameChangeRequest::setLeaseExpiresOn(const std::string&  value) {
     try {
         // Create a new ptime instance from the ISO date-time string in value
         // add assign it to lease_expires_on_.
@@ -399,7 +400,8 @@ void NameChangeRequest::setLeaseExpiresOn(const std::string&  value) {
 
 }
 
-void NameChangeRequest::setLeaseExpiresOn(const ptime&  value) {
+void 
+NameChangeRequest::setLeaseExpiresOn(const boost::posix_time::ptime&  value) {
     if (lease_expires_on_->is_not_a_date_time()) {
         isc_throw(NcrMessageError, "Invalid value for lease_expires_on");
     }
@@ -414,7 +416,7 @@ void NameChangeRequest::setLeaseExpiresOn(isc::data::ConstElementPtr element) {
 }
 
 void
-NameChangeRequest::setLeaseLength(uint32_t value) {
+NameChangeRequest::setLeaseLength(const uint32_t value) {
     lease_length_ = value;
 }
 
@@ -445,7 +447,7 @@ NameChangeRequest::setLeaseLength(isc::data::ConstElementPtr element) {
 }
 
 void
-NameChangeRequest::setStatus(NameChangeStatus value) {
+NameChangeRequest::setStatus(const NameChangeStatus value) {
     status_ = value;
 }
 
@@ -455,15 +457,15 @@ NameChangeRequest::toText() const {
 
     stream << "Type: " << static_cast<int>(change_type_) << " (";
     switch (change_type_) {
-        case CHG_ADD:
-            stream << "CHG_ADD)\n";
-            break;
-        case CHG_REMOVE:
-            stream << "CHG_REMOVE)\n";
-            break;
-        default:
-            // Shouldn't be possible.
-            stream << "Invalid Value\n";
+    case CHG_ADD:
+        stream << "CHG_ADD)\n";
+        break;
+    case CHG_REMOVE:
+        stream << "CHG_REMOVE)\n";
+        break;
+    default:
+        // Shouldn't be possible.
+        stream << "Invalid Value\n";
     }
 
     stream << "Forward Change: " << (forward_change_ ? "yes" : "no")

src/bin/d2/ncr_msg.h

@@ -101,7 +101,7 @@ public:
     }
 
 private:
-    /// @Brief Storage for the DHCID value in unsigned bytes.
+    /// @brief Storage for the DHCID value in unsigned bytes.
     std::vector<uint8_t> bytes_;
 };
 
@@ -146,14 +146,16 @@ public:
     /// updated.
     /// @param ip_address the ip address leased to the given FQDN.
     /// @param dhcid the lease client's unique DHCID.
-    /// @param ptime a timestamp containing the date/time the lease expires.
+    /// @param lease_expires_on a timestamp containing the date/time the lease 
+    /// expires.
     /// @param lease_length the amount of time in seconds for which the
     /// lease is valid (TTL).
-    NameChangeRequest(NameChangeType change_type, bool forward_change,
-                      bool reverse_change, const std::string& fqdn,
-                      const std::string& ip_address, const D2Dhcid& dhcid,
+    NameChangeRequest(const NameChangeType change_type,
+                      const bool forward_change, const bool reverse_change,
+                      const std::string& fqdn, const std::string& ip_address,
+                      const D2Dhcid& dhcid,
                       const boost::posix_time::ptime& lease_expires_on,
-                      uint32_t lease_length);
+                      const uint32_t lease_length);
 
     /// @brief Static method for creating a NameChangeRequest from a
     /// buffer containing a marshalled request in a given format.
@@ -176,7 +178,7 @@ public:
     ///
     /// @throw throws NcrMessageError if an error occurs creating new
     /// request.
-    static NameChangeRequestPtr fromFormat(NameChangeFormat format,
+    static NameChangeRequestPtr fromFormat(const NameChangeFormat format,
                                            isc::util::InputBuffer& buffer);
 
     /// @brief Instance method for marshalling the contents of the request
@@ -195,8 +197,8 @@ public:
     /// @param format indicates the data format to use
     /// @param buffer is the output buffer to which the request should be
     /// marshalled.
-    void
-    toFormat(NameChangeFormat format, isc::util::OutputBuffer& buffer) const;
+    void toFormat(const NameChangeFormat format,
+                  isc::util::OutputBuffer& buffer) const;
 
     /// @brief Static method for creating a NameChangeRequest from a
     /// string containing a JSON rendition of a request.
@@ -226,7 +228,7 @@ public:
     ///  - That at least one of the two direction flags, forward change and
     ///    reverse change is true.
     ///
-    /// @TODO This is an initial implementation which provides a minimal amount
+    /// @todo This is an initial implementation which provides a minimal amount
     /// of validation.  FQDN, DHCID, and IP Address members are all currently
     /// strings, these may be replaced with richer classes.
     ///
@@ -244,7 +246,7 @@ public:
     /// @brief Sets the change type to the given value.
     ///
     /// @param value is the NameChangeType value to assign to the request.
-    void setChangeType(NameChangeType value);
+    void setChangeType(const NameChangeType value);
 
     /// @brief Sets the change type to the value of the given Element.
     ///
@@ -265,7 +267,7 @@ public:
     ///
     /// @param value contains the new value to assign to the forward change
     /// flag
-    void setForwardChange(bool value);
+    void setForwardChange(const bool value);
 
     /// @brief Sets the forward change flag to the value of the given Element.
     ///
@@ -287,7 +289,7 @@ public:
     ///
     /// @param value contains the new value to assign to the reverse change
     /// flag
-    void setReverseChange(bool value);
+    void setReverseChange(const bool value);
 
     /// @brief Sets the reverse change flag to the value of the given Element.
     ///
@@ -347,7 +349,7 @@ public:
 
     /// @brief Sets the DHCID based on the given string value.
     ///
-    /// @param string is a string of hexadecimal digits. The format is simply
+    /// @param value is a string of hexadecimal digits. The format is simply
     /// a contiguous stream of digits, with no delimiters. For example a string
     /// containing "14A3" converts to a byte array containing:  0x14, 0xA3.
     ///
@@ -420,7 +422,7 @@ public:
     /// @brief Sets the lease length to the given value.
     ///
     /// @param value contains the new value to assign to the lease length
-    void setLeaseLength(uint32_t value);
+    void setLeaseLength(const uint32_t value);
 
     /// @brief Sets the lease length to the value of the given Element.
     ///
@@ -440,7 +442,7 @@ public:
     /// @brief Sets the request status to the given value.
     ///
     /// @param value contains the new value to assign to request status
-    void setStatus(NameChangeStatus value);
+    void setStatus(const NameChangeStatus value);
 
     /// @brief Given a name, finds and returns an element from a map of
     /// elements.
@@ -471,8 +473,8 @@ private:
     bool reverse_change_;
 
     /// @brief The domain name whose DNS entry(ies) are to be updated.
-    /// @TODO Currently, this is a std::string but may be replaced with 
-    /// dns::Name which provides additional validation and domain name 
+    /// @todo Currently, this is a std::string but may be replaced with
+    /// dns::Name which provides additional validation and domain name
     /// manipulation.
     std::string fqdn_;
 
@@ -480,7 +482,7 @@ private:
     std::string ip_address_;
 
     /// @brief The lease client's unique DHCID.
-    /// @TODO Currently, this is uses D2Dhcid it but may be replaced with 
+    /// @todo Currently, this is uses D2Dhcid it but may be replaced with
     /// dns::DHCID which provides additional validation.
     D2Dhcid dhcid_;
 

src/bin/d2/tests/ncr_unittests.cc

@@ -220,30 +220,26 @@ TEST(NameChangeRequestTest, constructionTests) {
     ncr.reset();
 
     // Verify blank FQDN is detected.
-    EXPECT_THROW(ncr.reset(new NameChangeRequest(CHG_ADD, true, true, "",
-                 "192.168.1.101", dhcid, expiry, 1300)), NcrMessageError);
+    EXPECT_THROW(NameChangeRequest(CHG_ADD, true, true, "",
+                 "192.168.1.101", dhcid, expiry, 1300), NcrMessageError);
 
     // Verify that an invalid IP address is detected.
-    EXPECT_THROW(ncr.reset(
-                 new NameChangeRequest(CHG_ADD, true, true, "valid.fqdn",
-                 "xxx.168.1.101", dhcid, expiry, 1300)), NcrMessageError);
+    EXPECT_THROW(NameChangeRequest(CHG_ADD, true, true, "valid.fqdn",
+                 "xxx.168.1.101", dhcid, expiry, 1300), NcrMessageError);
 
     // Verify that a blank DHCID is detected.
     D2Dhcid blank_dhcid;
-    EXPECT_THROW(ncr.reset(
-                 new NameChangeRequest(CHG_ADD, true, true, "walah.walah.com",
-                 "192.168.1.101", blank_dhcid, expiry, 1300)), NcrMessageError);
+    EXPECT_THROW(NameChangeRequest(CHG_ADD, true, true, "walah.walah.com",
+                 "192.168.1.101", blank_dhcid, expiry, 1300), NcrMessageError);
 
     // Verify that an invalid lease expiration is detected.
     ptime blank_expiry;
-    EXPECT_THROW(ncr.reset(
-                 new NameChangeRequest(CHG_ADD, true, true, "valid.fqdn",
-                 "192.168.1.101", dhcid, blank_expiry, 1300)), NcrMessageError);
+    EXPECT_THROW(NameChangeRequest(CHG_ADD, true, true, "valid.fqdn",
+                 "192.168.1.101", dhcid, blank_expiry, 1300), NcrMessageError);
 
     // Verify that one or both of direction flags must be true.
-    EXPECT_THROW(ncr.reset(
-                 new NameChangeRequest(CHG_ADD, false, false, "valid.fqdn",
-                "192.168.1.101", dhcid, expiry, 1300)), NcrMessageError);
+    EXPECT_THROW(NameChangeRequest(CHG_ADD, false, false, "valid.fqdn",
+                "192.168.1.101", dhcid, expiry, 1300), NcrMessageError);
 
 }
 
@@ -275,9 +271,9 @@ TEST(NameChangeRequestTest, dhcidTest) {
     // Fetch the byte vector from the dhcid and verify if equals the expected
     // content.
     const std::vector<uint8_t>& converted_bytes = dhcid.getBytes();
-    EXPECT_EQ(expected_bytes.size(), converted_bytes.size()); 
-    EXPECT_TRUE (std::equal(expected_bytes.begin(), 
-                            expected_bytes.begin()+expected_bytes.size(), 
+    EXPECT_EQ(expected_bytes.size(), converted_bytes.size());
+    EXPECT_TRUE (std::equal(expected_bytes.begin(),
+                            expected_bytes.begin()+expected_bytes.size(),
                             converted_bytes.begin()));
 
     // Convert the new dhcid back to string and verify it matches the original
@@ -356,22 +352,10 @@ TEST(NameChangeRequestTest, invalidMsgChecks) {
     // NameChangeRequest. The attempt should throw a NcrMessageError.
     int num_msgs = sizeof(invalid_msgs)/sizeof(char*);
     for (int i = 0; i < num_msgs; i++) {
-        bool it_threw = false;
-        try {
-            NameChangeRequest::fromJSON(invalid_msgs[i]);
-        } catch (NcrMessageError& ex) {
-            it_threw = true;
-            std::cout << "Invalid Message: " << ex.what() << std::endl;
-        }
-
-        // If it the conversion didn't fail, log the index message and fail
-        // the test.
-        if (!it_threw) {
-            std::cerr << "Invalid Message not caught, message idx: " << i
-                      << std::endl;
-            EXPECT_TRUE(it_threw);
-
-        }
+        EXPECT_THROW(NameChangeRequest::fromJSON(invalid_msgs[i]),
+                     NcrMessageError) << "Invalid message not caught idx: "
+                     << i << std::endl << " text:[" << invalid_msgs[i] << "]"
+                     << std::endl;
     }
 }
 
@@ -389,21 +373,10 @@ TEST(NameChangeRequestTest, validMsgChecks) {
     // NameChangeRequest. The attempt should succeed.
     int num_msgs = sizeof(valid_msgs)/sizeof(char*);
     for (int i = 0; i < num_msgs; i++) {
-        bool it_threw = false;
-        try {
-            NameChangeRequest::fromJSON(valid_msgs[i]);
-        } catch (NcrMessageError& ex) {
-            it_threw = true;
-            std::cout << "Message Error: " << ex.what() << std::endl;
-        }
-
-        // If it the conversion failed log the index message and fail
-        // the test.
-        if (it_threw) {
-            std::cerr << "Valid Message failed, message idx: " << i
-                      << std::endl;
-            EXPECT_TRUE(!it_threw);
-        }
+        EXPECT_NO_THROW(NameChangeRequest::fromJSON(valid_msgs[i]))
+                        << "Valid message failed,  message idx: " << i
+                        << std::endl << " text:[" << valid_msgs[i] << "]"
+                        << std::endl;
     }
 }