[9] Code Review Request For 8157350: Encapsulate impl_ methods in Shapes related classes

[Kevin Rushforth]

Jim Graham wrote:
>
>
> On 5/20/16 3:33 PM, Kevin Rushforth wrote:
>> This is needed for those cases where we need to encapsulate a method 
>> in the base Shape class that used to be public and
>> overridden in the subclasses, not all of which are in the same 
>> package. It may seem like overkill, but we need a way to
>> associate the the Shape instance of a particular subtype with the 
>> helper instance of the correct subtype. Each class in
>> the hierarchy calls the specific XxxxxHelper.initHelper(this) method 
>> so that it can store back an instance of the right
>> helper in the base class. A package-private method wouldn't work 
>> given that some shapes (e.g., Text) are in different
>> packages.
>
> Right, but (taking Arc as an example) Arc makes a specific reference 
> to ArcHelper which turns around and hands a specific instance to its 
> own instance field to a method that stores the value in the 
> shapeHelper field.  How is that any different from just putting 
> shapeHelper = ArcHelper.instance without 2 method calls and an 
> accessor in the way?

But the shapeHelper field is in the base Shape class not in the Arc 
class. If we wanted a different pattern for classes in the same package 
as the base class from classes in a different package then I guess I can 
see how this is solvable by making the ArcHelper.getInstance() method 
public and having the Arc() constructors call the package-scope 
setHelper(ShapeHelper) method in Shape, but as soon as Chien move the 
stored helper instance up to Node (which is the next step) it would stop 
working.

> Also, what if someone creates a custom sub-class of Shape?  (Not sure 
> if that is supported or possible, but it is a public class with a 
> public constructor so I don't think it is impossible.)

Since we don't have public API for many of the things they would need 
even today, an application isn't able to do that. They couldn't really 
do it anyway before this change, since impl_getPeer() and several other 
methods aren't implementable by an application (NGNode is not publicly 
exported for example).


>> Good reminder about the implicit "public Shape()" constructor. Chien 
>> already had to add an explicit public no-arg
>> constructor in two classes. We really shouldn't rely on the implicit 
>> constructor in our public classes, since it makes
>> it easy to make such a mistake.
>
> It would be good to have a tool and/or automated test that warns about 
> this.  Another reason is that the implicit constructor has no javadocs 
> associated with it...

Indeed. I wonder if doclint will help (we are in the process of making 
our docs doclint clean).

-- Kevin


>
>             ...jim