Repeated Instantiation of the Same Class in a Method

Question

This is sort of a follow up question on Multiple Same Object Instantiation. And I think, is not really language specific, so this is applicable to Java and C#?

Version A

public MyClass {
    public void methodX() {}

    public void methodY(int i, object o) {
        GeometrySplitter splitter = new GeomterySplitter(int i, object o);
        splitter.chop();
    }

    public void methodZ() {}
}

Version B

public MyClass {
    private GeometrySplitter splitter = new GeometrySplitter();

    public void methodX() {}

    public void methodY(int i, object o) {
        splitter.chop(int i, object o);
    }

    public void methodZ() {}
}

A colleague says that Version B is a better code practice, because the class is only instantiated once. My own reasoning is that the internal variable is only used in methodY(int i, object o) and thus it should be created within the method itself, as displayed in Version A.

My colleague then reasons that if that method is called 4000 times a second. For every method call an new instance of that class is created. And this is bad for performance and memory use.

Is this true? If so or not, can someone clarify this?

Answer 1

Prefer version A unless you have a concrete reason to worry about the effect of many invocations of the method.

There is a general principle "variables should be declared as close as possible to where they are used", which should be followed unless you have a good reason not to. Version A obeys this principle, and has several benefits:

• You can see the object declaration right where you are using it. In version B, once you go beyond a trivial example to real code, the declaration and instantiation of the object may be far from its use. This makes the code less clear.
• Limiting the scope limits opportunities for error. In version A, the object only exists right when you are using it. In version B, it is hanging around and accessible to other methods. It's possible that this could lead to a bug. For example, if you have to declare several objects like this for several different methods, you might accidentally use the wrong one somewhere.
• Memory is only being held while you use the object. Version A may cause more memory allocations, and that may be harmful in the context of languages with Java-like garbage collection. But it's worth noting that version A actually ties up less memory total in the longer term, because the object only exists while it is actually needed (and not at all if that method doesn't get called). It's not true that one version is definitely preferable in terms of memory.

For this reason I would prefer A (without knowing anything else about how this code is used).

It's true that B is beneficial in specific cases. As your friend notes, if the method is invoked many times, version B would use memory more effectively (and it would also avoid repeatedly incurring the overhead of instatiating the object).

But worrying about this when you don't actually know you have a memory or performance problem is a classic case of premature optimisation. And there are actually cases where version A performs better. For example, what if your code involves creating thousands of MyClass objects, but MethodY is rarely used? In this case, A may be the better design in terms of memory.

Answer 2

Assuming that GeometrySplitter doesn't have any state, then Version A is the cleaner, more easy-to-read option, as it minimises the lifetime of the object to when it is actually needed.

However, you say that this function is called 4000 times per second, and your colleague is concerned about performance. So analyse that performance. Set up a test harness, and run each version in turn with 4000 hits per second over a decent time span. Examine memory usage and measure exactly how they respond.

Without knowing how long it takes to create a GeometrySplitter, it's hard to know whether creating 4000 of them per second is going to tax the JVM.

Answer 3

Member scope doesn't solve the problem fully

Even if you declare GeometrySplitter as a class member, it will still get instantiated per instance of MyClass, and in addition, will need to be instantiated separately for any other class that uses it. So if you're worried about the overhead of constructing a GeometrySplitter, moving it out of local scope doesn't fully solve the problem.

Use IoC to get around all this

Under IoC, object creation is considered a separate concern, and making MyClass worry about how to instantiate something is a violation of SRP. These problems shouldn't need to be solved by MyClass.

If you use an IoC container, it eliminates the problem, and also might save you some instantiation overhead not just between different calls to MyClass::methodY but also between different calls to any method in any class that uses the splitter.

For example:

public MyClass {

    protected readonly IUnityContainer _container;

    public MyClass(IUnityContainer container) { 
        _container = container; 
    }

    public void methodY(int i, object o) {
        IGeometrySplitter splitter = _container.Resolve<IGeometrySplitter>();
        splitter.chop();
    }
}

