5
\$\begingroup\$

I'm writing a small library to do some image-processing on GPU for WinRT, however, I'm not sure if such design breaks SRP.

One class loads and saves image, renders and maintains filters - isn't it too much?

Code

public class FilteredImageProvider : IDisposable
{
    public event Action ImageSourceChangedEvent;
    GPUImage currentImage;
    SharpDXRenderer renderer;
    FilterContainer filterContainer;
    ImageGPULoaderFactory loaderFactory;

    public FilteredImageProvider()
    {
        this.renderer = new SharpDXRenderer();
        this.filterContainer = new FilterContainer();
        this.loaderFactory = new ImageGPULoaderFactory();
    }

    public ImageSource Image
    {
        get { return renderer.GetRendererSource(); }
    }

    public async Task LoadFromFile(IStorageFile sourceFile)
    {
        var imageLoader = loaderFactory.GetImageLoader(sourceFile);

        if (currentImage != null)
            currentImage.Dispose();

        currentImage = await imageLoader.LoadImage();
        renderer.Load(currentImage);
        ImageSourceChangedEvent();
    }

    public async Task SaveToFile(IStorageFile destinyFile)
    {
        var imageSaver = loaderFactory.GetImageLoader(destinyFile);
        var deviceManager = renderer.GetDeviceManager();
        await imageSaver.SaveImage(currentImage, deviceManager);
    }

    public void Render()
    {
        var filterCompilator = new FilterCompilator();
        var renderedImage = currentImage.GetImageAsEffect();
        var compiledEffect = filterCompilator.CompileFiltersToEffect(renderedImage, filterContainer);

        renderer.Render(compiledEffect);
    }

    public void AddFilter(IFilter filter)
    {
        filterContainer.AddFilter(filter);
    }

    public void RemoveFilter(IFilter filter)
    {
        filterContainer.RemoveFilter(filter);
    }

    public void Undo()
    {
        filterContainer.Undo();
    }

    public void Redo()
    {
        filterContainer.Redo();
    }


    public void Dispose()
    {
        if (currentImage != null)
            currentImage.Dispose();
        renderer.Dispose();
    }
}
\$\endgroup\$
0
5
\$\begingroup\$

Private Fields

I prefer to prefix my private fields with an underscore, and as readonly as possible. If you don't like the underscore, fine - but then be consistent with this, and don't use it only in your constructor - use this whenever you're referring to a private field. It greatly helps telling private fields from parameters.

Personally I don't like sprinkling this everywhere in my code, so I prefer the underscore; I also like my access modifiers explicit:

public class FilteredImageProvider : IDisposable
{
    private readonly SharpDXRenderer _renderer;
    private readonly FilterContainer _filterContainer;
    private readonly ImageGPULoaderFactory _loaderFactory;

    private GPUImage _currentImage;

    //...

Constructor

The readonly fields are still assigned in the constructor:

public FilteredImageProvider()
{
    _renderer = new SharpDXRenderer();
    _filterContainer = new FilterContainer();
    _loaderFactory = new ImageGPULoaderFactory();
}

These three should be injected as constructor arguments - creating these instances within the constructor breaks SRP, at least if you're applying DI principles. SRP is only the S of SOLID. By taking these instances in as constructor arguments, you greatly facilitate the rest of SOLID.


You're mostly exposing cherry-picked encapsulated methods, that works for me. However I find that you shouldn't be newing up the FilterCompilator like you're doing. It should be constructor-injected, and perhaps made a dependency of FilterContainer, or of another type that would deal with everything that can be done with a Filter.


Note: If _renderer is injected in the constructor, then you shouldn't call _renderer.Dispose() since you've put that burden onto the caller (ideally your favorite IoC container).

\$\endgroup\$
5
  • 1
    \$\begingroup\$ The problem with SharpDXRenderer, ImageGPULoaderFactory and FilterCompilator is that it's pretty "low-level" code messing with DirectX and I dont think this should be constructed by client. It is also not possible to write good unit tests for such code because of too big coupling with Windows Runtime library (can't use any mocking framework) so DI wouldn't give me any benefits. Don't you think UI code/Low level code should be treated a bit diffrent? \$\endgroup\$ – fex Jan 6 '14 at 11:18
  • \$\begingroup\$ I don't think you need to unit test DX stuff. But by treating them as the dependencies they are, you allow your own code to be testable independently of DX; by keeping the tight coupling you inevitably call code that you don't own when you test this class. DI cannot be applied "halfway". \$\endgroup\$ – Mathieu Guindon Jan 6 '14 at 11:52
  • \$\begingroup\$ I wouldn't want .SaveToFile to hit the file system in my unit tests; hence I'd want to inject a factory that I have control over, even if it means wrapping the DX stuff with an interface. \$\endgroup\$ – Mathieu Guindon Jan 6 '14 at 11:56
  • \$\begingroup\$ Ok. So I should create some common interface like IRenderable, IFilterRepository, ILoaderFactory (or maybe abstract class) and inject them in constructor? How to deal with .GetDeviceManager line - cause I don't think IRenderable interface should contains such a method? \$\endgroup\$ – fex Jan 6 '14 at 11:58
  • \$\begingroup\$ Whatever works with your mocking framework. GetDeviceManager sounds like an implementation detail, maybe loaderFactory can supply the imageSaver with its own deviceManager? \$\endgroup\$ – Mathieu Guindon Jan 6 '14 at 14:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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