Paul ([info]subtlety) wrote,
@ 2003-08-13 15:53:00
Previous Entry  Add to memories!  Tell a Friend!  Next Entry
Here's a neat trick for factory/ classes. Say you have a class, ConnectionManager, that you want to be the only guy to instantiate a Connection class. Sure, sure, you can make the constructor package-private, but I'm not that trusting a guy. So...

public class Configuration 
{
	/**  A constructor that ensures only ConfigurationManager can create configurations */
	Configuration( ConfigurationManager.ConstructionParameters construct )
	{
		this.name = construct.name;
	}
}

public final class ConfigurationManager
{
	/**  An small helper class that ensures only ConfigurationManager can instantiate Configuration */
	static class ConstructionParameters
	{
		final String name;
		private ConstructionParameters( String n ) { name = n; }
	}

}


Okay, not that impressive. But now I know no-one is creating those Configurations except for my manager, even within this package.

(Post a new comment)


[info]daniele
2003-08-14 01:09 am UTC (link)
I sorta replied. In my journal. ;)

(Reply to this)


[info]danshep
2003-08-14 06:08 am UTC (link)
Still doesn't stop me if I have reflection permission. 

import java.lang.reflect.Constructor;

public class ConfigurationHacker {

    public static Configuration hackedConfiguration(String name) {
        try {
            Constructor constr = ConfigurationManager.ConstructionParameters.class.getDeclaredConstructor(new Class[] {String.class});
            constr.setAccessible(true);
            ConfigurationManager.ConstructionParameters params =
                (ConfigurationManager.ConstructionParameters) constr.newInstance(
                    new Object[] { name });
            return new Configuration(params);
        } catch (Exception e) {
            e.printStackTrace();
            return null;
        }
    }

    public static void main(String[] args) {
        System.out.println(hackedConfiguration("blah").name);
    }
}


I think you're just obscuring your code for no benefit at all. Just make the constructor package private and save yourself the effort.

And while I'm being criticising, why have a parameter object that's populated via constructor? Why not have it no-args and give it set methods.

If you want to be really evil about who calls your constructor/method, you would be best served by using something like:

String callingClass = new Throwable().getStackTrace()[1].getClassName();
if(!"com.blah.bleh.Bler".equals(callingClass)) {
  throw new Error("Gotcha");
}


Though that is 1.4 specific... you can think of it as an extra level of security =)

http://freeroller.net/page/soxbox

(Reply to this) (Thread)

[info]subtlety
2003-08-14 07:19 am UTC (link)
Reflection is fun, isn't it? I wasn't worried about someone deliberately hacking my code, I was worried about a future maintainer creating a Configuration (from within this package) and violating one of my underlying design assumptions (which is that the lifecycle of a Configuration is completely and always managed by a ConfigurationManager - out of necessity, btw, not just for the joy of it. ;)

But I think if we're trying to produce bullet-proof code, your last example isn't even enough. With a seperate disconnected classloader I can have a class called "com.blah.bleh.Bler" that isn't your Bler and have it happily create all my Connections for me. I think you'd have to have a sealed jar with a custom security manager that prevented package definitions once your classes are originally loaded. It'd be an interesting experiment, really...

Oh, I use the parameter object populated by constructor because:

- It produces a nice syntax for creating it and passing it in a single line
- I can then declare the parameters 'final' and not *have* to provide accessors for them while still maintaining invarience.