Instancing rules belong in the composition root

If you want a new instance each time, set up your composition root this way:

container.RegisterType<IGeometrySplitter, GeometrySplitter>();

If you want one single (non-thread-safe) instance that is re-used:

container.RegisterType<IGeometrySplitter, GeometrySplitter>(new PerThreadLifetimeManager());

If you want one single (thread-safe) instance that is re-used:

container.RegisterType<IGeometrySplitter, GeometrySplitter>(new ContainerControlledLifetimeManager());

Then, register MyClass in the container as well, so that it will inject itself into the instantiation process:

container.RegisterType<IUnityContainer>(container);
container.RegisterType<MyClass>();

When you do it like this, Unity will automatically inject itself as the constructor argument to MyClass so that methodY can call it.

To instantiate MyClass, use:

var myClass = container.Resolve<MyClass>();

Notes

• My example above uses Unity, which is a c# technology. In Java I believe you'd use Spring instead (not quite sure). But the principle is language-agnostic and the techniques to control object lifespan should be similar.

• While this pattern isn't too uncommon, some folks would say that it's an anti-pattern. They'd say you'd have to inject a specific GeometrySplitterFactory instead of the IoC container itself, and implement instantiation rules in the factory. But the principle is the same: take the instantiation rules out of MyClass.

Answer 4

There is no general rule to this. It depends on the use case here.

If the method is used seldomly, there is no reason for moving the GeometrySplitter class outside of the scope of MethodY.

However, memory allocation is fairly expensive operation and should be used cautiously. If GeometrySplitter class does not have any properties that depend on the input parameters of MethodY, and it does not look like it does, then the second approach is better if MethodY is called often. You do not lose time to allocate memory, do not fragment memory, and do not burden the garbage collector later on to release all the unnecessary references.

The third option would be to pass an instance of that class, instantiated elsewhere, as an argument of MethodY. That way, you get the best of both worlds. You instantiate GeometrySplitter once, and simply use it where it is needed. This approach also clearly denotes the dependency of MyClass.MethodY on GeometrySplitter class, which is not clear in the previous approaches without looking at the body of the method.

Answer 5

Assuming chop does not affect the state of the GeometrySplitter instance, then A is preferable to B, but even better would be changing chop to a static method.

This leads to code which is simpler than either option:

public MyClass {
    public void methodX() {}

    public void methodY(int i, object o) {
        GeometrySplitter.chop(i, o);
    }

    public void methodZ() {}
}

(I would assume chop does not affect the state of GeometrySplitter because otherwise the two options you present would be functionally different, and one of them would not work correctly.)

More generally, you always want to reduce the amount of mutable state which may influence any section of code. All other things being equal, we want to limit mutable state to the tightest scope possible. This means we prefer static side-effect free methods to instance methods, and if that is not possible, prefer instances with the scope of a single method (option A) to instances with class scope (option B).

Answer 6

Creating new objects over and over again takes time. The change to avoid this is trivial. So you measure how much time you will safe, and when it is significant, you make the trivial change. If you don't measure, then obviously the gain is not significant.

AND one more thing: If your application is multi-threaded, you better make absolutely sure that your "splitter" is thread safe, that is that it will work correctly if the same splitter object is used from multiple threads, otherwise you are creating a time bomb for yourself.

Answer 7

You're both right and (potentially) wrong.

You're right if the business case really says that you don't have to call the method 4000 times. Then it's ok.

Your colleague is right as that is really a better code practice, not only for the reasons your colleague mentioned, but because this object is available throughout MyClass. This promotes smaller and more specified classes (like MyClass) and encapsulation (it hides the implementation and functioning of GeometrySplitter to outside world).

Maybe today you need GeometrySplitter only in that method, but in the future you could get another business case where you'll have to construct another method and instantiate it again and then you'll have more refactoring to do. The way you promote the object creation opens it to so called "God" classes and a lot of unnecesarry refactoring.