Re: is it wise to provide hooks to customize ExceptionalUnitGraph's constructors?

[Ondrej Lhotak <olhotak@sable.mcgill.ca>]

Hi John...

In general, I try to discourage placing a lot of code in the constructor
or in methods called from the constructor (although this is a common
practice in Soot), because of the various restrictions on constructors.
These include the overriding restriction that you noted, as well as the
fact that a constructor cannot signal a failure because it returns
nothing. Doing lots in the constructor can also be the source of subtle
bugs. For example, if the superclass constructor calls method foo()
and a subclass overrides foo(), then foo() will be called before the
subclass constructor has finished, and the class invariant may not have
been established yet.

My advice is this: the constructor should establish the class invariant,
and nothing else. The class invariant should be chosen to be simple
enough that it can be established in the constructor.

Your solution of having a protected constructor which calls initialize()
solves the problem you're having, but not all of the problems with doing
lots in the constructor. It's probably OK, but in general, I think it's
preferable to not call initialize() in the constructor, and call it at
the construction site instead. The common objection to this is that
people don't want to have to call initialize() all the time after
constructing the object. The object oriented (tm) answer to this
objection is to make the constructor accessible only to a suitable
factory method which calls initialize() for you.

When the amount of work to be done to build the thing is substantial, my
personal preference is to split up the builder and the container into
separate classes, so that one may subclass one without the other (see,
for example, the CallGraph and CallGraphBuilder). I don't know that I
can provide much justification for this; it's just personal preference.

Ondrej

