3
\$\begingroup\$

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);
        }
    }
}

enter image description here

\$\endgroup\$
  • 1
    \$\begingroup\$ Just curious, why Winforms instead of WPF? \$\endgroup\$ – user2023861 Jun 7 '16 at 13:50
  • \$\begingroup\$ @user2023861 Never looked into WPF actually. Why would you recommend it? \$\endgroup\$ – Michael Brandon Morris Jun 7 '16 at 13:57
  • 2
    \$\begingroup\$ WPF is newer. It came out around 2006. Winforms is older than that (though I'm having trouble finding a date). If you're just learning front-end programming, you might as well start with what's newer and well-established. From a few years of working with WPF, I've found that it reinforces good design practices. Specifically it works great with the MVVM pattern (similar to MVP). Other features like its ICommand and IValueConverter interfaces have helped me better understand the single-responsibility principle (S in SOLID) \$\endgroup\$ – user2023861 Jun 7 '16 at 14:04
3
+50
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ I read on SO that Random.Next() does not return an integer that actually goes up to the maximum integer value, whereas the NextInt32() does. \$\endgroup\$ – Michael Brandon Morris Jun 9 '16 at 21:47
  • \$\begingroup\$ Program 1: The presenter is created in the main method, so it should live until the application terminates, should it not? Program 2: I thought the point of MVP was separating the classes so that they were not (directly) aware of each other. DataPresenter 1: The objects should never be null. The only alternate code path I see as reasonable would be to have the program throw an exception if they are. DataPresenter 2: Could you elaborate more on this? DataPresenter 3: What is the point of it trying to load something that is null when it can rule such behavior out earlier? \$\endgroup\$ – Michael Brandon Morris Jun 9 '16 at 23:08
  • \$\begingroup\$ @MichaelBrandonMorris Program 1: The only reference to the DataPresenter object that is keeping it from being garbage collected is the event handler registered on the view's DataSelected object. It's cleaner to have an explicit object reference that is in explicitly in the Main function's scope. \$\endgroup\$ – Eric Jun 9 '16 at 23:12
  • \$\begingroup\$ @MichaelBrandonMorris Program 2: My point is that the form itself should not be a view. Instead, it should contain user controls that are the views. My take is that it should receive a model to present and then create / configure the presenters to handle that appropriately using its views. \$\endgroup\$ – Eric Jun 9 '16 at 23:14
  • \$\begingroup\$ @MichaelBrandonMorris DataPresenter 1: The thought that objects should never be null implies you would never have an "empty" view with no data. This may be the case now, but you may change your application in the future to allow this. It's safer to handle this explicitly within the view by either blanking the view or throwing an exception than it is to rely on logic outside the view to always use the view as you currently expect it. \$\endgroup\$ – Eric Jun 9 '16 at 23:16

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.