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-20111220
                   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:         |
  Datasrc refactoring                |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:21 stephen]:

 First, I've applied the proposed diff to the latest version of the
 branch as requested.

 > > I agree the added comments help understand what's happening (and
 > > in some specific cases why), and they are very good for that
 > > purpose. But at the same time I'm afraid the massive amount of
 > > in-method comments rather reduces readability at a glance and would
 > > make it more difficult to extend the code later with confidence. [...]
 > Here I have to disagree with you.  I find it more awkward to have to
 > bounce back and forth between the header and the code to decide what
 > is happening.  I think the header should be for the general overview
 > of what the function does, perhaps with details of the algorithm.
 > But if that gets too detailed, it becomes pseudocode. (Also, the
 > function header is usually put in the .h file, so is separate from
 > the code.)

 IMO in general, ideally we should make the code itself sufficiently
 clear that wouldn't require such literal comments (whether it's in the
 body or the head) just to understand what's happening.  If one cannot
 understand what a method does without that level of detailed comments
 we should actually fix it by refactoring the code itself (just as we
 are doing now) rather than just trying to explain it in comments.

 So, the basic assumption of mine here is that we have made the code
 reasonably readable.  Detailed comments may still be useful,
 especially for someone who first tries to read the code, so I'm
 positive about leaving the comments, but with the assumption that the
 code itself should already be quite clean and understandable,
 excessive amount of in-function comments will rather reduce the
 readability.

 I agree that it's awkward and inconvenient to have to bounce between
 code and comments, but if inserting the comments in the function body
 reduces the readability, I'd rather live with the inconvenience and
 keep the code body concise.

 This is what I wanted to convey via the example below.

 > > Now, imagine that each comment provides very detailed explanation of
 what is going on each case and why we do that, etc, and consumes the half
 of "vertical screen size". [...]
 > I think that if you are not clear what is happening - and very
 importantly, why it is happening - because of insufficient detail, it is
 probably unsafe to add code anyway.

 Of course, but the assumption is that the code should already be
 pretty clear about what's happening.

 The logic duplicate between find() and findWildcardMatch() was a good
 example of this.  The latter was long and less readable (to me) due to
 the inserted comments, so I first tried to get the entire view by
 extracting the code structure

 {{{#!c++
                 if (cni != found.second.end() && type != RRType::CNAME())
 {
                 } else if (nsi != found.second.end()) {
                 } else if (wti != found.second.end()) {
                 } else {
                     // Found a wildcard match to the name but not to the
 type
                     // (i.e. NXRRSET).
                }
 }}}

 and then noticed it was quite similar to the code logic of find():
 {{{#!c++
     if (!is_origin && ((options & FIND_GLUE_OK) == 0) &&
         nsi != found.second.end()) {
     } else if (type != RRType::CNAME() && cni != found.second.end()) {
     } else if (wti != found.second.end()) {
     } else if (!found.first) {
         return (findNoNameResult(name, type, options, dresult));
     } else {
          // NSEC case
     }
 }}}

 It would have been much easier to notice if each block of if/else if
 had had a few of lines of code (with possibly a small amount of
 comments).  But the large amount of comments in the actual code
 obscured the entire structure so I had to manually extracted it.

 > However...

 And that said...

 > I think we are both agreed on the need for adequate documentation, we
 just have a different opinion of where it should be put.  [...]  In the
 end though it doesn't really matter where the documentation is, providing
 it is there; so if you prefer to have all the documentation in the header,
 I'm fine with that.

 I also admit what's readable is basically a subjective matter, and
 since you were the original author of this refactoring, if we still
 disagree I respect your preference.

 In my latest push to the branch the in-function comments for
 findWildcardMatch were moved to the head of the function, but if you
 like I'm okay with reverting them to the body on merge.

 > '''findWildcardMatch'''[[BR]]
 > > Personally, I'd use assert for the "sanity check" for FIND_GLUE_OK
 because should this happen it's a pure internal bug within database.cc
 (even a broken database table cannot make this happen).
 > True, but if it ever happens in the wild due to a subsequent change to
 the code, we end up with the server stopping and a cryptic abort message.
 At least with an exception (a) the problem is reported through the normal
 logging channels, (b) it is not disabled if NDEBUG is set and (c) we have
 to opportunity to take remedial action, e.g. abort the current query.
 That way we could at least run a degraded service instead of no service.

 I see the points of the exception approach.  I'm still personally
 afraid about being too lenient about clear internal bugs, because if
 the bug actually severely breaks the internal integrity (e.g. a large
 amount of memory corruption) it may be just dangerous to pretend we
 can safely recover from it and keep running.  I think we should seek a
 good balance between minimizing the risk of keeping (possibly)
 corrupted state and improving resilience.

 I think this is a bigger issue than this particular case (maybe a good
 discussion topic for the next f2f meeting?).  For the purpose of this
 branch, I'm okay with keeping the current approach (if you don't want
 to change it for now).

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