[2976] Changes as a result of code review.

4 files changed, 77 insertions, 40 deletions

diff --git a/src/bin/d2/d2_update_message.cc b/src/bin/d2/d2_update_message.cc
index 1ee6543517..71fb9f341c 100644
--- a/src/bin/d2/d2_update_message.cc
+++ b/src/bin/d2/d2_update_message.cc
@@ -23,11 +23,12 @@ namespace d2 {
 
 using namespace isc::dns;
 
-D2UpdateMessage::D2UpdateMessage(const bool parse)
-    : message_(parse ? dns::Message::PARSE : dns::Message::RENDER) {
+D2UpdateMessage::D2UpdateMessage(const Direction direction)
+    : message_(direction == INBOUND ?
+               dns::Message::PARSE : dns::Message::RENDER) {
     // If this object is to create an outgoing message, we have to
     // set the proper Opcode field and QR flag here.
-    if (!parse) {
+    if (direction == OUTBOUND) {
         message_.setOpcode(Opcode(Opcode::UPDATE_CODE));
         message_.setHeaderFlag(dns::Message::HEADERFLAG_QR, false);
 
@@ -131,16 +132,18 @@ D2UpdateMessage::fromWire(isc::util::InputBuffer& buffer) {
     // not be the message that we are interested in, but needs to be
     // parsed so as we can check its ID, Opcode etc.
     message_.fromWire(buffer);
-    // This class implements exposes the getZone() function. This function
-    // will return pointer to the D2Zone object if non-empty Zone
-    // section exists in the received message. It will return NULL pointer
-    // if it doesn't exist. The pointer is held in the D2UpdateMessage class
-    // member. We need to update this pointer every time we parse the
-    // message.
+    // This class exposes the getZone() function. This function will return
+    // pointer to the D2Zone object if non-empty Zone section exists in the
+    // received message. It will return NULL pointer if it doesn't exist.
+    // The pointer is held in the D2UpdateMessage class member. We need to
+    // update this pointer every time we parse the message.
     if (getRRCount(D2UpdateMessage::SECTION_ZONE) > 0) {
         // There is a Zone section in the received message. Replace
         // Zone pointer with the new value.
         QuestionPtr question = *message_.beginQuestion();
+        // If the Zone counter is greater than 0 (which we have checked)
+        // there must be a valid Question pointer stored in the message_
+        // object. If there isn't, it is a programming error.
         assert(question);
         zone_.reset(new D2Zone(question->getName(), question->getClass()));
 
@@ -155,7 +158,7 @@ D2UpdateMessage::fromWire(isc::util::InputBuffer& buffer) {
     // or an error message can be printed. Other than that, we
     // will check that there is at most one Zone record and QR flag
     // is set.
-    validate();
+    validateResponse();
 }
 
 dns::Message::Section
@@ -184,7 +187,7 @@ D2UpdateMessage::ddnsToDnsSection(const UpdateMsgSection section) {
 }
 
 void
-D2UpdateMessage::validate() const {
+D2UpdateMessage::validateResponse() const {
     // Verify that we are dealing with the DNS Update message. According to
     // RFC 2136, section 3.8 server will copy the Opcode from the query.
     // If we are dealing with a different type of message, we may simply
@@ -198,9 +201,9 @@ D2UpdateMessage::validate() const {
     // Received message should have QR flag set, which indicates that it is
     // a RESPONSE.
     if (getQRFlag() == REQUEST) {
-        isc_throw(InvalidQRFlag, "received message should should have QR flag"
-                  << " set, to indicate that it is a RESPONSE message, the QR"
-                  << " flag is unset");
+        isc_throw(InvalidQRFlag, "received message should have QR flag set,"
+                  " to indicate that it is a RESPONSE message; the QR"
+                  << " flag in received message is unset");
     }
     // DNS server may copy a Zone record from the query message. Since query
     // must comprise exactly one Zone record (RFC 2136, section 2.3), the
diff --git a/src/bin/d2/d2_update_message.h b/src/bin/d2/d2_update_message.h
index 44d0cd4cfc..98e98a8a6c 100644
--- a/src/bin/d2/d2_update_message.h
+++ b/src/bin/d2/d2_update_message.h
@@ -81,14 +81,23 @@ public:
 class D2UpdateMessage {
 public:
 
-    /// Indicates whether DNS Update message is a REQUEST or RESPONSE.
+    /// @brief Indicates if the @c D2UpdateMessage object encapsulates Inbound
+    /// or Outbound message.
+    enum Direction {
+        INBOUND,
+        OUTBOUND
+    };
+
+    /// @brief Indicates whether DNS Update message is a REQUEST or RESPONSE.
     enum QRFlag {
         REQUEST,
         RESPONSE
     };
 
-    /// Identifies sections in the DNS Update Message. Each message comprises
-    /// message Header and may contain the following sections:
+    /// @brief Identifies sections in the DNS Update Message.
+    ///
+    /// Each message comprises message Header and may contain the following
+    /// sections:
     /// - ZONE
     /// - PREREQUISITE
     /// - UPDATE
@@ -116,9 +125,8 @@ public:
     /// message contents and @c D2UpdateMessage::toWire function to create
     /// on-wire data.
     ///
-    /// @param parse indicates if this is an incoming message (true) or outgoing
-    /// message (false).
-    D2UpdateMessage(const bool parse = false);
+    /// @param direction indicates if this is an inbound or outbound message.
+    D2UpdateMessage(const Direction direction = OUTBOUND);
 
     ///
     /// @name Copy constructor and assignment operator
@@ -303,9 +311,22 @@ private:
     /// @throw isc::d2::InvalidQRFlag if QR flag is not set to RESPONSE
     /// @throw isc::d2::InvalidZone section, if Zone section comprises more
     /// than one record.
-    void validate() const;
-
+    void validateResponse() const;
+
+    /// @brief An object representing DNS Message which is used by the
+    /// implementation of @c D2UpdateMessage to perform low level.
+    ///
+    /// Declaration of this object pollutes the header with the details
+    /// of @c D2UpdateMessage implementation. It might be cleaner to use
+    /// Pimpl idiom to hide this object in an D2UpdateMessageImpl. However,
+    /// it would bring additional complications to the implementation
+    /// while the benefit would low - this header is not a part of any
+    /// common library. Therefore, if implementation is changed, modification of
+    /// private members of this class in the header has low impact.
     dns::Message message_;
+
+    /// @brief Holds a pointer to the object, representing Zone in the DNS
+    /// Update.
     D2ZonePtr zone_;
 
 };
diff --git a/src/bin/d2/tests/d2_update_message_unittests.cc b/src/bin/d2/tests/d2_update_message_unittests.cc
index e83653e0ba..28e01c76e1 100644
--- a/src/bin/d2/tests/d2_update_message_unittests.cc
+++ b/src/bin/d2/tests/d2_update_message_unittests.cc
@@ -33,13 +33,18 @@ using namespace isc::util;
 
 namespace {
 
+// @brief Test fixture class for testing D2UpdateMessage object.
 class D2UpdateMessageTest : public ::testing::Test {
 public:
-    D2UpdateMessageTest() {
-    }
+    // @brief Constructor.
+    //
+    // Does nothing.
+    D2UpdateMessageTest() { }
 
-    ~D2UpdateMessageTest() {
-    };
+    // @brief Destructor.
+    //
+    // Does nothing.
+    ~D2UpdateMessageTest() { };
 
     // @brief Return string representation of the name encoded in wire format.
     //
@@ -58,18 +63,16 @@ public:
     //
     // @return string representation of the name.
     std::string readNameFromWire(InputBuffer& buf, size_t name_length,
-                                 bool no_zero_byte = false) {
-        // 64 characters bytes should be sufficent for current tests.
-        // It may be extended if required.
-        char name_data[64];
+                                 const bool no_zero_byte = false) {
+        std::vector<uint8_t> name_data;
         // Create another InputBuffer which holds only the name in the wire
         // format.
-        buf.readData(name_data, name_length);
+        buf.readVector(name_data, name_length);
         if (no_zero_byte) {
             ++name_length;
-            name_data[name_length-1] = 0;
+            name_data.push_back(0);
         }
-        InputBuffer name_buf(name_data, name_length);
+        InputBuffer name_buf(&name_data[0], name_length);
         // Parse the name and return its textual representation.
         Name name(name_buf);
         return (name.toText());
@@ -201,7 +204,7 @@ TEST_F(D2UpdateMessageTest, fromWire) {
     InputBuffer buf(bin_msg, sizeof(bin_msg));
 
     // Create an object to be used to decode the message from the wire format.
-    D2UpdateMessage msg(true);
+    D2UpdateMessage msg(D2UpdateMessage::INBOUND);
     // Decode the message.
     ASSERT_NO_THROW(msg.fromWire(buf));
 
@@ -288,7 +291,7 @@ TEST_F(D2UpdateMessageTest, fromWireInvalidOpcode) {
     // The 'true' argument passed to the constructor turns the
     // message into the parse mode in which the fromWire function
     // can be used to decode the binary mesasage data.
-    D2UpdateMessage msg(true);
+    D2UpdateMessage msg(D2UpdateMessage::INBOUND);
     // When using invalid Opcode, the fromWire function should
     // throw NotUpdateMessage exception.
     EXPECT_THROW(msg.fromWire(buf), isc::d2::NotUpdateMessage);
@@ -312,7 +315,7 @@ TEST_F(D2UpdateMessageTest, fromWireInvalidQRFlag) {
     // The 'true' argument passed to the constructor turns the
     // message into the parse mode in which the fromWire function
     // can be used to decode the binary mesasage data.
-    D2UpdateMessage msg(true);
+    D2UpdateMessage msg(D2UpdateMessage::INBOUND);
     // When using invalid QR flag, the fromWire function should
     // throw InvalidQRFlag exception.
     EXPECT_THROW(msg.fromWire(buf), isc::d2::InvalidQRFlag);
@@ -351,7 +354,7 @@ TEST_F(D2UpdateMessageTest, fromWireTooManyZones) {
     // The 'true' argument passed to the constructor turns the
     // message into the parse mode in which the fromWire function
     // can be used to decode the binary mesasage data.
-    D2UpdateMessage msg(true);
+    D2UpdateMessage msg(D2UpdateMessage::INBOUND);
     // When parsing a message with more than one Zone record,
     // exception should be thrown.
     EXPECT_THROW(msg.fromWire(buf), isc::d2::InvalidZoneSection);
@@ -572,7 +575,7 @@ TEST_F(D2UpdateMessageTest, toWireInvalidQRFlag) {
     // The 'true' argument passed to the constructor turns the
     // message into the parse mode in which the fromWire function
     // can be used to decode the binary mesasage data.
-    D2UpdateMessage msg(true);
+    D2UpdateMessage msg(D2UpdateMessage::INBOUND);
     ASSERT_NO_THROW(msg.fromWire(buf));
 
     // The message is parsed. The QR Flag should now indicate that
diff --git a/src/bin/d2/tests/d2_zone_unittests.cc b/src/bin/d2/tests/d2_zone_unittests.cc
index 8a7e9d5f20..853cdbeabe 100644
--- a/src/bin/d2/tests/d2_zone_unittests.cc
+++ b/src/bin/d2/tests/d2_zone_unittests.cc
@@ -23,24 +23,34 @@ using namespace isc::dns;
 
 namespace {
 
+// This test verifies that Zone object is created and its constructor sets
+// appropriate values for its members.
 TEST(D2ZoneTest, constructor) {
+    // Create first object.
     D2Zone zone1(Name("example.com"), RRClass::ANY());
     EXPECT_EQ("example.com.", zone1.getName().toText());
     EXPECT_EQ(RRClass::ANY().getCode(), zone1.getClass().getCode());
-
+    // Create another object to make sure that constructor doesn't assign
+    // fixed values, but they change when constructor's parameters change.
     D2Zone zone2(Name("foo.example.com"), RRClass::IN());
     EXPECT_EQ("foo.example.com.", zone2.getName().toText());
     EXPECT_EQ(RRClass::IN().getCode(), zone2.getClass().getCode());
 }
 
+// This test verifies that toText() function returns text representation of
+// of the zone in expected format.
 TEST(D2ZoneTest, toText) {
+    // Create first object.
     D2Zone zone1(Name("example.com"), RRClass::ANY());
     EXPECT_EQ("example.com. ANY SOA\n", zone1.toText());
-
+    // Create another object with different parameters to make sure that the
+    // function's output changes accordingly.
     D2Zone zone2(Name("foo.example.com"), RRClass::IN());
     EXPECT_EQ("foo.example.com. IN SOA\n", zone2.toText());
 }
 
+// This test verifies that the equality and inequality operators behave as
+// expected.
 TEST(D2ZoneTest, compare) {
     const Name a("a"), b("b");
     const RRClass in(RRClass::IN()), any(RRClass::ANY());