Should all my arguments be in the constructor?

Question

I have a class StreamCopyOperation which provides me such things like the average speed of the copy operation and other informations.

Now I have a constructor which looks like

public StreamCopyOperation(Stream source, Stream target, int bufferSize, int updateInterval)
{
    //Initialize values
}

and a method

public void CopyStream()
{
    //Copy the streams, send the progress updates, etc...
}

Now I don't know if all the arguments should be in the constructor or the streams should be passed in the method like this:

public void CopyStream(Stream source, Stream target)
{
    //Copy the streams, send the progress updates, etc...
}

and the constructor gets only the buffer size and the update interval passed. Or maybe everything should be in the CopyStream method.

Is there something like a best practice or is this just a design decision?

Maybe something like the constructor takes in the source and CopyStream takes in the destination? — Corey Ogburn, Aug 24 '11 at 17:25
I wouldn't have a class named StreamCopyOperation, the name itself tells you that this should be a Method, not a class. You can have a utility class that has a.method that copies one stream onto another. — Icarus, Aug 24 '11 at 17:30

David Hoerster · Accepted Answer · 2011-08-24T18:05:44.947

I think it's a design decision based upon how you expect the class to be used.

If it's a use-once type of class, then maybe all arguments should be passed into the constructor and then you set any other properties and then call CopyStream (with no arguments).

But, if you expect the stream parameters to change, then don't pass them into the constructor and have the values be passed into the CopyStream method.

Lastly, if it really is more of a use-once type of class, then maybe you should consider the class being a static class and CopyStream being static -- saves you a line of code and makes the class more of a helper type of class.

Hope this helps!

Agreed that it all depends on the usage and lifetime of the operation object. Well said! — Reddog, Aug 24 '11 at 17:34

score 2 · Answer 2 · answered Aug 24 '11 at 17:32

In my opinion, that would depend upon the lifetime of the StreamCopyOperation object...

In particular, since stream objects are usually (always?) disposable and bound to some system resource, I would want to keep them around for as little time as possible so I would think about taking the parameterized approach.

If the StreamCopyOperation is bound only to the lifetime of the streams themselves then the first method is appropriate.

But if you want to keep the operation object around longer (for example, because it's connected to UI) then I would go with the parameterized function approach and into more of a static "helper class".

+1 for consideration of the stream being IDisposable. Didn't consider that. — David Hoerster, Aug 24 '11 at 18:07

score 1 · Answer 3 · answered Aug 24 '11 at 17:30

It really depends how you expect CopyStream to be used. Is it something which will be used frequently? That would suggest using method arguments for the CopyStream method. If it's designed to be a one-time call, then use the constructor arguments when you spin up a new instance of your type.

score 0 · Answer 4 · answered Aug 24 '11 at 17:28

If you will use those Stream in class scope such as other methods, it's better to pass them as constructor argument and assign them to a variable which declared in class's scope, otherwise no need to pass them to constructor and recommend to use second CopyStream method.

hope this help.

score 0 · Answer 5 · answered Aug 24 '11 at 17:28

I would pretty much consider it a design decision.

If a single instance of CopyStreamOperator isn't reused for multiple sources and/or targets, I'd probably pass all the required values in the constructor to create an object that knows everything it needs to know. The input values can then be validated to ensure that your operator never is in an invalid state.

Then rename the parameterless CopyStream() method to Run() or Execute().

score 0 · Answer 6 · answered Aug 24 '11 at 17:30

I would consider using Default values for not so essential details and providing Properties in the Class for those details to set or get their values:

private const int DEF_BUFFER = 100;
private const int DEF_INTERVAL = 10;

public StreamCopyOperation(Stream source, Stream target)
{
   //Initialize values
   this.BufferSize = DEF_BUFFER;
   this.UpdateInterval = DEF_INTERVAL;
} 

public int BufferSize { get; set;} // or use a private member inside, if needed

public int UpdateInterval { get; set;} // or use a private member inside, if needed

Allon Guralnek · Answer 7 · 2011-08-24T17:44:36.873

My design guideline is to only require the user to provide the constructor what is absolutely necessary in order to construct the instance. Maybe the user doesn't want progress updates. Maybe the user doesn't yet have both streams when he wants to construct the instance and hookup the progress event.

Of course you could provide convenient constructor overloads (or optional argument), so that it's easier to use your class in more situations (but those can be deferred to 'constructing methods', e.g. TimeSpan.FromMinutes(5)). But remember that the constructor is meant to simply create and initialize your type and all its subsystems so it in a usable, consistent state, ready to do the user's bidding.

The exception, of course, is immutable types, where you must specify all parameters during construction. Usually immutable types are rather simple, so that isn't an issue. If they're more complex and the number of constructor parameters grows out of control, you could use a mutable 'initializer specifier' object, that contains all the various configurations of how to initialize the immutable object, but is easier to work with since it has mutable properties.

Should all my arguments be in the constructor?

7 Answers7