Model-View-Presenter Winforms app

Question

This is my first attempt at a Model-View-Presenter pattern Winforms application. The model is a class that stores a bunch of primitives, and the view is a Form that provides a ListBox for selecting individual Data items, whose members are then displayed in text boxes.

DataModel.cs

using System;
using System.Collections.Generic;
using System.Linq;

namespace ModelViewPresenter
{
    public static class RandomExtensions
    {
        public static decimal NextDecimal(this Random random)
        {
            byte scale = (byte)random.Next(29);
            bool sign = random.Next(2) == 1;
            return new decimal(
                random.NextInt32(),
                random.NextInt32(),
                random.NextInt32(),
                sign,
                scale);
        }

        public static int NextInt32(this Random random)
        {
            unchecked
            {
                int firstBits = random.Next(0, 1 << 4) << 28;
                int lastBits = random.Next(0, 1 << 28);
                return firstBits | lastBits;
            }
        }

        public static string NextString(this Random random, int length)
        {
            const string chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
            return new string(Enumerable.Repeat(chars, length).Select(s =>
            s[random.Next(s.Length)]).ToArray());
        }
    }

    public class DataModel : IDataModel
    {
        private static int NextId = 1;
        private static Random random = new Random();
        public bool Bool { get; set; }
        public byte Byte { get; set; }
        public char Char { get; set; }

        public IList<DataModel> Data
        {
            get
            {
                var data = new List<DataModel>();
                for (int i = 0; i < 10; i++)
                {
                    data.Add(RandomData());
                }
                return data;
            }
        }

        public decimal Decimal { get; set; }
        public double Double { get; set; }
        public float Float { get; set; }
        public int Id { get; set; }
        public int Int { get; set; }
        public long Long { get; set; }
        public object Object { get; set; }
        public short Short { get; set; }
        public string String { get; set; }

        private static DataModel RandomData()
        {
            return new DataModel
            {
                Id = NextId++,
                Bool = Convert.ToBoolean(random.Next(2)),
                Byte = Convert.ToByte(random.Next(Byte.MaxValue)),
                Char = Convert.ToChar(random.Next(Char.MaxValue)),
                Decimal = random.NextDecimal(),
                Double = random.NextDouble() * Double.MaxValue,
                Float = (float)random.NextDouble() * float.MaxValue,
                Int = random.NextInt32(),
                Long = random.Next() << 32 | random.Next(),
                Object = new int[] { random.Next(), random.Next() },
                Short = (short)random.Next(short.MaxValue),
                String = random.NextString(32)
            };
        }
    }
}

IDataModel.cs

using System.Collections.Generic;

namespace ModelViewPresenter
{
    internal interface IDataModel
    {
        IList<DataModel> Data { get; }
    }
}

DataView.cs

using System;
using System.Collections.Generic;
using System.Windows.Forms;

namespace ModelViewPresenter
{
    public partial class DataView : Form, IDataView
    {
        public DataView()
        {
            InitializeComponent();
            BindComponent();
        }

        public event Action DataSelected;

        public IList<DataModel> Data
        {
            get
            {
                return this.dataListBox.DataSource as IList<DataModel>;
            }
        }

        public DataModel SelectedData
        {
            get
            {
                return this.dataListBox.SelectedItem as DataModel;
            }
        }

        public void LoadData(IList<DataModel> data)
        {
            this.dataListBox.DataSource = data;
        }

        public void LoadData(DataModel data)
        {
            this.boolTextBox.Text = data.Bool.ToString();
            this.byteTextBox.Text = data.Byte.ToString();
            this.charTextBox.Text = data.Char.ToString();
            this.decimalTextBox.Text = data.Decimal.ToString();
            this.doubleTextBox.Text = data.Double.ToString();
            this.floatTextBox.Text = data.Float.ToString();
            this.intTextBox.Text = data.Int.ToString();
            this.longTextBox.Text = data.Long.ToString();
            this.objectTextBox.Text = data.Object.ToString();
            this.shortTextBox.Text = data.Short.ToString();
            this.stringTextBox.Text = data.String.ToString();
        }

