BIND 10 #977: ACL base class

[BIND 10 Development]

#977: ACL base class
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  vorner                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20110614
                  Component:         |            Resolution:
  Unclassified                       |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  4.0
Feature Depending on Ticket:  ACL    |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:8 vorner]:

 > Well, having this kind of hierarchy for single context (if we want
 different kinds of checks) looks like textbook example for polymorphism.
 >
 > And while I believe we want to share code with different contexts where
 it makes sense, parts of the hierarchy will be completely different. And
 it would be problematic to write some generic base class for context,
 check every time it is the correct child, typecast it... So this looks
 like a textbook example for templates. So I just put them together into
 one class.

 While I'm not yet sure whether this is the best, I don't object to
 this design policy at the moment (I'm just uncertain, not necessarily
 doubting it).

 > >   - the fact that having both Check and CompoundCheck are a base class
 > >     indicates we'll have quite deep levels of inheritance hierarchy.
 > >     without a care such a design can be difficult to manage.
 >
 > Why so? And is 3 so deep? Most GUI components in whatever library
 > has depths around 15.

 Depth itself is not immediately a problem, but when we (heavily) try
 to represent different behaviors by defining derived classes
 recursively, it will easily lead to an explosion of classes.  If we
 talk about textbooks, that's a common lesson of design pattern
 textbooks.  But, after all, it depends on specific cases, so this
 concern may or may not be applicable to our case.  As I said in the
 previous bullet, I'm simply not certain as I've only seen a piece of
 the entire design with actual code and practices.

 This is not a showstopper comment anyway, and I think we now should
 move forward.  I'd suggest closing this discussion, completing this
 branch and allowing other tasks to start.

 > > - maybe it's better to make the constructor of the base class
 protected.
 >
 > What is the rationale behind this? There's no constructor, so nothing to
 make hidden really. And the compiler won't let to create this class
 directly anyway, as it has pure virtual methods, so the goal to stop
 people from creating it by accident is fulfilled automatically as well. Is
 there any other reason I don't see?

 Hmm, my reason was to prevent an accidental instantiation of the base
 class, but you're right that it's not possible due to the existence of
 a pure virtual (with no definition) method.  I still think it's a good
 practice so that it will be robust against a possible future change
 like introducing the default implementation of these methods (while we
 still want to make the base class abstract), but I'd leave the
 decision to you.

 > > - I'd like to have descriptions about possible exceptions, at least
 > >   for some important methods such as matches().
 >
 > Like this?

 Looks okay.

 > > - is it intentional that Check::toText() is *not* virtual?  If it's
 intentional,
 > >   the fact that this class has a pure virtual method will cause an
 > >   awkward effect, i.e., we'll have to provide the implementation of
 > >   the pure virtual method, although it's not prohibited.
 >
 > This was an error, thanks for noticing. I wrote the header and comment
 only, but forgot even the implementation.

 The revised code looks okay.  (Actually I was a bit confused myself
 and was wrong about saying we'll have to provide the implementation,
 but in any case my another point held; I thought it was intended to be
 virtual, and the revised code looks okay in that sense).

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