0

After reading this question How to avoid Dependency Injection constructor madness? I still have some concerns about my application design. Suppose I have a class which takes few parameters in its constructor:

public class SampleViewModel
{
    public SampleViewModel(IReader1 reader1, IReader2 reader2, IReader3 reader3)
    {
        // ...
    }
}

IReaderX is an interface for retrieving data from different sources and looks like this:

public interface IReader1
{
    int Value1 { get; }

    string Value2 { get; }
}

Now, if I wanted to aggregate this interfaces into one, I would have to create another class, say ReaderManager, which would act as a wrapper for underlying classes properties. Lot of plumbing code. Not good, if you ask me.

I tried using Composition and having all readers as properties in ReaderManager class, but then I would violate Law of Demeter if I attempted to use these readers outside.

So the question is: how should I decrease number of constructor dependencies which do not communicate with each other and only expose properties, not internal logic?

Community
  • 1
  • 1
Marek Kowalczyk
  • 3
  • 3
  • Can your view model be split up into smaller view models requiring less parameters? – NeddySpaghetti Jul 17 '14 at 11:46
  • Not really. This is core view model, the one that starts with application and manages other windows. It has to use validation data that comes from all readers and based on that, change the state of the view. – Marek Kowalczyk Jul 17 '14 at 11:49
  • Is there a reason that the interfaces for different data sources are also different, can't there be a single interface which is implemented by all data sources, so that you can use a List of one interface as dependency? – S2S2 Jul 17 '14 at 11:53
  • Each interface reads different data from different source. They have lots of properties, binding them together would end up in huge class, which also violates SRP. – Marek Kowalczyk Jul 17 '14 at 11:56
  • Difficult to say, because the code you posted is very generic and doesn't reveal the ViewModel's responsibilites. But if your readers are so very different, Ned Stoyanov's suggestion to consider splitting up the ViewModel sounds right. Even your description of it sounds like it has too many responsibilities (start app, manage windows, load data, validate, change view state). – EagleBeak Jul 17 '14 at 12:13
  • The main view model has not so many responsibilites as you would suspect. It has to read few properties from readers which are window position, size and color. Let's say that each of these properties is read from different reader. Additionally, view model performs actions only when data is valid. So I created class ReaderManager composing all other readers, which has event DataValidated. When this event is fired, my view model's handler reads position, size and color and sets them on the main window. – Marek Kowalczyk Jul 17 '14 at 12:21
  • The only problem is that, now I retrieve data like that ReaderManager.Reader1.Position, ReaderManager.Reader2.Size etc. – Marek Kowalczyk Jul 17 '14 at 12:21

1 Answers1

0

Look at it from a couple of different perspectives: the consumer, and a higher-level design.

From the perspective of `SampleViewModel`

Does it not like having so many collaborators? Maybe if it had its druthers, it would only have a single collaborator. How would that collaborator look? Create an interface to represent the role for it.

For example:

public interface ISampleViewModelReader
{
    int Value1    { get; }
    string Value2 { get; }

    double Value3 { get; }

    string Value4 { get; }
}

public class AggregatedSampleViewModelReader : ISampleViewModelReader
{
    public AggregatedSampleViewModelReader(IReader1 reader1, IReader2 reader2, IReader3 reader3)
    {
        // ...
    }
    // ...
    double Value3 { get { return reader2.Value3; } }
    // ...
}

public class SampleViewModel
{
    public SampleViewModel(ISampleViewModelReader reader)
    {
        // ...
    }
}

You indicated that you have a concern about this approach, since it would involve a "lot of plumbing code". But consider that this plumbing code is going to exist with or without a wrapper class. By defining a wrapper class, at least you're identifying an object whose sole responsibility is to handle this plumbing, rather than mixing it into the other responsibilities of the SampleViewModel.

From a higher-level design perspective

How do other objects use the IReaderX objects? Are IReader1, IReader2, and IReader3 often used together? How about IReader1 and IReader3?

The point of asking this question is to identify "hidden" abstractions so that they can be made more explicit. If certain objects are often used in tandem, it's usually representative of a broader design concept.

But sometimes a rose is a rose is a rose. Maybe SampleViewModel is the only thing that uses the IReaderX objects. Perhaps SampleViewModel's sole responsibility is to aggregate the individual readers. In these types of cases, there's nothing wrong with having several collaborators.

If another collaborator is added later on (e.g., IReader4), then all of this evaluation should take place again. Sometimes design just happens to jump out at you.