        private void BindComponent()
        {
            this.dataListBox.DisplayMember = "Id";
            this.dataListBox.SelectedIndexChanged +=
                OnDataListBoxSelectedIndexChanged;
        }

        private void OnDataListBoxSelectedIndexChanged(
                                    object sender, EventArgs e)
        {
            if (this.DataSelected != null)
            {
                this.DataSelected();
            }
        }
    }
}

IDataView.cs

using System;
using System.Collections.Generic;

namespace ModelViewPresenter
{
    public interface IDataView
    {
        event Action DataSelected;

        IList<DataModel> Data { get; }
        DataModel SelectedData { get; }

        void LoadData(IList<DataModel> data);

        void LoadData(DataModel data);
    }
}

DataPresenter.cs

namespace ModelViewPresenter
{
    internal class DataPresenter
    {
        private IDataModel model;
        private IDataView view;

        public DataPresenter(IDataModel model, IDataView view)
        {
            this.model = model;
            this.view = view;

            this.view.DataSelected += OnDataSelected;
            this.view.LoadData(model.Data);
        }

        public void OnDataSelected()
        {
            if (this.view.SelectedData != null)
            {
                this.view.LoadData(this.view.SelectedData);
            }
        }
    }
}

Program.cs

using System;
using System.Windows.Forms;

namespace ModelViewPresenter
{
    internal static class Program
    {
        [STAThread]
        private static void Main()
        {
            Application.EnableVisualStyles();
            Application.SetCompatibleTextRenderingDefault(false);
            var model = new DataModel();
            var view = new DataView();
            new DataPresenter(model, view);
            Application.Run(view);
        }
    }
}

Answer 1

RandomExtensions

1. Your RandomExtensions should exist in their own file as they have nothing to do with the DataModel.
2. I don't understand your NextInt32 implementation. Random already has a method to return an integer. I can only assume you are trying to allow the next value to be negative. If so, why the 4 bits or'd with 28 bits?

DataModel

1. Consider having events raised on the data model itself when its properties changed. This allows you to have two different, linked views of the same data model.
2. The Id property has a public setter, but this is probably not your intent.
3. Every time the Data property is queried, a new, random list is returned. Changes to that list are not persisted in the model. It would be better to have the list as a member variable, initialize it in the constructor and return it from the property. If you want to allow the entire list to be replaced, then you also need a setter.
4. The ID assignment in the RandomData factory method should used Interlocked.Increment if you are concerned about thread-safety.
5. You are mixing conversion styles. In RandomExtensions.NextDecimal you create a random byte with (byte)random.Next, but you use Convert.ToByte in your RandomData factory method.
6. There is no default constructor defined, which means any DataModel not initialized with a call to RandomData will not respect the auto-incrementing Id semantics to assign a unique id.

IDataModel

1. You've defined a recursive relationship between your IDataModel and DataModel. IDataModel defines a generic interface to your data model, which returns an explicit list of your data objects that also implement your IDataModel interface. Instead, you should rename IDataModel to be IDataModelCollection, implement this in a new DataModelCollection class, and do not have DataModel implement the IDataModelCollection interface.

IDataView

1. I prefer names like UpdateFrom(DataModel dataModel) instead of LoadData. The view is presenting data, it is not loading data.
2. Your view combines two distinct views of the data: a view of a collection of DataModel items and a view of an individual DataModel. It would be better to create separate views for each of these and, if necessary, a parent view that contains these two subviews.

DataView

1. It would be better to have a UserControl that implements the IDataView interface than to have the form implement it.
2. LoadData(DataModel data) should handle the case when data is null, probably by clearing the view.
3. OnDataListBoxSelectedIndexChanged should allow the DataSelected event to be fired when the selected item is null.

DataPresenter

1. The constructor should ensure the input objects are not null or should have different code paths if they are.
2. There should be a way to detach the presenter from the view's event handler so that both can be cleanly disposed.
3. OnDataSelected should call LoadData even when view.SelectedData is null.

Program

1. You are not persisting the reference to the DataPresenter so you are implicitly relying on the fact that the presenter is kept alive by subscribing to the view's event to keep it around. This is unclean.
2. The presenter should probably be contained within the form itself rather than outside the form.