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

[BIND 10 Development]

#1198: Split DatabaseClient::Finder::find
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  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 jinmei):

 Replying to [comment:10 stephen]:
 > Extracted from find() the logic that searches for delegations and the
 logic that checks for wildcard matches into separate methods.
 >
 > In the header for those functions added a a description of the
 functionality beyond that in the original code based on my understanding
 of the source.

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

 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.
 (of course, you may or may not agree with the changes, so they can be
 reverted based on the review discussion).  The suggested changes are
 commits from cf8596b to b447162.  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.  In particular, it was quite difficult to
 figure out how record_found and get_cover affect the final return
 value because the points where they are set and used were not close.
 On looking into the code more closely, I found that the cases where
 the special handling is necessary are actually quite limited.  So
 after some incremental cycles of further refactoring, I eliminated the
 use of these variables and let each "(else) if" case return the
 determined result at that point.

 I've also introduced a new private method findNoNameResult(), mainly
 for making find() more concise.  This may not absolutely be necessary,
 but IMO the resulting code is now easy to understand "at a glance"
 thanks to its concision.

 Commit b447162 may be most controversial: it introduced another method
 to log the event, which was lost due to the first attempt of the
 refactoring.  Also, we need to provide more log IDs for each specific
 case in find().  And, while I didn't do it in my proposal, we'll also
 need to do similar thing for findNoNameResult().

 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.

 Now, here are comments on the original branch before applying my
 suggestion.

 - 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).

 - (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.

 - Does DelegationSearchResult have to be public?

 - (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?
 {{{#!c++
                         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?

 - findWildcardMatch document: there's another case (correctly)
   considered in the code: wildcard match was canceled due to the
   existence of a more specific subdomain.

 - 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':
 {{{#!c++
             result_rrset = nsi->second;

         } else if (type != isc::dns::RRType::CNAME() &&
 }}}
   but not here:
 {{{#!c++
             }
         } else if (dnssec_data) {
 }}}
   There is a blank line after 'if':
 {{{#!c++
     if (!result_rrset) { // Only if we didn't find a redirect already

 }}}
   but not here:
 {{{#!c++
         if (found.first) {
             if (first_ns) {
 }}}
   etc.

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