On Sun, Mar 21, 2004 at 06:26:47PM -0600, John Jorgensen wrote:
> About two weeks ago, I suggested to Venkatesh Ranganath
> (whose code I broke by reorganizing Soot's CFG classes) that he
> could extend ExceptionalUnitGraph instead of UnitGraph, and that
> he might be able to get the graphs he wants by selectively
> overriding methods called by the ExceptionalUnitGraph
> constructor.
> 
> A week ago, Venkatesh told me about a problem with my
> suggestion to override ExceptionalUnitGraph and suggested a
> solution. The solution cleanly solves the immediate difficulty,
> but it made me realize that by encouraging people to extend
> Soot's concrete graph classes---especially when the subclass
> needs to change what is produced by the superclass's
> constructor---I may be unwisely limiting the scope for future
> changes to the graph implementations.  So I'm looking for advice
> on how to proceed, while the graph changes are still
> unreleased and Venkatesh is still the only person who will suffer
> from my waffling.
> 
> First, the hitch that Venkatesh encountered while trying to
> extend ExceptionalUnitGraph:
> 
> The ExceptionalUnitGraph constructor has this basic form:
> 
>   public ExceptionalUnitGraph(Body b, ThrowAnalysis t, boolean o) {
> 
>     super(b);
> 
>     buildUnexceptionalEdges(); // Establish regular control flow.
> 
>     buildExceptionDests();     // Determine which units throw
> 			       // which exceptions to which
> 			       // handlers.
> 
>     buildExceptionalEdges();   // Add CFG edges that correspond
> 			       // to the caught exceptions.
> 
>     buildHeadsAndTails();
>   }
> 
> buildUnexceptionalEdges() is inherited from UnitGraph while the
> other methods are defined in ExceptionalUnitGraph. All are
> "protected".  
> 
> My understanding is that Venkatesh wants to filter out some graph
> edges, depending on input from a user.  I suggested that if
> Venkatesh couldn't get the set of graph edges he wants by passing
> a customized ThrowAnalysis, he could override one or both of
> buildExceptionDests() or buildExceptionalEdges().  He replied
> that overriding buildExceptionalEdges() could do what he wants
> except for one hitch: his buildExceptionalEdges() would need
> information provided as a parameter to his subclass's
> constructor, but since the first thing a subclass constructor
> does is to invoke its superclass's constructor, there would be no
> opportunity to do anything with the subclass parameters until
> after the superclass constructor had already called
> buildExceptionalEdges().
> 
> Venkatesh suggested moving the bulk of the current constructor
> into a separate method.  The public ExceptionalUnitGraph
> constructor would call this method to actually build the graph,
> while a new protected constructor would not, like this:
> 
>   public ExceptionalUnitGraph(Body b, ThrowAnalysis t, boolean o) {
>     super(b);
>     initialize(t, o);
>   }
> 
>   protected ExceptionalUnitGraph(Body b) {
>     super(b);
>   }
> 
>   protected void initialize(ThrowAnalysis t, boolean o) {
>     buildUnexceptionalEdges();
>     buildExceptionDests();
>     buildExceptionalEdges();
>     buildHeadsAndTails();
>   }
> 
> Subclass constructors could then delay calling initialize() until
> after they perform subclass-specific initialization.
> 
> This is a clever way of dealing with the problem at hand, but it
> gives the subclass the responsibility of ensuring that the
> superclass is initialized properly. That makes me reconsider the
> wisdom of encouraging the subclassing of ExceptionalUnitGraph.  
> 
> (Incidentally, I can imagine Rube Goldbergian schemes that would
> allow Venkatesh to do what he wanted without modifying
> ExceptionalUnitGraph: things like providing his subclass with a
> static method, stashMyParameters(...), that tucked the
> information required by his buildExceptionalEdges() into some
> static variables from which his it could retrieve them.  Not only
> would that be an ugly kludge, though, it would still depend on
> the internal organization of ExceptionalUnitGraph's constructor,
> and it's the wisdom of that commitment that I'm starting to
> doubt.)
> 
> I think I can explain my misgivings in terms of the 
> different ways to think about inheritance.  
> 
> My reorganization of the graph classes focuses on the inheritance
> of implementation, to minimize duplicated code while keeping
> subclass-specific code in the subclasses. Venkatesh's suggestion
> widens the scope for inheriting implementation by providing
> another hook to customize the inherited code.
> 
> Another aspect of inheritance is the inheritance of an
> interface.  Now in a narrow sense, Java's language definition
> guarantees that when you define class C2 by extending C1, C2 will
> have the same interface:  all the public and protected methods of
> C1 will be present in C2, with the same signatures and return
> types.
> 
> But there's a fuzzier notion of what it means to inherit an
> interface: calling C2.m() should in some sense mean the same
> thing as calling C1.m().  I presume that maintaining this
> conceptual interface is what textbooks have in mind when they
> encourage us to think of "C2 extends C1" as meaning "C2 is a C1".
> 
> To provide a whimsical illustration, my initial misgivings were
> that I might have created a sensible hierarchy akin to
> 
>    Dog is-a Mammal is-a Vertebrate is-a Creature
> 
> and then, by encouraging them to reuse code in Dog, I was
> tempting programmers into using Dog to implement Cat. That might
> well save code, but callers of bark() don't expect to hear
> "meow".
> 
> These misgivings about the appropriate use of subclassing are not
> by themselves a reason to bar extension to ExceptionalUnitGraph:
> Soot is a framework for programmers to build on; they have to
> decide for themselves which extensions make sense and which
> tradeoffs between expedience and clarity are worthwhile.  But the
> conceptual misgivings lead me to a more pragmatic concern.
> 
> By encouraging people to subclass the graph classes, I am
> implicitly guaranteeing client code that the ExceptionalUnitGraph
> constructor will always be structured the way it is now.  But I'm
> not smart enough to think of all the ways that
> ExceptionalUnitGraph might need to change in the future, nor to
> re-re-organize it so that the code appropriate for reuse is
> factored into abstract classes (i.e., I can see that Dog.bark()
> should be replaced with a parameterized Creature.speak(), but
> it's not so clear to me how to factor out the reusable parts of
> ExceptionalUnitGraph.)
> 
> While thinking about this I reread Items 14, "Favor Composition
> over Inheritance", and 15, "Design and Document for Inheritance
> or else Prohibit it", in Joshua Bloch's "Effective Java". In
> light of the former, I've asked Venkatesh if he can get what he
> needs by creating an instance of ExceptionalUnitGraph within a
> wrapper class. In light of the latter, I'm leaning to changing
> most of the protected hooks I've introduced into some
> soot.toolkits.graph constructors into package private members;
> that would let us continue to inherit implementations within the
> package without turning the implementation into part of the
> interface to which we are committed.
> 
> Any insights? 
>