Velocity Reviews

Velocity Reviews (http://www.velocityreviews.com/forums/index.php)
-   Java (http://www.velocityreviews.com/forums/f30-java.html)
-   -   Re: Automatic linking of related objects in constructor (http://www.velocityreviews.com/forums/t750659-re-automatic-linking-of-related-objects-in-constructor.html)

Eric Sosman 06-29-2011 12:29 PM

Re: Automatic linking of related objects in constructor
 
On 6/29/2011 5:56 AM, Qu0ll wrote:
> Suppose you have class A which contains a list of objects of class B and
> that the constructor for B takes in a reference to the A object which is
> to include it in said list. Is it safe to make a call to A's method
> addB(B b) in that B constructor passing in "this"? I have heard that
> it's bad practice to "leak" a reference to an object from within its own
> constructor because it may be in an invalid state.


I guess you mean

class B {
public B(A a) {
...
a.addB(this);
}
...
}

It's not a wonderful idea, but even if it's not safe it looks sort
of safe-ish, right? Ah, but code changes, and lo! one of the
changes introduces a subclass:

class C extends B {
public C(A a) {
super(a);
...
}
}

Okay, still not *too* bad -- or is it? Let's expand that last
set of ellipses:

class C extends B {
public C(A a) {
super(a);
if (phaseOfTheMoon() != Moon.FULL)
throw new IllegalStateException();
...
}
}

Yikes! Now if C's constructor throws up, the A instance is left
holding a reference to something that appears to be a C in B's
clothing, but is in fact nothing sensible at all!

> If not, how else can I automatically add the B object to the list in A
> without forcing the client programmer to explicitly call addB() given
> that they have already passed in the B as an argument?


(I assume you meant "A as an argument.") Consider using a
factory method:

class B {
private B(A a) {
// so clients can't do "new B(a)"
...
}
public static B instanceOf(A a) {
B b = new B(a);
// B's constructor is now completely finished
a.addB(b);
return b;
}
}

--
Eric Sosman
esosman@ieee-dot-org.invalid

Tom Anderson 06-30-2011 09:51 PM

Re: Automatic linking of related objects in constructor
 
On Wed, 29 Jun 2011, Eric Sosman wrote:

> On 6/29/2011 5:56 AM, Qu0ll wrote:
>
>> Suppose you have class A which contains a list of objects of class B
>> and that the constructor for B takes in a reference to the A object
>> which is to include it in said list. Is it safe to make a call to A's
>> method addB(B b) in that B constructor passing in "this"? I have heard
>> that it's bad practice to "leak" a reference to an object from within
>> its own constructor because it may be in an invalid state.


It can be safe, but it can also be unsafe, as Eric explained in the
passage i have so rudely snipped. It is widely considered that if
something can be unsafe, then it is unsafe.

That said, if you control A and B, and you are confident that nobody who
might work on A or B, or add subclasses or whatever, will do something
wrong, then it is safe. But that's a gamble. Or a tradeoff, as some people
call it.

>> If not, how else can I automatically add the B object to the list in A
>> without forcing the client programmer to explicitly call addB() given
>> that they have already passed in the B as an argument?

>
> (I assume you meant "A as an argument.") Consider using a
> factory method:
>
> class B {
> private B(A a) {
> // so clients can't do "new B(a)"
> ...
> }
> public static B instanceOf(A a) {
> B b = new B(a);
> // B's constructor is now completely finished
> a.addB(b);
> return b;
> }
> }


+1.

A slightly qualified +1.

Note that this prevents subclassing of B - the constructor is private. If
someone later wants to add a subclass, they will have to revisit this, and
will hopefully change it in a way which preserves the
b.getA().getBs().contains(b) invariant for the subclass. However, that's
another gamble - they might not. And once the constructor is protected
rather than private, the door is open to further subclasses which do not
preserve the invariant. This situation might actually be *less* safe than
handling the addition in the B constructor; you trade risk of exposing
incompletely constructed instances of B for risk of creating
inconsistently configured instances of A.

As supercalifragi... you know, i'm just going to call him Supes, said:

> But if B objects are supposed to all be tracked by an A, that suggests
> that B objects' lifecycles should be managed by A objects rather than
> directly by end-users


I don't really buy into any of his solutions, so i'll add another -
parental advisory, i haven't compiled any of this, so it might be wrong in
the details. We already know that:

class A {
private final Set<B> bs;
Set<B> getBs() {
return Collections.unmodifiableSet(bs);
}
}

class B {
private final A a;
A getA() {
return a;
}
}

And we would like to ensure that:

/* ∀ */ B b; assert b.getA().getBs().contains(b);

So i would suggest that:

class A /* again */ {
class Provider {
A getA() {
return A.this;
}
private Provider() {}
}
}

class B /* again */ {
static interface Constructor<T extends B> {
T newInstance(A.Provider aProvider);
}
static final Constructor<B> CONSTRUCTOR = new Constructor<B>() {
B newInstance(A.Provider aProvider) {
return new B(aProvider);
}
}
protected B(A.Provider aProvider) {
this.a = aProvider.getA();
}
}

class A /* for the last time */ {
<T extends B> T addB(B.Constructor<T> bCtor) {
T b = bCtor.newInstance(new Provider(this));
bs.add(b);
return b;
}
}

Which is then easily used by client code:

A a;
B b = a.addB(B.CONSTRUCTOR);

I would shoot on sight anyone who actually did this, but i think what i've
done here is made it impossible to construct an instance of B or a
subclass of B for which the invariant does not hold.

To illustrate this, if you want to write C, a subclass of B, you have to
do:

class C extends B {
public C(A.Provider aProvider) {
super(aProvider);
}
}

You are forced to do this because B constructor requires an A.Provider,
and since you can't create them, the only way to get one is as a
parameter. But since the only way to get one as a parameter is through
B.Constructor.newInstance, you have no choice but to provide some
plumbing:

class C extends B /* again */ {
static final Constructor<C> CONSTRUCTOR = new Constructor<C>() {
C newInstance(A.Provider aProvider) {
return new C(aProvider);
}
}
}

You could do that differently (with a named class or whatever), but if you
want to create instances of C, it has to boil down to something like that.

Usage is again:

A a;
C c = a.addB(C.CONSTRUCTOR);

Note that you could get round the static checking by passing a null to the
B constructor, but that will immediately blow up at runtime, when B's
constructor calls aProvider.getA(), so it doesn't actually let you create
anything incorrect.

Anyway, having written that Constructor implementation, the only code
which can call it is A.addB, because only that can create instances of
A.Provider. So it's all still safe.

The key moves are (i) the constructor of B won't let anyone create a B, or
a subclass of B, unless they can provide an instance of ProviderA, and
(ii) only A has the power to create instances of ProviderA. So, only
instances of A can create instances of B or subclasses of B, and when they
do so, they also add them to their set. The rest of it is just
set-dressing, MacGuffins, and matchmaking. A.Provider in particular is a
class that does absolutely nothing, but which serves as an unforgeable
token.

Now, A.Provider is unforgeable, but as written, they are reusable. A
villain could write:

class X extends B {
static final Constructor<X> CONSTRUCTOR = new Constructor<X>() {
X newInstance(A.Provider aProvider) {
return new X(aProvider);
}
}
public A.Provider muaHaHa;
X(A.Provider aProvider) {
super(aProvider);
this.muaHaHa = aProvider;
}
}

class Z extends B {
Z(X coConspirator) {
super(coConspirator.muaHaHa);
}
}

A a;
X x = a.addB(X.CONSTRUCTOR);
while (true) {
Z z = new Z(x);
assert !b.getA().getBs().contains(b); // YOUR HEAD A SPLODE
}

However, for the truly paranoid, this can be dealt with:

class A /* special guest appearance */ {
class Provider /* again */ {
private boolean used = false;
A getA() {
if (used) throw new IllegalStateException();
used = true;
return A.this;
}
private Provider() {}
}
}

I think that's now watertight. Bar cheating.

As i said, this is not actually a good idea - too clever for its own good,
and probably has some gaping vulnerability i haven't thought of - but it
does go to show what you can do with the combination of strong types and
access modifiers. You just try that in Ruby!

tom

--
The purpose of local government was to demonstrate, on a small scale,
what identical bastards the LibDems inevitably became if they ever came
close to power. -- Quercus

supercalifragilisticexpialadiamaticonormalizeringelimatisticantations 06-30-2011 10:23 PM

Re: Automatic linking of related objects in constructor
 
On 30/06/2011 5:51 PM, Tom Anderson wrote:
> As i said, this is not actually a good idea - too clever for its own
> good, and probably has some gaping vulnerability i haven't thought of -


1. setAccessible(true) followed by your choice of reflection dirty
tricks -- either on A, or B, or even the "unmodifiable" Set
returned by A.getBs().

2. public class C extends B implements Cloneable, Serializable

followed by clone, round-trip through ObjectFooStreams and a byte
array or disk file, etc. etc.

3. Native code hacks -- pass a B to a native method that then goes to
town on it with C pointer arithmetic and unsafe casts.

4. Assorted byte code hacking.

Of course 1 won't work in e.g. unsigned applets, nor 3, and 4 probably
won't pass the bytecode verifier in stock JVMs, though 4 combined with
gcj or Jet compilation to native code might work. 2 is the biggest hole
but you can implement clone and writeObject in B to throw exceptions to
plug it. Note that just copying the object by either method will break
the invariant, and serialization adds the ability to further hack the
serialized object while it's in the form of a defenseless byte array or
disk file.

If you want safety combined with serialization you need the B-has-a-C
strategy pattern approach, I suspect.


All times are GMT. The time now is 01:46 PM.

Powered by vBulletin®. Copyright ©2000 - 2014, vBulletin Solutions, Inc.
SEO by vBSEO ©2010, Crawlability, Inc.