2

As the title says, Im thinking if it's a good practise to have classes that have Open and Close methods in the sense that they can be reused without a new instance being created again.

Also, if I want to use Dependency Injection with the class and use it with IoC, then I dont think I can pass for example a "filePath" argument to the constructor, since normally the dependencies are resolved by the framework/IoC lib itself. Thats why for the following example with an Open method feels approriate. But correct me, if Im wrong.

Let me give an example of a class Im working on, called CsvWriterService It has a dependency on FileSystem And also StreamReader

    public CsvWriterService(
        IFileSystem fileSystem, 
        IStreamReaderWrapperFactory streamReaderWrapperFactory
        )

Now inside of it I have a method Open that creates an instance for the StreamReader with a wrapper and also a new instance of CsvReader which is a dotnet lib.

    public void Open(string filePath)
    {
        if (disposed)
        {
            throw new ObjectDisposedException(nameof(CsvReaderService), "Cannot call Open on a disposed object.");
        }

        if (!_fileSystem.FileExists(filePath))
        {
            _fileSystem.CreateEmptyFile(filePath);
        }

        _streamReader = _streamReaderWrapperFactory.Create(filePath) ?? throw new ArgumentNullException(filePath);
        _csvReader = new CsvReader(new StreamReaderWrapperAdapter(_streamReader), CultureInfo.InvariantCulture);
    }

Also theres the Close method, which right now is a bit useless and just disposes the managed resources

    public void Close()
    {
        _csvReader?.Dispose();
        _streamReader?.Dispose();
    }

