7
\$\begingroup\$

I'm actually building some kind of framework to promote code reuse without over-reusing it. Besides, sometimes as it occurs with generic types, one expects type parameters to be constrained for the purpose.

In MVP, those constraints translate well enough.

Presenter

public abstract class Presenter<V, M> where V : IView<M> where M : IModel {
    protected Presenter(V view) {
        if (view == null) throw new ArgumentNullException("view");
        View = view;
    }

    public V View { get; private set; }

    public void CloseView() { View.CloseSelf(); }
    public void HideView() { View.HideSelf(); }
    public void SetViewMessage(string message) { View.Message = message; }
    public void SetViewTitle(string title) { View.Title = title; }
    public void ShowView() { View.ShowSelf(); }
}

IView

public interface IView<M> where M : IModel {
    string Message { get; set; }
    M Model { get; set; }
    string Title { get; set; }

    void CloseSelf();
    void HideSelf();
    void ShowSelf();
}

IModel

// This is called the marker interface pattern.
public interface IModel { }

So any business domain object shall implement interface IModel or be marked as being an IModel in order to be accepted in type parameter M to the IView<M> interface.

But now, when comes time to play with the MVPVM UI architecture, this gets kind of repetitive, or else, annoying, to have to specify all those types because the same type can be stated twice and even three times for a single class declaration.

Presenter

public abstract class Presenter<V, VM, M> 
    where V : IView<VM, M> 
    where VM : ViewModel<M> 
    where M : IModel {

    protected Presenter(V view) {
        if (view == null) throw new ArgumentNullException("view");
        View = view;
    }

    public V View { get; private set; }

    public void CloseView() { View.CloseSelf(); }
    public void HideView() { View.HideSelf(); }
    public void SetViewMessage(string message) { View.Message = message; }
    public void SetViewTitle(string title) { View.Title = title; }
    public void ShowView() { View.ShowSelf(); }
}

IView

public interface IView<VM, M> where VM : ViewModel<M> where M : IModel {
    string Message { get; set; }
    string Title { get; set; }
    VM ViewModel { get; }

    void CloseSelf();
    void HideSelf();
    void ShowSelf();
}

ViewModel

public abstract class ViewModel<M> : INotifyPropertyChanged where M : IModel {
    protected ViewModel(M model) {
        if (model == null) throw new ArgumentNullException("model");
        this.model = model;
    }

    public M Model { get { return model; } set { setNonNullModel(value); } }

    protected void RaisePropertyChangedFor(
        Expression<Func<M, object>> propertyExpression) {
        var propertyName = propertyExpression.GetMemberName();
        RaisePropertyChanged(propertyName);
    }

    protected void RaisePropertyChangedFor(string propertyName) {
        if (propertyName == null) throw new ArgumentNullException("propertyName");
        if (PropertyChanged != null)
            PropertyChanged(this, new PropertyChangedEventArgs(propertyName);
    }

    private void setNonNullModel(M value) {
        if (value != null) {
            model = value;
            RaisePropertyChangedFor("Model");
        }            
    }

    private M model;
}

IModel

public interface IModel { }

So, when I come to test my MVPVM Presenter class, I need to instantiate it as follows:

[TestFixture]
public class PresenterTests {
    [SetUp]
    public void SetUp() {
        viewMock = new Mock<IView>();
        sut = new DummyPresenter(viewMock.Object);
    }

    Presenter<IView<ViewModel<IModel>, IModel>, ViewModel<IModel>, IModel> sut;
}

public class DummyPresenter 
    : Presenter<IView<ViewModel<IModel>, IModel>, ViewModel<IModel>, IModel> {
    public DummyPresenter(IView<ViewModel<IModel>, IModel> view) : base(view) { }
}

I believe I'm overcomplicating things, and I just don't seem to see my way out since I wish to be able to constraint types and make these objects as generic as possible.

Any review in regards to solution which could lead to a simplification of this code is welcome.

EDIT

As discussed in comments with @Mat's Mug, I take it that the generic types are not much of a problem as long as one doesn't have to bring them throughout the code. This being said, only the derived types shall be brought along the code since these generic types are abstracts. Once inheritred from, one shan't need to work with them all the time. For instance:

CustomerManagementPresenter

public class CustomerManagementPresenter 
    : Presenter<ICustomerManagementView, CustomerManagementViewModel, Customer> {
    public CustomerManagementPresenter(ICustomerManagementView view) : base(view) { }

    // Operations and members specific to this presenter.
}  

ICustomerManagementView

public interface ICustomerManagementView 
    : IView<CustomerManagementViewModel, Customer> {

    // Member definitions specific to this view.
}

CustomerManagementViewModel

public class CustomerManagementViewModel : ViewModel<IList<Customer>> {
    public CustomerManagementViewModel(IList<Customer> model) : base(model) { }

