Reputation: 715

Is an Initialize method a code smell?

I'm coding a bunch of systems right now. They do not derive from a common interface.

Some example systems: MusicSystem, PhysicsSystem, InputSystem, et cetera.

Currently, MusicSystem loads a lot of audio files in its constructor and as a result, there may be some brief lag when the object is first created.

Because of this, should this code loading all the audio files be placed in an Initialize() method instead? This allows the programmer to determine when he wants to load the audio files but then if he forgets to call Initialize() the program will crash.

Because not all systems need an Initialize() method the programmer has to look through every system to see if the class has an Initialize() method and if so, invoke it. This is a bit cumbersome.

Which approach is preferable in terms of general design principles?

Upvotes: 16

Answers (16)

Dranyar

Reputation: 610

Certainly not. According to MSDN's guidelines, a constructor should do minimal work. Therefore, such things as resource allocation or heavy object initialization would need to be placed somewhere else like Initialize().

https://msdn.microsoft.com/en-us/library/ms229060(v=vs.110).aspx

Constructors should not do much work other than capture the constructor parameters. The cost of any other processing should be delayed until required.

Upvotes: 0

Matt Fenwick

Reputation: 49085

Basically, you're trying to balance program efficiency against program-mer efficiency. So it's not necessarily a code smell in this case, depending on which is more important. Would you rather make the code slightly easier to use and harder to break, or have the program load faster?

However, an alternative you could try is lazy-loading.

Then, you can have your cake and eat it, too:

client code won't have to call Initialize
the data won't be loaded until/unless it is needed

Upvotes: 12

Erik Dietrich

Reputation: 6090

If you have a "MusicSystem" that makes no sense (and expresses this by blowing up) without initializing, I'm having trouble understanding the use case for instantiating this object but not initializing it.

If it's a question of passing it around from place to place, I might suggest having a look at Generic Lazy, where you could lazy load the object.

This way, you'd avoid the unnatural coupling of having to instantiate and then know immediately to call Initialize(), but you'd get the benefits of not having the expense until the object was actually used.

As for a code smell, I personally consider Initialize() methods to be a code smell. It's not always indicative that something this is wrong, of course, but it usually suggests to me that opportunities for dependency inversion are being missed. If I have to instantiate something and then call Initialize(), I'm wondering why I can't instantiate the object and then pass it whatever it needs in order to be considered initialized (or why it doesn't demand an initialized Foo in its constructor).

Upvotes: 0

Bevan

Reputation: 44307

Moving heavy initialization out of the constructor isn't a code smell.

However, relying on an external caller to invoke that initialization is a smell - it's called Temporal Coupling.

Create the Initialize() method on your class, but make it private (or protected if you must).

Also write an EnsureInitialized() method that triggers initialization if required, but only once per instance.

Then, each public entry point of your class should call EnsureInitialized() at the start - you initialization gets deferred to the point of first use.

With this in place, and provided you're comfortable with locks, you can spin off a call to EnsureInitalized() into a background thread to do the work in the background, with minimal impact on the foreground.

Upvotes: 13

Sergei B.

Reputation: 3227

You need to think about fragmentation of your *System's, because it looks like you have one super-master type which rules all the world and do most of the stuff in constructor. Which is not really good, looks smelly and ruins SOLID principle.

You could move out your long-running and IO operations into specialized wrappers which then you could pass as parameters, like Stream, Connection, IDataReader etc. in .Net. With this it will be more-less predictable if operation could consume lot of CPU, memory or IO throughput.

Upvotes: 1

Matthew

Reputation: 25763

I think a good idea would be to document that the specific class is "expensive" to instanciate. The programmer should then only instanciate the object when it's absolutely required to be used.

Upvotes: 2

Marcin Wieczorek

Reputation: 390

How about making initialized private/protected method, and calling initialize internally when any method which requires initialization is executed?

E.g.

public class MyClass
{
  private bool _isInitialized;

  public MyClass()
  {
    ... only basic initializations...
  }

  private void initialize()
  {
    if (_isInitialized)
      return;

    // initialize here
  }

  public void SimpleMethod()
  {
    // doesn't need to initialize
  }

  public void ComplexMethod()
  {
    initialize();

    // do something...
  }
}

Upvotes: 2

Skizz

Reputation: 71070

At first I was going to say that it wasn't a code smell since it allows the programmer to decide when to initialise an object, say, by doing all objects in parallel using worker threads.

But then I thought, it is a code smell because the choice of synchronous or asynchronous construction could easily be implemented in the constructor itself, with perhaps a constructor argument to prefer one over the other.

And then I thought, what about scenarios where it is not possible to throw exceptions? By setting up all the data in the constructor, there is no way to indicate a failure if there's no exception capability other than an accessor like IsInitialised or something. (Symbian is an example of an OS without support for exceptions.)

So I guess the smellyness is really dependent on the environment you're working in and your own personal preferences.

Upvotes: 1

Ferruccio

Reputation: 100658

I don't think having an initialization method is a code smell, but crashing (rather than failing gracefully) if the user fails to call your initialization method sure sounds like one.

If you make sure all your systems have an Initialize() method (even if it does nothing) then your users don't need to worry about whether or not to call it.

Upvotes: 5

Jim Mischel

Reputation: 133995

It's not a "code smell" at all. It's not exactly uncommon to have something like that. For example, look at the SqlConnection class in .NET. There's a default constructor that takes no parameters, and there's a constructor that takes a connection string. The one that takes the connection string is handy when you want to connect to the database and use the default values for connection timeout, etc. But if you want to change those properties, or if you want to have a connection instance ready but control more precisely when it's connected, you call the default constructor, set the properties, and then call Open when you're ready to do the connection.

Upvotes: 1

Matt Ball

Reputation: 359826

Think about other APIs which you have written code against. When was the last time that an API required the programmer to know to call an init method, otherwise crashing at runtime?

As a consumer of your API, it would drive me nuts if I had to know to call an init method after constructing an object. I would recommend an alternative that I have seen and used firsthand: document expensive object instantiation. What's the point of deferring an expensive initialization if it's required for the program to not crash?

Upvotes: 23

Dylan Smith

Reputation: 22245

Your constructor could kick off a background thread that does the load. Then anything in the class that needs to use that data just checks to see if the asynchronous loading has completed yet, and if not wait.

This makes class construction fast, it hides all the multithreading details from your class consumers, and it gets rid of the ugly init pattern.

Upvotes: 1

Valamas

Reputation: 24729

If the audio files are always the same. You could try loading them from a static property. Once an object loads the audio files. They will be available to all if the list of audio files is stored in a static property.

Upvotes: 1

Chris Trombley

Reputation: 2322

Few points here.

First of all, while writing an interface with an Initialize method is not necessarily wrong, it is generally considered bad practice to rely on the consumers of your service/API to configure some kind of state on their own before being able to use the features provided.

Second, I'm not sure what you mean by "some brief lag". If the problem is that your UI is losing responsivess when the loading occurs, a background task could be used to eliminate the lag. Look into BackgroundWorker for instance, or implement some other multi-threaded behavior to get where you want to be.

Upvotes: 4

wsanville

Reputation: 37516

Why not have an overloaded constructor, with a boolean to allow programmers to specify whether they want to go through the costly initialize method.

Then, you wouldn't have to check for the existence of a special method.

Upvotes: 1

ispiro

Reputation: 27673

Just an idea off the top of my head: add a bool to the constructor – whether the caller wants to initialize in the constructor.

Upvotes: 3

Is an Initialize method a code smell?

Answers (16)

Related Questions