Are primitive constructor parameters a bad idea when using an IoC Container? [closed]

Question

Standard newbie disclaimer: I'm new to IoC and am getting mixed signals. I'm looking for some guidance on the following situation please.

Suppose I have the following interface and implementation:

public interface IImageFileGenerator
{
    void RenameFiles();
    void CopyFiles();
}

public class ImageFileGenerator : IImageFileGenerator
{
    private readonly IList<IImageLink> _links;
    private readonly string _sourceFolder;
    private readonly string _destinationFolder;
    private readonly int _folderPrefixLength;

    public ImageFileGenerator(IList<IImageLink> links, string sourceFolder, string destinationFolder)
    {
        _links = links;
        _sourceFolder = sourceFolder;
        _destinationFolder = destinationFolder;
        _folderPrefixLength = 4;
    }

    public void RenameFiles()
    {
        // Do stuff, uses all the class fields except destination folder
    }

    public void CopyFiles()
    {
        // Do stuff, also uses the class fields
    }
}

I'm getting confused whether I should only send interface/dependencies to the constructor, create some parameter object and pass it to the constructor or keep it as is and pass in the parameters at the time of resolving an instance.

So is there a more correct way of setting up this code to work best with an IoC container? Would either of the following be preferred design-wise over my current layout?

1.

public interface IImageFileGenerator
{
    void RenameFiles(IList<IImageLink> links, string sourceFolder);
    void CopyFiles(IList<IImageLink> links, string sourceFolder, stringDestFolder);
}

public class ImageFileGenerator : IImageFileGenerator
{
    private readonly int _folderPrefixLength;

    public ImageFileGenerator()
    {
        _folderPrefixLength = 4;
    }

    public void RenameFiles(IList<IImageLink> links, string sourceFolder)
    {
        // Do stuff
    }

    public void CopyFiles(IList<IImageLink> links, string sourceFolder, stringDestFolder)
    {
        // Do stuff
    }
}

I don't like that I'm passing in the exact same thing in both cases (except the destination folder). In the current implementation of the IImageFileGenerator, I need to execute both methods and the same values were needed for each method. That is why I passed the state in via the constructor.

2.

public interface IImageFileGenerator
{
    void RenameFiles();
    void CopyFiles();
}

public class ImageLinkContext
{
    // various properties to hold the values needed in the
    // ImageFileGenerator implementation.
}

public class ImageFileGenerator : IImageFileGenerator
{
    private readonly IList<IImageLink> _links;
    private readonly string _sourceFolder;
    private readonly string _destinationFolder;
    private readonly int _folderPrefixLength;

    public ImageFileGenerator(ImageLinkContext imageLinkContext)
    {
        // could also use these values directly in the methods 
        // by adding a single ImageLinkContext field and skip 
        // creating the other fields
        _links = imageLinkContext.ImageLinks;
        _sourceFolder = imageLinkContext.Source;
        _destinationFolder = imageLinkContext.Destination;
        _folderPrefixLength = 4;
    }

    public void RenameFiles()
    {
        // Do stuff, uses all the class fields except destination folder
    }

    public void CopyFiles()
    {
        // Do stuff, uses all the class fields
    }
}

This approach may even be tweaked to a Facade Service (previously called aggregate services) as mentioned by Mark Seemann here.

I've also read that you could use properties for those values and use property injection, though it seems like that is not preferred anymore (autofac mentions constructor injection is preferred... Ninject I believe even removed the ability in version 2).

Alternatively I've read that you can also create an initialize method and ensure that the properties are set in there.

So many options and I'm getting more confused as I read more about this stuff. I'm sure there is no definitive correct way (or maybe there is, at least for this example???), but maybe someone can provide pros and cons of each approach. Or maybe there is another approach that I've totally missed.

I realize now that this question is probably a little on the subjective side (and is really more than one question), but I'm hoping you can forgive me and provide some guidance.

PS - I'm currently trying my hand with autofac in case that influences which design may fit better.

NOTE: I've made a slight change to the code about the destination folder... it is not used by RenameFiles (may have a bearing on your answer).

Ludhiana

Here's a related discussion on injecting services vs data: #1819039 – Gaidano

@Peter Lillevold: Interesting, I'll spend some time looking at factory delegates. I can see them coming in handy. Thanks for the link (and the attached article in your answer: peterspattern.com/generate-generic-factories-with-autofac). – Ludhiana

Answer 1

Well I ended up redesigning this after reading the book Dependency Injection in .Net (I highly recommend this book to any object-oriented developer, not just .Net developers and not just those interested in using an IoC container!).

I've now got the following in a Domain assembly:

public interface IImageFileService
{
    void RenameFiles();
    void CopyFiles(); 
}

public interface IImageLinkMapRepository
{
    IList<ImageLink> GetImageLinks(); 
}

Then in a FileAccess assembly I've created implementations for these interfaces:

public class ImageFileService : IImageFileService
{
    public ImageFileService(IImageLinkMapRepository repository)
    {
        // null checks etc. left out for brevity
        _repository = repository;
    }

    public void RenameFiles()
    {
        // rename files, using _repository.GetImageLinks(), which encapsulates
        // enough information for it to do the rename operations without this
        // class needing to know the specific details of the source/dest dirs.
    }

    public void CopyFiles() 
    { 
        // same deal as above
    }
}

So essentially, I've removed the need for primitive types in my constructor, at least for this class. At some point I did need that information, but that was injected into the ImageLinkMapRepository where the information made more sense. I used autofac named parameters to handle injecting them.

So I guess to answer my own question, primitive constructor parameters are a good idea if they make sense, but make sure you put them where they belong. If things don't seem to be jiving properly, it can probably be improved by rethinking the design.

Ludhiana

Answer 2

In your example what you are actually passing are dependencies, but moreover data needed by the class to operate.

In your case it sounds like the methods RenameFiles() and CopyFiles() operate on the parameters that are passed to them. Given their names I would think that the methods on a single instance of ImageFileGenerator can be called with different parameters. If that is true arguably the parameters should be on the method calls themselves not the constructor.

If, on the other hand, on one instance RenameFiles() and CopyFiles() are each only called once with the same parameters the parameters would be good candidates for the constructor.

I personally would try to avoid property injection for required dependencies - in that case constructor injection is much more appropriate.

Markus

The current usage is that both are called only once and with the same parameter values. And it is likely that we will always be calling RenameFiles, and immediately after, calling CopyFiles. I suppose I could combine them into a single public method and then make both of those private. I was trying to split up the functionality for easier unit testing. – Ludhiana

@JasonDown: In that case I think you answered your own question. Also if there really is just one operation on the data you pass in you could just move this into a static method. – Markus

Definitely something to consider. Thinking out load... there is a actually good chance that another implementation of the interface will be needed that will only copy images... and will have a different source and destination (it would be copying the images, but the copies would be converted to a thumbnail size). So maybe passing the parameters into each method is the way to go. – Ludhiana