The class also has a dispose method

    protected virtual void Dispose(bool disposing)
    {
        if (!disposed)
        {
            if (disposing)
            {
                Close();
            }

            // Release unmanaged resources (if any) and set large fields to null
            // ...

            disposed = true;
        }
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

When I call Dispose I don't want the client of the class to call open again, because the object should be unusable.

Obviously the class does other things besides this.

But does it seem like a good class design? With this the client can use the same object many times with the Open method.

5
  • 1
    Not to be a smart aleck but the term "good" is not an engineering term so it is very hard to tell you if a design is good or not without defining it. If you're asking "Does this design allow me to re-use objects?" then the answer is yes. I don't know that object re-use is all that important-- in fact it may introduce more code errors as you have to manage both the life cycle of the instance and its state. Do you see some advantage of this compared to using a new object instance each time?
    – John Wu
    Commented Aug 8, 2023 at 20:33
  • There is no inherit problem with such a design. The issue is bring able to correctly handle state transitions between closed state and opened state. This is why it's technically easier to use a different instance, you only need to deal when the opened-closed transition, and not closed - opened.
    – Ccm
    Commented Aug 9, 2023 at 7:37
  • I see @JohnWu but what about IoC for the injection of dependencies? The constructor shouldn´t be used to pass a parameter like "filePath" I believe.
    – Nmaster88
    Commented Aug 9, 2023 at 8:27
  • 1
    @Nmaster88 The factory pattern can help with calling a constructor which has got some DI parameters and some "runtime" parameters (for lack of a better word). This allows you to have the best of both worlds.
    – Flater
    Commented Aug 9, 2023 at 9:08
  • @Flater I see, Im already making a StreamReaderWrapperFactory call in the Open method, which is responsible to return the actual instance of StreamReader. But if we use IoC and call an endpoint the instance of CsvWriterService I mention will be created by the framework, so how can I pass a runtime parameter like "filePath" without creating a standalone method? The only way is to force the user to do the instantiation of the class itself, which might be the right way.
    – Nmaster88
    Commented Aug 9, 2023 at 10:26

3 Answers 3

3

I am thinking if it's a good practice to have classes that have Open and Close methods

Not in general. But in this example, where a class creates an abstraction for some file writer or stream writer service, where the terms Open and Close are well-established and popular, such methods make sense. It will allow you to create an CsvWriterService with a specific combination of IFileSystem and IStreamReaderWrapperFactory and then process multiple files with that combination. This can be useful regardless whether ones uses an IoC container for it, or does Dependency Injection manually.

What you have to care for, however, is the temporal coupling and the possibility of someone calling the methods in the wrong order. For example, your current implementation misses preventive measures against someone calling Open twice, without calling Close in between.

For dealing with this, I would probably set _csvReader and _streamReader to null inside Close after disposing them. That will allow to check if those variables are not null inside Open, and when not, either call Close there, or throw a specific exception. There may also be other operations in the CsvWriterService which will not work as long as the file stream is not in the "opened" state. They all should check this and throw a specific exception as well.

Can it be benefical to omit Open / Close and don't separate construction and opening? That is somewhat opinionated and may depend on the specific case. When the design using Open / Close will force you to write a lot of boiler plate code inside where you have to check for the Open state in several methods and operations, then the answer may be "yes". However, this comes for a price to pay: as mentioned in a comment below your question, this design will probably require an extra factory class in case you need to process multiple files with the same pair (IFileSystem, IStreamReaderWrapperFactory), and that's also boilerplate code. Such a design may be less convenient for the user of this class. Hence, it is classic trade-off situation, and you have to decide which of the alternatives fit better to your case.

3

Also, if I want to use Dependency Injection with the class and use it with IoC, then I dont think I can pass for example a "filePath" argument to the constructor, ...

Reading between the lines, I feel there's already a general agreement on your end that it would be better to tie the open/closed nature of the stream to the lifecycle of the objects, i.e. the constructor opens it, the destructor/dispose closes it. I assume this is why you immediately jump to pointing out the issues with opening the stream via the constructor.

Based on my assumption, I'm not going to further answer why because it seems you're already in agreement on that point, the remaining question is how to achieve it.

..., since normally the dependencies are resolved by the framework/IoC lib itself.

The core of your question here seems to be about how to marry the ideas of DI-related constructor parameters and the kinds of parameters that are scoped to a particular request (e.g. a specific filename) and that cannot have its value hardcoded in the same way that DI dependencies (or their types) are.

For the sake of brevity I'm going to refer to these as "dependencies" (DI) and "parameters" (e.g. the filename) in the rest of the answer.

The question I'm going to answer here is how a constructor can accept both dependencies and parameters at the same time. This should resolve the root problem that led you to instead trying to design these Open/Close methods.

When an object receives dependencies, the constructor is being called by the DI framework itself. This mean that you are only able to provide constructor arguments that are known by the DI framework, which in turn limits you to knowledge that you had at the time of configuring the DI framework. This seems to make it impossible for such a constructor to contain a parameter.

That's true. By the time I show you my answer, you'll see that the object in question will no longer be constructed by the DI framework, specifically to work around this problem.

But then we would face the opposite problem. Because we are now constructing this object outside of the DI container, we lose access to its knowledge about which dependencies need to be provided to the constructor. How do we retain the knowledge of the DI framework without being tied to the DI framework calling the constructor itself?

This can be achieved by introducing an intermediary factory layer. The factory produces the object. This means that we can provide the parameter values from calling the factory method. But at the same time, the factory itself can be registered in (and generated from) the DI container, which means that it can have all the dependencies you need injected into it (the factory itself), which it can then pass on to the object that it creates.

In the following example, I've made it so your CsvWriter takes in a dependency (ILogger) and a parameter (filename):

public class CsvWriter : IDisposable
{
    private readonly ILogger logger;
    private readonly string fileName;

    private readonly CsvReader csvReader;

    public CsvWriter(Ilogger logger, string fileName)
    {
        this.logger = logger;
        this.fileName = fileName;

        logger.Log($"Opening file {filename}");
        this.csvReader = GetCsvReader(fileName);
    }

    public void Dispose()
    {
        csvReader.Dispose();
    }
}

I've omitted the content of the private GetCsvReader method for brevity, it's essentially what you did in your Open method.

I hope you agree that this is what your ultimate goal was, i.e. to have a clean and simple CsvWriter implementation. But now we've also set the stage for the problem, how do we solve it? Well, the intermediary factory would look like this:

public class CsvWriterFactory
{
    private readonly ILogger logger;

    public CsvWriterFactory(ILogger logger)
    {
        this.logger = logger;
    }

    public CsvWriter Create(string fileName)
    {
        return new CsvWriter(this.logger, fileName);
    }
}

Look at the CsvWriter constructor and the two arguments we're passing in. Now trace the origin of these arguments.

  • Assuming this factory was instantiated by the DI container, the DI container will have provided the ILogger dependency; thus using the knowledge of the DI container.
  • The fileName is given by whoever calls this method. The filename was not known in advance in the same way that the logger was.

Now let's look at how you would use this. If you have a service that want to be able to make use of a CsvWriter, it can simply have a CsvWriterFactory injected. For example:

public class PersonService
{
    private reasonly CsvWriterFactory factory;

    public PersonService(CsvWriterFactory factory)
    {
        this.factory = factory;
    }

    public void Save(Person person)
    {
        string filename = $"{person.FirstName}-{person.LastName}.csv";

        using(var writer = factory.Create(filename))
        {
            writer.WriteCell(person.FirstName);
            writer.WriteCell(person.LastName);
            writer.WriteCell(person.Age);
            writer.WriteCell(person.Address);
        }
    }
}

This is, in my opinion, the cleanest way to implement all of this. Notice how the consumer (PersonService) did not need to care about the writer's dependency (ILogger) in any way, and at the same time CsvWriter was able to tie its lifecycle to that of the underlying disposable components without needing to resort to clumsy Open/Close logic.

This is the route I'd take if I were you.

0

I would avoid reusing the same instance for different purposes, if the objects have any kind of identity. Say you have instances representing customers. And among other things, a view displaying customers holds references to those instances.

If one of those instances are reused, then that view will display the wrong data.

1
  • The way I have now its reusable because the user can call Open method multiple times, if he hadnt call Dispose. If the class instantiation is managed by the client and not by IoC we can use constructor with the "filePath" which is a dynamic parameter, in my opinion.
    – Nmaster88
    Commented Aug 9, 2023 at 10:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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