Prev: Documentation hiding
Next: JFrame window dimensions
From: Robert Klemme on 11 Jul 2010 04:44
On 10.07.2010 19:56, Simon Brooke wrote:
> On Sat, 10 Jul 2010 08:15:52 -0700, markspace wrote:
>
>> Robert Klemme wrote:
>>
>>
>>> This is even better because you don't even need a cast:
>>
>>> final Class<? extends Authenticator> cl =
>>> Class.forName(className).asSubclass(Authenticator.class);
>>
>>
>> Interesting. Although "asSubclass(Class<?>)" is a cast and the
>> documentation even says "Casts this Class object to represent a subclass
>> of...." It will throw ClassCastException if the named class is not a
>> type of Authenticator, which should be caught, imo, because you probably
>> want to provide a better error mechanism that just halting.
>>
>> Definitely more than one way to do it, I agree.
>
> No, but that solution definitely looks interesting and tidy. I wasn't
> aware of the asSubclass() refinement, that's useful.
>
> The solution I've ended up with is as follows:
>
> 1 String authClassName = config
> 2 .getValueAsString( AUTHENTICATORCLASSMAGICTOKEN);
> 3
> 4 if (authClassName != null) {
> 5 try {
> 6 this.authenticator = Class.forName( authClassName).asSubclass(
> 7 Authenticator.class).newInstance();
> 8 } catch (ClassCastException cce) {
> 9 throw new InitialisationException(
> 10 "Specified class 'v' was not an authenticator".replace(
> 11 "v", authClassName), cce);
> 12 } catch (ClassNotFoundException cnfe) {
> 13 throw new InitialisationException(
> 14 "Specified class 'v' was not found".replace( "v",
> 15 authClassName), cnfe);
> 16 } catch (InstantiationException ie) {
> 17 throw new InitialisationException(
> 18 "Could not create an instance of " + authClassName, ie);
> 19 } catch (IllegalAccessException iae) {
> 20 throw new InitialisationException( "Operation not permitted",
> 21 iae);
> 22 }
> 23 }
> 24
> 25 authenticator.init( config);
>
> This still makes the code quite hard to read because there are so many
> different exceptions being thrown; while I agree with the general
> statement that one should only seek to catch the exceptions once expects
> to occur (because unexpected exceptions ought to give rise to some
> further escalation) given that all I'm going to do with any of these
> exceptions is log it, I can't help wishing they were all subclasses of
> some common 'WhileInstantiatingClassException' superclass which I could
> catch just once!

From the point of modularity I would definitively refactor the creation
code out into a separate method - even if it's just a private one. This
helps keep your other method much more readable.

Btw, what's the value of "authenticator" if the config does not contain
this entry? Is there some default value? In that case I'd structure
the code rather as

final String authClassName = config.getValueAsString(
AUTHENTICATORCLASSMAGICTOKEN);
authenticator = authClassName == null ? createDefaultAuth() :
createAuth(authClassName);
authenticator.init( config );

If this is in the constructor you can even make "authenticator" final
(if it is supposed to never change of course).

Kind regards

robert


--
remember.guy do |as, often| as.you_can - without end
http://blog.rubybestpractices.com/
 | 
Pages: 1
Prev: Documentation hiding
Next: JFrame window dimensions