Chapter X, where I discover some nasty code, and get on my OO soapbox....

My idea was to have each subclass of NewTypeWizardPage pass in a template ID on construction, and then NewTypeWizardPage would pass the ID along to my new StubUtility.getTypeBody in a parameter.

So with new enthusiasm, I got the latest integ, hoping Martin would have committed my changes, and the source for the HEAD revisions of the packages Martin recommended above, and used the Synchronize function in the Team Synchronize view in eclipse.

When I surfed in the code however, not only were my changes not in there, but I noticed that somebody had introduced a member in NewTypeWizardPage called "fTypeKind" that identified which subclass was realizing the abstract NewTypeWizardPage. This member was accessed all over the place in NewTypeWizardPage, and was passed into the constructor.

This type of design is very frowned upon among software developers who understand OO technology. It causes a lot of maintenance headache when you want to introduce a new subclass. (You have to edit the superclass, and possibly some of the other subclasses. The whole idea is to rarely have to edit the superclass, or to even understand much of it beyond its interface.) I couldn't keep quiet about this. Not only did it make my job harder in getting the changes in, but it meant if I was to fit in with the existing design, I would have to add more of the same kind of badness, and contribute to the deterioration of the code. There was no way I was going to do this unless the committers told me explicitly that's what they wanted. So here I go on my soapbox...

- - - - - - - - - -

Martin,
Took in the latest integ, then the head revisions of the parts you listed. The
changes aren't in the head revision yet, so I re-applied my patch to my local
copy and started making the new changes. I have the new code templates done,
but haven't finished the changes to StubUtility and NewTypeWizardPage yet
because I didn't want to have merge problems with your changes.
It turns out there already are constants that identify the type in
NewTypeWizardPage, and they are placed in a member fTypeKind. It is referenced
17 times throughout the class in if branches and switch statments. This is
kind of shocking to me - I'm an OO purist and would always do overridden
methods in subclasses rather than subclass-aware logic in the parent class.
But, that would take quite a bit of cleaning up, so if you'd rather, I could
keep changes to a minimum by piggybacking on the switch statement currently in
constructTypeHeader (which I'll refactor back into constructTypeStub, the way
it was before I started) and assign the right template ID that way.
I could go either way, it's either doing some extra work to clean things up or
holding my nose and slipping in some quick-and-dirty code.
Let me know your preference, and tell me when you check in the past changes,
and I'll update, finish the new changes, and resubmit another patch.
Thanks,
- John

- - - - - - - - - - - - - - -

Martin's reply came back the next day.

- - - - - - - - - - - - - -

I see what you mean. It's all API, so changes are difficult and enums and
annotation are recent additions. I would introduce a protected method
'getTypeBodyContent', but make sure the default implemention (with the switch)
is in the NewTypeWizardPage so that no existing clients not knowing of the
change are broken.

- - - - - - - - - - - - - - - -

Yuck. So far, it seems that whenever there are bad programming practices in the code, they are always protected by being "API." This is why when you create an API, you need to be cautious about it in the first place so you don't get locked into a code base of deteriorating quality. I think there are is an alternative to throwing more bad code after the worse. You could deprecate the old nasty API, replace it with something better, and clean up the uses of it that you can get to. Besides, the "API" we're talking about is the constructor to an abstract class (NewTypeWizardPage) which really should be protected, and since the class is subclass-aware, you couldn't add another subclass anyway without altering the base class. This is a pretty weak API. So I stay on my soapbox and try one more time....

- - - - - - - - - - - -

Martin - to clarify, the "API" that relates to this is the NewTypeWizardPage
constructor - and any "clients" would be subclasses of NewTypeWizardPage. The
rest of it is hidden in NewTypeWizardPage implementation. Since the superclass
is aware of its subclasses, I don't see how you could have any client that
could be affected by this, except subclasses that inherit directly from
NewTypeWizardPage, but they would have to pass in one of the four valid "type
kind" parameters and pretend to be that type kind. I couldn't conceive of
there being other subclasses, but maybe that's just me.

My inclination would not be to create a new virtual method just to pass in a
string identifier, (though I would use virtual methods to clean a lot of the
other sub-type based logic out of NewTypeWizardPage, but that's a side issue).
I was thinking this is best solved by having the subclasses pass in the data
to a new version of the constructor, then the rest is encapsulated in
NewTypeWizardPage. The problem I have with that is the switch statement on the
types in the code that used to be in constructTypeStub that I broke out into
constructTypeHeader. I don't like that way of doing things, but I like less
doing what amounts to the exact same thing with two different designs in two
different places in the same class.

So, I believe my choices are to change the NewTypeWizardPage constructor and
the constructors in all the subclasses, and pass both the template ID and the
text that will be embedded in the code that way, (to my mind, this is the
clean solution) or to piggyback the template IDs into the same switch
statement that is currently there (to my mind, this is the quick-and-dirty
solution). If the constructor calling sequences can't change because they are
API, then the quick-and-dirty solution would be the one to go with.

- John

- - - - - - - - - - - -

Martin wrote back right away as usual, but unfortunately, I didn't win this argument either.

- - - - - - - - - - - -

The code template framework is currently JDT.UI internal. No other plugin can
contribute a code template and also can't access code templates by an ID.
That means using the code template ID in the constructor makes no sense for
other non-JDT-UI plugin.

What I would suggest is a protected method
getTypeBodyContent(ICmplilationUnit cu, String newTypeName, String linDelim)
that by default returns the type body template according to the current
fTypeKind, but could be overridden by clients like the NewTestSuitePage (in the
junit plugin) to return a different type body.

- - - - - - - - - - - -

Oh well. It is his code after all and he has to live with it. No matter what, I can't see the new protected method. The code already there has switch statements in it, and I don't see putting in a virtual method that switches on type right next to a switch statement that switches on an "fTypeKind" member. I'm just going to assign my template ID variable in that switch statement, and be done with it. Kind of like that part in "Alice's Resturaunt" where instead of picking up the one ton of garbage, they threw theirs down so there was two tons of garbage at the foot of the hill...

- - - - - - - - - - - -

OK - I'll leave the constructor alone and make the changes in the class as you
suggest. I haven't seen the last patch checked in when I synchronized. I can
accumulate all the changes (including the parameter to pass the type name as
you suggested earlier) and send a new patch either tonight or tomorrow. Since
I'm removing the original "newType" code template and replacing it with the 4
subclass templates, it'll save me some change merging.

Thanks,
- John

- - - - - - - - - - - -

Next - syncing, as in the Titanic, or, that syncing feeling....