1

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?

  • 2
    To be honest I think that without a proper context I can't say A or B. If your class is only utility and there is no abstraction, you could even go for static methods : no memory, no time to allocate, one less code line. – Walfrat Apr 10 '17 at 11:55
  • Strange that the answers don't focus on thread-safety (though some answers mentioned it among other things): with version B, splitter can be accessed from different threads, which might interfere with the chop method. – Bernhard Hiller Apr 11 '17 at 9:25
  • In C#, GeometrySplitter can be a struct, and you can have your cake and eat it too: no heap allocation, yet instantiated close to use. – Frank Hileman Apr 11 '17 at 16:49
10

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.

  • Method B might use fewer allocations. The Jitter could easily see that the object lifetime is strictly bound by the method body, and allocate it in the frame. – Caleth Apr 10 '17 at 13:14
  • 1
    Calling a method with arguments allocates memory as well. Allocating for the object is only different in that it happens on the heap. It's still fast. It just pointer addition. Please worry more about writing easy to understand code. If this ever becomes a real performance issue it'd be nice if we could read the code we have to optimize. – candied_orange Apr 10 '17 at 19:58
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.

2

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.

  • 1
    There's no need to inject the Unity Container into the instance, some people even consider it to be an anti-pattern (though feel free to disagree with Mr. Seemann). However the usual pattern is to let the container pass the dependency into the constructor (or maybe a property in C#), rather than making the container a dependency in itself. – Ben Cottrell Apr 10 '17 at 19:15
  • Interesting. Some of Mark's criticisms seem a little weak but I see his point. But if we do what he says-- inject a GeometrySplitter and not a resolver in the constructor-- we can never have more than one instance per instantiation of MyClass. To address OP's question we'd have to write an additional GeometrySplitterFactory, which doesn't seem too bad, but maybe a little too much for this example. – John Wu Apr 10 '17 at 19:27
  • 1
    @Ben - I've added a note in deference to your and Mark's criticism. Also, I may add a new question to stackexchange, and I invite you to chime in. – John Wu Apr 10 '17 at 19:34
  • @Ben please comment on my other question. – John Wu Apr 10 '17 at 23:06
  • 1
    Such a simple question; the answer should have nothing to do with additional complexity such as IoC. – Frank Hileman Apr 11 '17 at 16:47
1

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.

  • Given that the class is called GeometrySplitter and the way it is used in the given example, it can easily be assumed that it is used in some graphical editor or some geographic data processor, where this operation could be used a great number of times. This would particularly be the case in applications that handle GIS data. However, you do have a point in that if you are certain that the method would be used seldomly, there is no reason to extract the class outside of the scope of the method it is used in. – Vladimir Stokic Apr 10 '17 at 11:16
0

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

  • 1
    We do not know for sure if Chop affects the state of GeometrySplitter. We can only assume that. Also, we do not know if GeometrySplitter is a class inherited from some other class and whether Chop is a virtual method, overriden in GeometrySplitter, in which case it cannot be made static. Furthermore, GeometrySplitter can be a class coming from a 3rd party library which you cannot change, thus being forced to use it this way. If none of the above is the case, then your suggestion is absolutely valid. Otherwise, it is not correct. Please, update your answer to reflect this. – Vladimir Stokic Apr 10 '17 at 12:13
  • @VladimirStokic: If the chop-method did affect the state of the GeomerySplitter-instance, then the two examples provided would be functionally different. In that case I don't think the OP would have asked the question in the first place. – JacquesB Apr 10 '17 at 13:23
0

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.

-1

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.

  • 3
    Of course if you need the object to be available throughout MyClass, you should make it persist. But surely the question presupposes that it is only needed within one method. You shouldn't give variables a broader scope than necessary just in case they might be needed somewhere else in the future. – user82096 Apr 10 '17 at 10:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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