Simple dictionary storing/viewing application

Question

A TreeView on the left which just displays a list of files saved by the application. A DataGridViewon the right to display data from a file clicked in the TreeView, or to type directly into to create a new file. Files in the application are just Dictionary<string,string> objects written to disk with a BinaryFormatter.

IMappingsView.cs

using System;
using System.Data;
using System.Windows.Forms;

namespace Mapper.Interfaces
{
    public interface IMappingsView
    {
        DataTable MappingData { get; set; }
        TreeNodeCollection TreeNodes { get; }
        TreeNode SelectedNode { get; }

        event EventHandler SaveMapping;
        event EventHandler NewMapping;
        event EventHandler DeleteMapping;
        event EventHandler PasteData;
        event EventHandler NodeClicked;

        string GetFilename();
        void ExpandTreeView();
    }
}

MappingsView.cs

using Mapper.Interfaces;
using System;
using System.Data;
using System.Windows.Forms;

namespace Mapper
{
    public partial class MappingsView : Form, IMappingsView
    {
        public event EventHandler SaveMapping;
        public event EventHandler NewMapping;
        public event EventHandler DeleteMapping;
        public event EventHandler PasteData;
        public event EventHandler NodeClicked;

        public DataTable MappingData
        {
            get { return (DataTable)dataGridView1.DataSource; }
            set { dataGridView1.DataSource = value; }
        }

        public TreeNodeCollection TreeNodes
        {
            get { return treeView1.Nodes; }
        }

        public TreeNode SelectedNode
        {
            get { return treeView1.SelectedNode; }
        }

        public MappingsView()
        {
           InitializeComponent();
        }

        private void buttonNewMapping_Click(object sender, EventArgs e)
        {
            if (NewMapping != null)
            {
                NewMapping(this, EventArgs.Empty);
            }
        }

        private void buttonSaveMapping_Click(object sender, EventArgs e)
        {
            if (SaveMapping != null)
            {
                SaveMapping(this, EventArgs.Empty);
            }
        }

        private void buttonDeleteMapping_Click(object sender, EventArgs e)
        {
            if (DeleteMapping != null)
            {
                DeleteMapping(this, EventArgs.Empty);
            }
        }

        private void buttonPaste_Click(object sender, EventArgs e)
        {
            if (PasteData != null)
            {
                PasteData(this, EventArgs.Empty);
            }
        }

        private void treeView1_AfterSelect(object sender, TreeViewEventArgs e)
        {
            if (NodeClicked != null)
            {
                NodeClicked(this, EventArgs.Empty);
            }
        }

        public string GetFilename()
        {
            var input = new InputBox();
            input.StartPosition = FormStartPosition.CenterParent;
            input.ShowDialog();

            return input.MappingName;
        }

        public void ExpandTreeView()
        {
            treeView1.ExpandAll();
        }
    }
}

IMappingModel.cs

using System.Collections.Generic;

namespace Mapper.Interfaces
{
    public interface IMappingModel
    {
        Dictionary<string, string> mapping { get;  set; }
    }
}

MappingModel.cs

using Mapper.Interfaces;
using System.Collections.Generic;

namespace Mapper
{
    public class MappingModel : IMappingModel
    {
        public Dictionary<string, string> mapping { get; set; }

        public MappingModel(Dictionary<string, string> mapping)
        {
            this.mapping = mapping;
        }
    }
}

MappingPresenter.cs

using Mapper.Interfaces;
using System;
using System.Collections.Generic;
using System.Data;
using System.IO;
using System.Text.RegularExpressions;
using System.Windows.Forms;

namespace Mapper
{
    class MappingPresenter
    {
        private const string RootDir = @"C:\";
        private IMappingsView view;
        private bool isNewMapping;

        public MappingPresenter(IMappingsView view)
        {
            this.view = view;

            isNewMapping = true;
            AssignEventHandlers();
            RefreshTreeView();
        }

        private void AssignEventHandlers()
        {
            view.DeleteMapping += view_DeleteMapping;
            view.NodeClicked += view_NodeClicked;
            view.NewMapping += view_NewMapping;
            view.SaveMapping += view_SaveMapping;
            view.PasteData += view_PasteData;
        }

