Repeated Instantiation of the Same Class in a Method

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.

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.

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.

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).

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.

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.