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

[Jim Graham]

Sounds good...

			...jim

On 5/20/16 3:56 PM, Kevin Rushforth wrote:
>
>
> 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