        private void RefreshTreeView()
        {
            var rootDirInfo = new DirectoryInfo(RootDir);

            view.TreeNodes.Clear();
            view.TreeNodes.Add(CreateDirectoryNode(rootDirInfo));
            view.ExpandTreeView();
        }

        private void ClearDisplay()
        {
            view.MappingData = null;
        }

        private DataTable CreateEmptyTable()
        {
            var dt = new DataTable("MappingData");
            dt.Columns.Add("Key", typeof(string));
            dt.Columns.Add("Value", typeof(string));

            return dt;
        }

        private TreeNode CreateDirectoryNode(DirectoryInfo directoryInfo)
        {
            var directoryNode = new TreeNode(directoryInfo.Name);
            foreach (var directory in directoryInfo.GetDirectories())
                directoryNode.Nodes.Add(CreateDirectoryNode(directory));
            foreach (var file in directoryInfo.GetFiles())
                directoryNode.Nodes.Add(new TreeNode(file.Name));

            return directoryNode;
        }

        private IMappingModel LoadMapping(string path)
        {
            var mapping = new Dictionary<string, string>();

            using (var fs = File.OpenRead(path))
            {
                using (var br = new BinaryReader(fs))
                {
                    int count = br.ReadInt32();

                    for (int i = 0; i < count; i++)
                    {
                        string key = br.ReadString();
                        string val = br.ReadString();
                        mapping[key] = val;
                    }
                }
            }

            return new MappingModel(mapping);
        }

        private void SaveMapping(IMappingModel mappingModel,string path)
        {
            using (var fs = File.OpenWrite(path))
            {
                using (var bw = new BinaryWriter(fs))
                {
                    bw.Write(mappingModel.mapping.Count);

                    foreach (var pair in mappingModel.mapping)
                    {
                        bw.Write(pair.Key);
                        bw.Write(pair.Value);
                    }
                }
            }
        }

        void view_SaveMapping(object sender, EventArgs e)
        {
            var mapping = new Dictionary<string, string>();

            foreach (DataRow row in view.MappingData.Rows)
            {
                var key = row[0].ToString();
                var val = row[1].ToString();

                if (!key.Equals(String.Empty) && !val.Equals(String.Empty))
                {
                    mapping.Add(key, val);
                }
            }

            var path = String.Empty;

            if (isNewMapping)
            {
                var filename = view.GetFilename();

                if (filename.Equals(String.Empty))
                {
                    filename = String.Format("mapping{0}", DateTime.Now.ToString("MMddhhmmss"));
                }

                path = Path.Combine(RootDir, String.Format("{0}.bin", filename));
            }
            else
            {
                path = view.SelectedMappingPath;
            }

            SaveMapping(new MappingModel(mapping),path);
            RefreshTreeView();
        }

        void view_NewMapping(object sender, EventArgs e)
        {
            var dt = CreateEmptyTable();
            view.MappingData = dt;
            isNewMapping = true;
        }

        void view_NodeClicked(object sender, EventArgs e)
        {
            if (view.SelectedNode.Text.EndsWith(".bin"))
            {
                var path = Path.Combine(RootDir, view.SelectedNode.Text);
                IMappingModel mappingModel = LoadMapping(path);

                var dt = CreateEmptyTable();

                foreach (var pair in mappingModel.mapping)
                {
                    var row = dt.NewRow();
                    row[0] = pair.Key;
                    row[1] = pair.Value;
                    dt.Rows.Add(row);
                }

                view.MappingData = dt;
                isNewMapping = false;
            }
        }

        void view_DeleteMapping(object sender, EventArgs e)
        {
            if (view.SelectedNode.Text.EndsWith(".bin"))
            {
                File.Delete(Path.Combine(RootDir, view.SelectedNode.Text));
                ClearDisplay();
                RefreshTreeView();
            }
        }

        void view_PasteData(object sender, EventArgs e)
        {
            DataObject o = (DataObject)Clipboard.GetDataObject();

            if (o.GetDataPresent(DataFormats.Text))
            {
                var pastedRows = Regex.Split(o.GetData(DataFormats.Text).ToString().TrimEnd("\r\n".ToCharArray()), "\r\n");
                var dt = CreateEmptyTable();

                foreach (string pastedRow in pastedRows)
                {
                    var pastedRowCells = pastedRow.Split(new char[] { '\t' });
                    var row = dt.NewRow();

                    for (int i = 0; i < pastedRowCells.Length; i++)
                    {
                        row[i] = pastedRowCells[i];
                    }

                    dt.Rows.Add(row);
                }

                view.MappingData = dt;
            }
        }
    }
}

