0
\$\begingroup\$

I implemented this code, looking at a dependency injection program as an example. I wanted to confirm with you all whether this implementation is the right way of doing the same.

namespace TestAsyncProjectWithDI
    {
            public interface IReadFileTypes
            {
                    void ReadUsingFileSteamAsync();
            }
            public class ReadfileFromSystem:   IReadFileTypes
            {
                private readonly StreamReader _reader =null;

                public ReadfileFromSystem(StreamReader reader)
                {
                    _reader = reader;
                }
                public async void ReadUsingFileSteamAsync()
                {
                    await Task.Run(()=> Console.WriteLine("Reading the content of File: {0}", _reader.ReadToEnd()));
                    Console.ReadLine();
                }
          }

        public class Builder
        {
                public  IReadFileTypes ReadFileTypes { get; set; }
                private string path = "C:\\Brackets\\Projects\\index2.html";

                public Builder()
                {
                    ReadFileTypes = new ReadfileFromSystem(new StreamReader(new FileStream(path, FileMode.Open, FileAccess.Read)));
                }
        }

        public class Program
        {
                private readonly IReadFileTypes _readFileTypes = null;

                public Program(IReadFileTypes readFileTypes)
                {
                    this._readFileTypes = readFileTypes;
                }
                public void FunctionInvoke()
                {
                    _readFileTypes.ReadUsingFileSteamAsync();
                }
        }

        //starting point of the class
        public class Default
        {
            static void Main(string[] args)
            {
                var build = new Builder();
                var obj = new Program(build.ReadFileTypes);
                obj.FunctionInvoke();
                Console.ReadLine();
            }
        }
    }
\$\endgroup\$
2
  • \$\begingroup\$ To make life easier for reviewers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question. \$\endgroup\$ Commented May 14, 2015 at 18:23
  • \$\begingroup\$ I have added additional details for the question \$\endgroup\$ Commented May 14, 2015 at 19:02

1 Answer 1

2
\$\begingroup\$

You have implemented dependency injection correctly. IReadFileTypes is injected into Program. In terms of the correctness of your example however, pretty much everything else is completely wrong! Don't worry though, we all have to start somewhere, so I will offer some tips. These tips might help you with this example, but if you want to improve, you should probably do some personal training/studying. Pluralsight is a really nice place to start. They have some beginner C# courses that you can do with a free trial. Anyway, on with the code review!


Streams implement IDisposable, and should always be disposed of when used. Typically this is done with using block.


Builder is not a builder, it is a factory. It's job is to create instances of IReadFileTypes. This shouldn't be done in it's constructor because then you need a new factory, each time you want a new IReadFileTypes.


async void should almost always be avoided. Try to return Tasks from async methods so they can be awaited by the caller. A Task should be returned from ReadUsingFileSteamAsync() and have it's Wait() method called.


The point of an interface is to abstract away the implementation details from the client. IReadFileTypes has the method ReadUsingFileSteamAsync() which publicises exactly what the implementation is. This is called a leaky abstraction. When writing an interface, think about what other implementations might look like. One thing that you do know about all implementations is that they are going to need a file path. An alternative interface might then look something like this

public interface IFilePrinter
{
    Task PrintFileAsync(string filePath);
}

You could imagine this interface being implemented by e.g. ColoredFilePrinter, VerboseFilePrinter e.t.c.


ReadfileFromSystem is a bit nuts! You have wrapped _reader.ReadToEnd() in a Task.Run() and awaited that. Instead you could just await _reader.ReadToEndAsync().


Putting all of this together, your example could look something like

public class Default
{
    static void Main()
    {
        var printerFactory = new FilePrinterFactory();
        var filePrinter = printerFactory.CreateDefaultPrinter();

        var program = new Program(filePrinter);
        program.DoStuff();

        Console.ReadLine();
    }
}

public interface IFilePrinter
{
    Task PintFileAsync(string filePath);
}

internal class DefaultFilePrinter : IFilePrinter
{
    public async Task PintFileAsync(string filePath)
    {
        using (var reader = File.OpenText(filePath))
        {
            var fileContent = await reader.ReadToEndAsync();
            Console.WriteLine("File content: " + fileContent);
        }
    }
}

public class FilePrinterFactory
{
    public IFilePrinter CreateDefaultPrinter()
    {
        return new DefaultFilePrinter();
    }

    public IFilePrinter CreateSpellCheckingPrinter(string language)
    {
        throw new NotImplementedException();
    }
}

public class Program
{
    private const string FilePath = @"C:\Brackets\Projects\index2.html";
    private readonly IFilePrinter _filePrinter;

    public Program(IFilePrinter filePrinter)
    {
        // I don't care what kind of printer I have, I just know that I need to print a file!
        _filePrinter = filePrinter;
    }

    public void DoStuff()
    {
        _filePrinter.PintFileAsync(FilePath).Wait();

        // TODO: Do more stuff            
    }
}

PS: In case you are interested, you can do the whole thing in 1 line of code

Console.WriteLine("File content: " + File.ReadAllText(@"C:\Brackets\Projects\index2.html"));
\$\endgroup\$
0

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.