Code review comment for lp:~marcustomlinson/unity-api/lp-1552082

Revision history for this message
Michi Henning (michihenning) wrote :

Looks good! Nice coverage too! :-)

Some very minor code and doc quibbles below...

In the constructor and destructor, there is no need to lock the mutex.

"To extract / insert a value, simply specify the group and key
 names to the get* / set* methods of this class respectively."

The "/" needs to be close set. I'd also remove the "*", and a comma needs to be added:

"To extract/insert a value, simply specify the group and key
 names to the get/set methods of this class, respectively."

"The get methods indicate errors by throwing LogicExceptions."

I'd remove the plural:

"The get methods indicate errors by throwing LogicException."

"Examples why this might happen is because a value can't be
coerced into the given type (i.e trying to convert the value
"hello" into a boolean)."

"i.e" is missing a full stop here. But I'd be inclined to delete this sentence entirely because almost the same information is provided in the doc for the read methods below. For completeness, I'd reword that comment as:

    /** @name Read Methods
     * These member functions provide read access to configuration entries by group and key.<br>
     * Attempts to retrieve a value as the wrong type (such as retrieving a string value
     * as an integer) or to retrieve a value for a non-existent group or
     * key throw LogicException.
     **/

In the summary comment for the class, I'd add that all methods are thread-safe.

    /** @name Write Methods
     * These member functions provide write access to configuration entries by group and key.<br>
     * Attempts to remove groups or keys that do not exist, throw LogicException.
      **/

Remove the comma before "throw LogicException." I would also add:

       The set methods replace the value for a key if the key exists. Calling a set method for
       a non-existent group and/or key creates the group and/or key.

    /** @name Sync Method
     * This member function will write unsaved changes back to the configuration file.<br>
     * A failure to write to file will throw a FileException.
      **/

I would get rid of the future tense:

    /** @name Sync Method
     * This member function writes unsaved changes back to the configuration file.<br>
     * A failure to write to the file throws a FileException.
     **/

review: Needs Fixing

« Back to merge proposal