Program.cs

using System;
using System.Windows.Forms;

namespace Mapper
{
    static class Program
    {
        [STAThread]
        static void Main()
        {
            Application.EnableVisualStyles();
            Application.SetCompatibleTextRenderingDefault(false);

            MappingsView view = new MappingsView();
            var presenter = new MappingPresenter(view);

            Application.Run(view);
        }
    }
}

I welcome all feedback/criticisims/suggestions, but I wasn't sure about some specific items:

1. Where should constants be stored? The one I have for this application is in MappingPresenter.cs.

2. Should MappingModel know how to Save and Load itself? I had it like this originally but wasn't sure if the Presenter was supposed to manage that.

   2a. Should the code in SaveMapping and LoadMapping be in the event handlers instead?

3. Do I need to have an IMappingModel in the Presenter?
4. What is the correct way to start the application from Program.cs as what I have doesn't seem correct?
5. Should methods like RefreshTreeView() and ClearDisplay() be public methods on the view?
6. Is having a propery (e.g. TreeNode SelectedNode, TreeNodeCollection TreeNodes, DataTable MappingData) the best practices method for getting/setting data from controls on the view? I'm guessing you don't want to expose the whole control in the interface?
7. Is it wrong to have private members in the Presenter (e.g. isNewMapping)? If so, where would you manage something like that?
8. What is the best practice way for displaying another view? (InputBox in this example) Having a public method on the IMappingsView was the only way I could see to work it for positioning.

Answer 1

It's hard to review good code, and this is relatively good code. Things are generally well named and no methods are overly long. You've done well, but there is room for improvement.

Let's start at the entry point, and this question.

What is the correct way to start the application from Program.cs as what I have doesn't seem correct?

Nope. This is perfectly good. I wouldn't change a thing about Program.cs. You've started up your MVP architecture correctly. Once it grow beyond this one feature and dependencies start piling up, you may want to look into using an IoC container to create your dependency graph and inject it into your presenter, but the current size of your project doesn't warrant it.

You've done really well at the MVP architecture, but you do have some leaky abstractions.

public DataTable MappingData
{
    get { return (DataTable)dataGridView1.DataSource; }
    set { dataGridView1.DataSource = value; }
}

Returning a DataTable out from your view tightly couples your presenter to this particular view and architecture. All in all, not too bad, but leaking the TreeView details into your presenter is much worse. Returning a DataTable won't stop you from creating a WebForms web page that uses the same presenter, but leaking the TreeView into your presenter will. One of the big benefits of MVP (other than test-ability, which I'll get to) is the ability to use the same business logic on desktop & web. You can't do that because you've leaked details of your UI layer into the business logic. I don't have to time to figure out how exactly to encapsulate the tree view logic in your case, but it is a view concern and should stay in your view. The DataTable issue is typically fixed by using a BindingList of [your data model] in the view interface definition.

Like I was saying before, one of the other benefits we get from MVP is test-ability or, at least, we should get those benefits, but you're not at the moment. This is because you're accessing UI elements directly (TreeNodeCollection) and writing to the file system directly from your presenter. I would take a bit of time to create a new class and interface for writing/reading from the file system. This would decouple you enough that you could write automated tests against your presenter. Bonus, it would let you swap out implementations so you could write to a database instead, if you choose to later.

Which brings me back to

Where should constants be stored?

Considering your constants seem to be related to writing those files, move those constants to your new FooBarRepository class and you'll have no more worries that they're in the wrong place.

Speaking of repositories...

Should MappingModel know how to Save and Load itself? I had it like this originally but wasn't sure if the Presenter was supposed to manage that.

This is a touchy subject. I personally don't like models that know how to save and load themselves. It feels like too many responsibilities to me, but I know many developers who prefer models that do have this responsibility. It may make some sense for MVVM architectured applications, but I'm not entirely convinced of that either. Anyway, it's kind of a holy war and I'll let you make up your own mind, but I prefer the Repository Pattern when working with MVP. I'd recommend looking into it.