Re: Obscuring for no benefit at all. Perhaps you're right. Originally, the constructor was package private. But then I started wondering if, in the course of my refactoring, I'd left any calls to that constructor. (I guess if I'd had a little more coffee I just would have hit alt-shift-f7 and found out). But then I started thinking about how amazingly important it was that Configurations didn't exist outside of their ConfigurationManager's - I can't stress this enough - I didn't provide the context, but there are background threads and sockets involved such that I want to ensure uniqueness for my Configurations, even if they get collected & re-created at a later date. A hashmap with weak references works well, but I need to ensure I add every single Connection to it or it will be hell to debug later.

Although, I suppose, I could assert that my creating class name is the configuration manager in the constructor using the technique you mentioned. I had something like that in a previous project (except before 1.4, so printStackTracing' the exception into memory and parsing it - you know how it's done, I'm sure), but when I profiled it 60% of my time was being spent taking that stack trace! So I avoid that now, basically - even when it's just in debug (assertions enabled) builds.

(Reply to this) (Parent)(Thread)

[info]danshep
2003-08-14 07:41 am UTC (link)
I got halfway through the response and realised that was where you probably coming from.

I'm sure there's a better way of doing this than cluttering up your code though. It just looks so damn ugly.

If ConfigurationManager's control over Configuration is so important, why isn't Configuration an inner class of ConfigurationManager? Put it in there with a private constructor, and you've got just the assurances that you're after without so much horridness.

(If you'd like to make it a nice safe refactoring, turn the existing Configuration object into an interface and have the inner class implement it, so that you don't have to change your references all over the rest of the code).

(Reply to this) (Parent)(Thread)

[info]subtlety
2003-08-14 04:21 pm UTC (link)
Shoot. The interface/inner-class thing is a good idea. Why didn't I think of that?

(Reply to this) (Parent)(Thread)

[info]subtlety
2003-08-14 04:39 pm UTC (link)
I went to go do this, but it was bothering me that I'd have to pick up all the code inside Configuration.java and put it into ConfigurationManager.java. It seems like it would just make the two classes harder to read.

But I think I can adapt the technique by leaving the package-private constructor in Configuration and flagging Configuration abstract.

Then, inside ConfigurationManager, I can:

static class CreatableConfiguration extends Configuration {}


Which I can create as normal. But this doesn't stop someone else from deriving from Configuration somewhere else in the package. But I think that they aren't going to do that accidently...

(Reply to this) (Parent)(Thread)

[info]subtlety
2003-08-14 04:51 pm UTC (link)
Heh. Just did it. The line inside ConfigurationManager even neater than that:

c = new Configuration( configurationName ) {};


Thanks for the prod, danshep, I think this way is cleaner.

(Reply to this) (Parent)

[info]danshep
2003-08-14 11:53 pm UTC (link)
Because complex solutions are more likely to make you sit there and go "I'm a genius", and it's very hard to convince yourself that there's a simpler solution after hitting that point.

(I'm currently in the process of maintaining code I wrote over a year ago, and finding my various 'strokes of genius' make me more and more embarrassed every day).

(Reply to this) (Parent)

[info]charles
2003-08-14 08:30 am UTC (link)

I think you're solving a problem that shouldn't exist. Leaving aside the question of whether [Foo]Manager classes are a code-smell in themselves, I'd solve it thus:

public class Configuration {
    private final String name;

    /**
     * <b>NOBODY should call this constructor except the
     * ConfigurationManager</b>
     */
    Configuration(String name) {
        this.name = name;
    }

    // blah...
}

Anything more than this is obfuscation. While it's tempting to make your code "perfect" - impossible for anyone to misuse, you must admit that misuse is always possible, as demonstrated in other comments, and put the limit at some convenient point that the culprit "should know better". Package-level access is a good place to put that level.

As a general rule, the only safe thing to do with a class you do not understand, is call the public interface. It's a question of how tightly-coupled you want to be to the class you are calling, and you should never become tightly-coupled to something you do not grok. If something is declared protected, it means "You must understand precisely how the superclass works if you want to do this." If something is declared package-private, it means "You must understand the way this package works if you want to do this.

(Reply to this) (Thread)

[info]subtlety
2003-08-14 04:47 pm UTC (link)
Well, I know what you're saying. But I often "half understand" classes and use them anyway. It works for me as long as the classes are pretty well designed and aren't subject to out of the ordinary rules.

And I often call methods without reading their javadoc. Given it's just a ctrl-t away, perhaps that's too lazy. :)

My real issue is I want to decouple classes *within* the same package as much as possible, but I'm too lazy to create interfaces for each and every one of them and put them in their own little packages. Well, maybe not too lazy - it just seems to be too much code for too little benefit.

Which is what you're saying with the comment thing, right? Well, I guess I consider 3 lines of code that makes it hard to compile something that accidently does the wrong thing better than 3 lines of comments that tells you not to do the same thing.

That's a philosophy open to interpretation, I guess. Comments should be used to explain, the design of your classes should be used to constrain. It seems that, taking this to it's logical conclusion, if you have a class library with lots of comments like "don't call this before that", and "don't do X unless you're Y", what you have is a badly designed class library that's going to be hard to program against. One rule doesn't make it hard, but a hundred of them will. That's just too much documentation.

(Reply to this) (Parent)

Create an Account
Forgot your login or password?
Login w/ OpenID
English • Español • Deutsch • Русский…