BIND 10 #2634: The DhcpConfigParser::getParam should be abstracted to a separate class.

[BIND 10 Development]

#2634: The DhcpConfigParser::getParam should be abstracted to a separate class.
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:  tmark
                Type:  enhancement   |                       Status:
            Priority:  low           |  reviewing
           Component:  dhcpconf      |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130411
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => tmark


Comment:

 Reviewed commit ff8f84b695de0e5350af325c675ab2bdbab79cc0

 '''General'''[[BR]]
 There is no need to attach the output of unit tests to the ticket.

 '''src/bin/dhcp4/config_parser.cc'''[[BR]]
 Why did one of the changes move the include of dhcp4/config_parser.h lower
 down the list of includes (originally the include files were in
 alphabetical order)?

 The include of dhcpsrv/dhcp_config_parser.h is commented out.

 createSubnet - please start comments with a capital letter ("Rethrow with
 precise error").

 '''src/bin/dhcp4/tests/config_parser_unittest.cc'''[[BR]]
 Dhcp4ParserTest::checkGlobalUint32: commented-out declarations have been
 left in.  They should be removed.

 '''src/lib/dhcpsrv/dhcp_config_parser.h'''[[BR]]
 The file "stdint.h" is included.  The C++ version of this is "cstdint"

 Typo in header of !ValueStorage class - "@tparam".

 setParam - typos in header: "parmater", "paramater". (Second typo also
 occurs in the description of the return value in getParam.)

 setParam takes its parameters by value.  If these are complex objects
 (like strings), that will cause a local variable to be created via a copy
 constructor then deleted.  I suggest that they are passed by reference.

 Suggest adding a blank line after the end of setParam to separate it from
 the next method. (This is done elsewhere.)

 getParam - text in exception should start with a capital letter ("Missing
 parameter").

 getParam - is there a need for the intermediate statement setting "value"?
 {{{return (param->second)}}} seems reasonably clear.

 delParam - pass parameter by reference?

 '''src/lib/dhcpsrv/tests/cfgmgr_unittest.cc'''[[BR]]
 (All comments apply to the new tests)

 Please start comments with capital letters.

 What is the "them" in the comments?

 The tests should also check:
 * Trying to get something that never existed in the storage throws an
 exception. (The current test only checks that getting something that has
 been deleted throws an exception.)
 * The expected behaviour of deleting something that does not exist.

 The failure of deleting one of the parameters can be done via an
 EXPECT_THROW.  If that test fails, the subsequent tests can still be
 carried out. (The same goes for the test after emptying the list.)

 I'm surprised it's not caught by the compiler, but in the testing of
 Uint32Storage, one of the test value is negative.

-- 
Ticket URL: <http://bind10.isc.org/ticket/2634#comment:4>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development