BIND 10 #1198: Split DatabaseClient::Finder::find

[BIND 10 Development]

#1198: Split DatabaseClient::Finder::find
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  stephen
  vorner                             |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20111206
                   Priority:  minor  |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  4
            Defect Severity:  High   |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by stephen):

 ''This is just a comment - the changes still have to be made''

 > Basically, it looked very good. The resulting code was much, much easier
 to understand.
 Thank you.

 > But, I wanted to make the main find() method even more concise and (at
 least to me) understandable. Instead of pointing out how we could do it in
 the form review comment, I've made several proposed commits to the branch
 as I thought that would be easier to convey the intent.
 That's fine.


 > The key observation is that the code before the proposed changes were
 still not very readable due to intermediate mutable variables:
 result_status, result_rrset, record_found, and get_cover.
 Accepted - I'm afraid I was taking the simplest route and on the return
 from the method trying to set the state of find() back to what it was
 before I extracted a block of code into a separate method.

 > If you agree with the general intent of my proposal, please also note
 that you'll need to update the documentation of some of the methods you
 introduced in the branch and provide document for newer methods and new
 log messages.
 What would really help is to have a general description of the algorithm
 somewhere.  I found it difficult to follow the logic in some places - I
 was reverse engineering the documentation from the code.


 > First, I noted some variables can be defined as const. Since it should
 be obvious and less controversial, I made the changes directly (commit
 b77f5d1).
 NP

 > (In case we agree on the further refactoring idea) I'd also like to
 simplify the for loop of findWildcardMatch() using the same/similar
 technique as find(). But this method may not be so unreadable in its
 current form, so I didn't touch it.
 I'll have a look at it.

 > Does !DelegationSearchResult? have to be public?
 My bad, it should be private.

 > (Although this may not actually be a topic of this refactoring task) in
 findWildcardMatch(), isn't it possible (thought probably rare or due to a
 pathological database) that getting NSEC fails here?
 {{{
     found = getRRsets(wildcard, NSEC_TYPES(), true);
     result_rrset =
         found.second.find(RRType::NSEC())->second;
 }}}
 > should we handle such case add a test case for it?
 Probably - I'll investigate that one further.

 > findWildcardMatch document: there's another case (correctly) considered
 in the code: wildcard match was canceled due to the existence of a more
 specific subdomain.
 I'll add documentation for it.

 > A minor style issue: there are some blank lines for unclear purposes and
 with seemingly not so consistent policy. I'd personally remove these lines
 because it would make the methods shorter while the blank lines don't
 improve readability (at least to me). At least the policy of when to
 insert blank should be consistent. e.g. there's a blank line before 'else
 if':
 The idea is to make it easier to read - divide the code into "paragraphs"
 that do some particular task.  I find that easier to comprehend than one
 solid block of code and comments (even with syntax colouring).  I admit
 it's a bit inconsistent - for example, a two lines each containing a brace
 already gives enough white space separating the "paragraph" above from the
 one below.  They are separated more on visual effect than any particular
 policy.

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