    // Operations specific to this view model.
}

Customer

public class Customer {
    public string Email { get; set; }
    public string GivenName { get; set; }
    public long PhoneNumber { get; set; }
    public string Surname { get; set; }
    // Other possible members here...
}
\$\endgroup\$
6
\$\begingroup\$
public abstract class Presenter<V, VM, M> 
    where V : IView<VM, M> 
    where VM : ViewModel<M> 
    where M : IModel

Convention for single-letter generic type parameter is to start with T, followed by U. If you're going to have more than that, In any case, it's better to give them meaningful names, and to start them with a T to stick with the convention.

This would be:

public abstract class Presenter<TView, TViewModel, TModel> 
    where TView : IView<TViewModel, Model> 
    where TViewModel : ViewModel<TModel> 
    where TModel : IModel

This isn't a useful comment:

// This is called the marker interface pattern.
public interface IModel { }

A more use useful comment would be a XML summary that shows up on tooltips and IntelliSense when writing the client code. A good framework is a documented framework: every exposed member should at least have a short descriptive XML summary:

/// <summary>A marker interface that marks a class as a model.</summary>
public interface IModel { }

So that the consumer of your code knows what to expect, as they type : IModel - these XML comments show up in IntelliSense.

But the problem is not the comment; Marker Interface is not only a pattern, it's also a code/design smell, 99% of the time (as would be, say, a Singleton Pattern).

You're probably better off just constraining the M/TModel type parameter like this:

    where TModel : class

Which ensures the model is a reference type, and leaves you with more flexibility and less boilerplate in implementing classes - limiting valid models to types implementing an empty IModel interface, serves no real purpose.


protected void RaisePropertyChangedFor(string propertyName) {
    if (propertyName == null) throw new ArgumentNullException("propertyName");
    if (PropertyChanged != null)
        PropertyChanged(this, new PropertyChangedEventArgs(propertyName);
}

You're missing a ) on the event method call line, just before the ;. Visual Studio has it squiggly-red underlined, and refuses to build your project. Pretend I didn't see it.

The invocation isn't thread-safe. This would be:

protected void RaisePropertyChangedFor(string propertyName) 
{
    if (propertyName == null) 
    { 
        throw new ArgumentNullException("propertyName");
    }

    var handler = PropertyChanged;
    if (handler != null)
    {
        handler(this, new PropertyChangedEventArgs(propertyName));
    }
}

Notice the local handler variable belongs to whatever thread is running this; it isn't the case for PropertyChanged, so there's a little race condition where a thread would remove the last handler on PropertyChanged after the null check, but before the invocation.

\$\endgroup\$
  • \$\begingroup\$ 1. The missing parameter is a typo. I sure type all parenthesis in my code. =P 2. I agree with your point on using XML comments. In fact, the comment was there only for this question's purpose, as I have no comments at all in real code. Besides, you're probably making a good point with this type constraint. Instead, I shall consider using the class type constraint. Besides, doesn't it allow anyone to use a ViewModel<M> for any purpose else then interacting with an actual model? I pretty much like your review. As for the local variable handler, didn't know 'bout it and race condition (+1) \$\endgroup\$ – Will Marcouiller Nov 27 '14 at 7:17
  • \$\begingroup\$ Now, let's say I apply all of your recommendations. Isn't it over-complicated to have so much types within the Presenter of MVPVM pattern, or is it just what it takes in the end to make it work as expected and I just don'T have to very much care about it? I mean, look at the Presenter initialization within the PresenterTests class, it gets very llooooonnnnggg! =P Is this just okay, or would there be any suggestion to make it simpler? I wish to improve my coding, hence my interest to Code Review. \$\endgroup\$ – Will Marcouiller Nov 27 '14 at 7:24
  • \$\begingroup\$ You could use var for implicit typing of the sut identifier, and let the compiler infer the generic type parameters whenever possible. \$\endgroup\$ – Mathieu Guindon Nov 27 '14 at 14:51
  • \$\begingroup\$ So if I get it right, both the type contraints and the design looks good to you, doesn't it? \$\endgroup\$ – Will Marcouiller Nov 27 '14 at 16:10
  • \$\begingroup\$ I merely skimmed over your code and made a few observations, but I don't think the generic type parameters are much of a problem if they don't force the client code to specify them all the time. \$\endgroup\$ – Mathieu Guindon Nov 27 '14 at 16:14
1
\$\begingroup\$

Raising the PropertyChanged-Event with null or string.Empty should be possible and is perfectly valid when using WPF as UI.

In WPF this indicates that all of the properties have changed.

protected void RaisePropertyChangedFor(string propertyName) 
{
    var handler = PropertyChanged;
    if (handler != null)
    {
        handler(this, new PropertyChangedEventArgs(propertyName));
    }
}
\$\endgroup\$
  • \$\begingroup\$ I was unaware of this. Thanks for your review. =) \$\endgroup\$ – Will Marcouiller Nov 27 '14 at 13:55

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.