current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite

Generally, I structure small threadsafe immutable objects like this:

public class SmallObject {
    private final String state;
    public SmallObject(final String state) {
        this.state = state;
    }

    // ...
}

And then wire these up in Spring like this:

<bean name="SmallObjectForThisThing" class="my.package.SmallObject">
    <constructor-arg name="state" value="in practice this is usually a ref"/>
</bean>

However, this leads to complications with circular dependencies. To keep the immutability when this happens, I use a "freeze" pattern, where the variables are set once. This is what I want reviewed:

public class SmallObject {
    private String state = null;

    public void setState(String state) {
        if (this.state != null) {
            throw new IllegalStateException("state already set: '" + state + "'.");
        }
        this.state = state;
    }

    private void ensureInitialized() {
        if (this.state == null) {
            throw new IllegalStateException(
                "state must be set before this instance is used."
            );
        }
    }

    // ... For every additional method on the object, I call 
    // ensureInitialized() first.
}

And then wire them up like this:

<bean name="SmallObjectForThisThing" class="my.package.SmallObject">
    <property name="state" value="in practice this is usually a ref"/>
</bean>
share|improve this question
2  
I do not understand the purpose, an opinion about this solution depends heavily on the circumstances. This approach will generate a lot of problems if it comes to threadsafety. And it is not really immutable. Perhaps, it could be a better way to invest some time to solve the "circular dependencies" problem. –  tb- Jan 8 '13 at 11:36

1 Answer 1

up vote 7 down vote accepted

This class should instead use an AtomicReference to ensure the state is kept valid. Alternatively, you should incorporate thread-safe handling of the String.

Consider:

private final AtomicReference<String> stateref = new AtomicReference<String>();

public void setState(final String state) {
    // only one initializer will succeed (assuming state is not null)...
    if (!stateref.compareAndSet(null, state)) {
        throw new IllegalStateException("state already set: '" + state + "'.");
    }
}

private void ensureInitialized() {
    if (stateref.get() == null) {
        throw new IllegalStateException(
            "state must be set before this instance is used."
        );
    }
}

This pattern ensures usage is consistent, there can be only one initialization of the instance, and that any thread-unsafe practices are handled well.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.