cxf-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Glynn, Eoghan" <eoghan.gl...@iona.com>
Subject RE: Checkstyle
Date Wed, 04 Apr 2007 15:35:55 GMT
 

> -----Original Message-----
> From: Polar Humenn [mailto:phumenn@iona.com] 
> Sent: 04 April 2007 14:57
> To: cxf-dev@incubator.apache.org
> Subject: Re: Checkstyle
> 
> I would firmly have to disagree.
> 
> You have lots of generated code that are classes, don't use 
> interfaces, which people have to use to use the API. But I digress.


What generated code exactly is in the form of classes (that the user has
to extend)?

At least for JAX-WS, the main bit of generated code exposed to users is
the SEI, where the "I" stands for interface.

Clearly I'm not saying don't use classes. You can't program any
executable logic into interfaces. 

What I'm saying is, if the user is to extend or implement some of our
code, give them the option to implement an interface. What's so
controversial or disagreeable about that?

 
> If I am to use a *class* like:
> 
>        final class UserPass {
>            final String user;
>            final String pass;
>             UserPass(String u, String p) {
>                    user = u; pass = p;
>            }
>            String getUser() { return user; }
>            String getPass() { return pass; }
>       }
> 
> I have a contract with the an operation
> 
>           UserPass getUserPass();
> 
> That what I get back won't change. It wont deliver different 
> things for subsequent calls to getUser or getPass.


So keep UserPass as a class! 

The point is that the application code wouldn't need to extend UserPass
anyway.

What they do need to extend is the HttpBasicAuthCredsSupplier. So that
should be an interface. And also possibly an abstract class implementing
it, if it contains any non-trivial bolier-plate code. So that the user
has the choice to extend or implement.

So assuming HttpBasicAuthCredsSupplier is an interface, then UserPass
could be either an inner interface (if want to keep the inner/outter
scoping), or a stand-alone class (if you want to control the
implementation). Take your choice.


> This allows me to make certain assumptions on the calling 
> side that I cannot make if UserPass was forced to be an 
> interface. Such as I can cache the object instead of the 
> results. And I have some assurance that there are no convert 
> channels from it.

Covert channels?
 
> Sure, you can put a certain Abstract logic built "immutable" 
> class underneath it, but it doesn't *mean* the same thing. I 
> can't *guarantee* immutability from the calling side, if all 
> I'm requiring it be is an interface.


No-one is requiring UserPass to be an interface. As the user would never
have to extend it, it doesn't really matter.

But you can easily get the immutablility you want even if UserPass is an
inner interface (assuming the inner/outter scoping was more important to
you). As Java strings are themselves immutable, just extract the
username & passwd fields from the returned value and re-wrap in a
trivial immutable UserPass impl. 

On the other hand, if you want to dictate the actual UserPass impl, then
make it an outter class.

No rocket science. And not worth the time that's already been burned on
this discussion either.


> Nope, I don't buy the argument.
> 
> The other thing, I can alleviate memory leaks somewhat 
> because if it were an interface and somebody decided to give 
> me an implementation of it as an inner class and I cached the 
> object I end up keeping a reference to the whole outer class.


A *static* inner class (like the UserPass in your patch) has no
reference to the outter class.

Only a *non-static* inner class carries a reference to an instance of
the outter class.

/Eoghan