Lilshieste
  • 2,744
  • 14
  • 20
  • reminds me of mark seemann blog bost of leaky abstractions: http://blog.ploeh.dk/2010/12/02/Interfacesarenotabstractions/ and this also: http://blog.ploeh.dk/2010/12/03/Towardsbetterabstractions/ – danfromisrael Jul 18 '14 at 06:53
  • The problem is that all `IReaderX` instances are used both seperately and together, depending on the consumer. There seem to be no one good solution for that. If I don't want to create many small wrappers for particular properties, then I have to create one big wrapper class, which in turn violates SRP. And by plumbing code I meant multiple property wrappers, which we could avoid by using readers explicitly. I have to use existing architecture to design my part of the application, maybe the problem lays in higher levels of the system design. – Marek Kowalczyk Jul 18 '14 at 11:30
  • @MarekKowalczyk Then there are almost certainly some abstractions that haven't been identified yet. The `IReaderX` objects are being used as the means to some end. That is, they're the "how" to something's "what". (And since there as different things using them in different ways, each of those situations could represent an opportunity to identify an abstraction.) A question that could help identify an abstraction could be: "After something gets `Value1` from `IReader1`, what does it get used for?" – Lilshieste Jul 18 '14 at 11:53
  • @Lilshieste Ok, maybe we should consider real example. I have three readers, which I use in my view model to change its visual state, i.e. the view. But first I need to perform validation, checking if data in all readers is loaded correctly. This is not a trivial logic, so I need another man for that job, say `ReaderValidator`. It has event `DataValidated` which is fired after asynchrous validation. And yet I still need these readers explicitly. How would you compose such logic in view model? Given ofc that we need to avoid constructor madness and obey SRP. – Marek Kowalczyk Jul 18 '14 at 20:22
  • @MarekKowalczyk Who currently invokes the `ReaderValidator`? The viewmodel? – Lilshieste Jul 21 '14 at 04:43
  • @Lilshieste Yes, viewmodel gets `ReaderValidator` injected through constructor and binds to its event `DataValidated`. – Marek Kowalczyk Jul 21 '14 at 08:35
  • @MarekKowalczyk If the viewmodel is responsible for coordinating the validation of the data provided by the readers, using the `ReaderValidator`, you could consider moving this responsibility out of the viewmodel. If the viewmodel isn't responsible for this coordination, then maybe it just "blindly" handles the `DataValidated` event (i.e., without explicitly validating data from the readers) - in which case there's an *implicit* relationship between the `ReaderValidator` and `IReaderX` objects that could be made *explicit*. – Lilshieste Jul 21 '14 at 14:53
  • @Lilshieste Ok, but I need to bind to its event, because when it's fired asynchronously, only then I can poroceed and change look&feel of the view. I could probably create another thin layer with `INotifyPropertyChanged` interface implemented, but that's just overenginnering I guess. I want to do this in the viewmodel. Here's the workflow: 1. Viewmodel commands to start validating data 2. Viewmodel does other things 3. `DataValidated` is fired, that means validation ended, we can now use `IReaderX` 4. Get data from readers and change the look of the view – Marek Kowalczyk Jul 22 '14 at 08:19
  • @MarekKowalczyk It sounds like the viewmodel is only really interested in steps 2 and 4, with the understanding that the data retrieved in step 4 has been validated. So what if the validation process (steps 1 and 3) was moved out of the viewmodel? For example: what if the call to `IReaderX.Value1` returned *validated* data? – Lilshieste Jul 22 '14 at 16:57
  • @Lilshieste But if I move validation process out of the view model, how then view model can know, that all data is correct? If data is corrupted, view model changes its state accordingly (i.e. label is red) and nothing else happens - application workflow comes to an end. That is determined in `DataValidated` event handler. If everything is fine, view model can then proceed to other things. – Marek Kowalczyk Jul 23 '14 at 09:10
  • @MarekKowalczyk The viewmodel just needs to know whether or not the data is correct. Currently, the viewmodel initiates the validation process and responds appropriately when validation completes. One alternative could be for the viewmodel to ask the data if it is valid and respond accordingly (i.e., let something else initiate the validation process, and just enjoy the fruit of its labors). Keep in mind that your goal here is to identify ways to reduce the number of responsibilities the viewmodel has, since that's the core problem that leads to constructor madness/over-injection. – Lilshieste Jul 23 '14 at 13:54
  • @Lilshieste I totally agree with you, but the view model is the entry point of the application, a director which commands what needs to be done first. I cannot think of any other class that would schedule the validation first, before the view model is constructed (and it is constructed (instantiated) at the very beginning)). Hence the problem, I suppose you think that view model could be created later, after data validation occured, so it retrieves 'fruit of someone else's labor' and directly checks `IsValid`. But that's not the case, I believe. – Marek Kowalczyk Jul 23 '14 at 14:36