> The basic upshot is that you can implement it any number of 6 
> different ways to Sunday but the "contracts" *are* different, 
> subtle as they may be.
> 
> Cheers,
> -Polar
> 
> Glynn, Eoghan wrote:
> > Well the mumblings were more like advice to use interfaces *in 
> > addition
> > to* abstract classes, not *instead of* abstract classes. 
> >
> > Plus some pointers to Java features/idiom like @Override, inner 
> > interfaces, composition v. inheritance, that would solve 
> some of the 
> > problems Polar perceives.
> >
> > However there *is* a compelling reason for casting public APIs in 
> > terms of interfaces, as opposed to the corresponding 
> abstract classes. 
> > We generally do not want to *force* users to extend our abstract 
> > classes and use up their one shot at implementation inheritance in 
> > doing so, just to get some boiler-plate code.
> >
> > Example would be Polar's HttpBasicAuthSupplier. That should be an 
> > interface, possibly also implemented by an abstract class 
> holding the 
> > boiler-plate logic. A user may want to directly implement the 
> > interface themselves if they needed to extend a different 
> base class 
> > for some reason, e.g. DialogBoxHttpBasicAuthSupplier 
> extends GUIWidget 
> > implements HttpBasicAuthSupplier.
> >
> > So the abstract base class is in my view a convenience that 
> the user 
> > may choose to take advantage of, or not, as the case may 
> be. But they 
> > should not be forced to do so. So AbstractWSFeature is 
> grand, as long 
> > there's also a WSFeature interface that the user can choose to 
> > implement directly.
> >
> > Sure, users who make that choice would be impacted more by future 
> > additions to the interface. But in reality, not all new 
> methods would 
> > have a sensible default impl that extensions of the 
> abstract class can 
> > just pick up without any change.
> >
> > As far as the Abstract* naming convention is concerned, I'm 
> not a big 
> > fan of such rules when forced to change my own class names, 
> but once 
> > you get over that irritation I can see the value of the consistency 
> > that the rule brings. So I'm not pushed either way.
> >
> > Hey, it could be a lot worse ... at least we don't have the 
> C# IFooBar 
> > naming scheme for interfaces :)
> >
> > Cheers,
> > Eoghan
> >
> >
> >   
> >> -----Original Message-----
> >> From: Dan Diephouse [mailto:dan@envoisolutions.com]
> >> Sent: 04 April 2007 00:50
> >> To: cxf-dev@incubator.apache.org
> >> Subject: Re: Checkstyle
> >>
> >> I completely agree that we should get rid of this rule.
> >>
> >> First of all, Aegis from XFire use Type which is abstract, and I 
> >> don't want to change this for migration reasons which is 
> one of the 
> >> reasons pmd is disabled for this module.
> >>
> >> Second, I agree that the Abstract/base/factory naming is 
> incredibly 
> >> awkward.
> >> I hear some mumblings of "just use interfaces" - but 
> abstract classes 
> >> provide a much more robust way to add features in the 
> future. With an 
> >> interface, if you add a method, it will break every class that 
> >> implemented that interface. With abstract classes if you add a new 
> >> method, all you need to do is provide a default 
> implementation of it 
> >> and everything will work swell in the future.  So I tend to use 
> >> abstract classes more and more to avoid future incompatibilities 
> >> (AbstractWSFeature, AbstractServiceFactoryBean, 
> >> AbstractEndpointFactory). With at least the first two, I 
> would like 
> >> to get ride of the Abstract.
> >>
> >> - Dan
> >>
> >> On 4/3/07, Polar Humenn <phumenn@iona.com> wrote:
> >>     
> >>> Is there a "good" motivation of why "abstract" classes have to be 
> >>> named "Abstract" "Base" or "Factory"?
> >>>
> >>> If a class is declared "abstract", I want the *compiler* to
> >>>       
> >> tell the
> >>     
> >>> developer that s/he has not filled out a particular
> >>>       
> >> functionality when
> >>     
> >>> s/he extends the abstract class. Not that I want it named
> >>>       
> >> "Abstract".
> >>     
> >>> For example,
> >>>
> >>> abstract class Muffin {
> >>>      ......
> >>>      abstract Kind kind();
> >>> }
> >>>
> >>> I really don't want my code littered with method definitions like:
> >>>
> >>>       void eat(AbstractMuffin muff) {
> >>>
> >>> I want it to be:
> >>>
> >>>       void eat(Muffin muff);
> >>>
> >>> because that's what it is. It's not an AbstractMuffin, it's
> >>>       
> >> a Muffin!
> >>     
> >>> Can we get rid of that particular checkstyle rule?
> >>>
> >>> I say that, because it forces, either
> >>>
> >>> a) illogical names, like AbstractMuffin, to be used in 
> definitions, 
> >>> making for
> >>>     awkwardness. (i.e. eat(AbstractMuffin muff);
> >>>
> >>> b) default implementations, just to avoid the illogical names!
> >>>
> >>>       This particular avoidance causes errors to be caught
> >>>       
> >> at run time
> >>     
> >>>       instead of compile time, where it should be caught! And 
> >>> sometimes causing
> >>>       a loss of time to find it.
> >>>
> >>> For example, with the current checkstyle rule I could be 
> forced to 
> >>> write the class with a default implementation expecting it to be 
> >>> overridden. (Except there is no way to tell a compiler that).
> >>>
> >>>      class Muffin {
> >>>            .....
> >>>            Kind kind() {
> >>>                  return Kind.BLUEBERRY;
> >>>           }
> >>>       }
> >>>
> >>>       void eat(Muffin muff) {
> >>>         System.out.println("I'm eating a " + muff.kind() +
> >>>       
> >> " muffin!");
> >>     
> >>>      }
> >>>
> >>> and a developer goes ahead and writes:
> >>>
> >>>        class CornMuffin extends Muffin {
> >>>             Kind kiend() {
> >>>                   return Kind.CORN;
> >>>             }
> >>>        }
> >>>
> >>> and it compiles fine without problems. Subsequently he 
> can't figure 
> >>> out why his application still says he has a BLUEBERRY muffin, 
> >>> especially when he has used "eat(new CornMuffin())".
> >>>
> >>> This kind of pattern complete adverted the use of compiler
> >>>       
> >> protections.
> >>     
> >>> Cheers,
> >>> -Polar
> >>>
> >>>
> >>>
> >>>       
> >> --
> >> Dan Diephouse
> >> Envoi Solutions
> >> http://envoisolutions.com | http://netzooid.com/blog
> >>
> >>     
> 
> 

Mime
View raw message