Finding Magic Comments to Create a Custom Task List

Question

I've been working on a COM Add-in for the VBA IDE with a friend. The idea is to search the active VBA Project for "magic" comments and create a task list much like any sane IDE would have.

This is my first "real" project in C#, so I'm feeling like this code could be better. The code below is all in the code behind of my TodoListControl, which I don't much care for. I was told that using a Model View Presenter pattern would help me out with that, but I don't see how I can decouple the control from the Module object that it parses and uses to jump to a specific line of code on double click. I'm also not very happy with the private TryGetMarker method. I just can't shake the feeling that there's a better way to return a marker.

The control gets embedded into a native window and looks like this.

For context purposes, I've included some of the other classes. ToDoItem is an item in the binding list for the gridview. It includes a reference to the code module where the "magic" comment has been found. ToDoMarker is an XML Deserialized class that retrieves the text to search for and the priority level of the marker.

ToDoListConrol

using System;
using System.ComponentModel;
using System.Linq;
using System.Windows.Forms;
using Microsoft.Vbe.Interop;
using System.Collections.Generic;

namespace Rubberduck.ToDoItems
{
    public partial class ToDoItemsControl : UserControl
    {
        private VBE vbe;
        private BindingList<ToDoItem> taskList;
        private List<Config.ToDoMarker> markers;

        public ToDoItemsControl(VBE vbe, List<Config.ToDoMarker> markers)
        {
            this.vbe = vbe;
            this.markers = markers;

            InitializeComponent();

            RefreshTaskList();
            InitializeGrid();

        }

        private void InitializeGrid()
        {
            todoItemsGridView.DataSource = taskList;
            var descriptionColumn = todoItemsGridView.Columns["Description"];
            if (descriptionColumn != null)
            {
                descriptionColumn.AutoSizeMode = DataGridViewAutoSizeColumnMode.Fill;
            }

            todoItemsGridView.CellDoubleClick += taskListGridView_CellDoubleClick;
            refreshButton.Click += refreshButton_Click;

        }

        void taskListGridView_CellDoubleClick(object sender, DataGridViewCellEventArgs e)
        {
            ToDoItem task = taskList.ElementAt(e.RowIndex);
            VBComponent component = vbe.ActiveVBProject.VBComponents.Item(task.Module);

            component.CodeModule.CodePane.Show();
            component.CodeModule.CodePane.SetSelection(task.LineNumber, 1, task.LineNumber, 1);
        }

        private void RefreshGridView()
        {
            RefreshTaskList();
            todoItemsGridView.DataSource = taskList;
            todoItemsGridView.Refresh();
        }

        public void RefreshTaskList()
        {
            this.taskList = new BindingList<ToDoItem>();

            foreach (VBComponent component in this.vbe.ActiveVBProject.VBComponents)
            {
                CodeModule module = component.CodeModule;
                for (var i = 1; i <= module.CountOfLines; i++)
                {
                    string line = module.get_Lines(i, 1);
                    Config.ToDoMarker marker;

                    if (TryGetMarker(line, out marker))
                    {
                        var priority = (TaskPriority)marker.priority;
                        this.taskList.Add(new ToDoItem(priority, line, module, i));
                    }
                }
            }
        }

        private bool TryGetMarker(string line, out Config.ToDoMarker result)
        {
            var upCasedLine = line.ToUpper();
            foreach (var marker in this.markers)
            {
                if (upCasedLine.Contains(marker.text))
                {
                    result = marker;
                    return true;
                }
            }
            result = null;
            return false;
        }

        private void refreshButton_Click(object sender, EventArgs e)
        {
            RefreshGridView();
        }
    }
}

ToDoItem

using Microsoft.Vbe.Interop;

namespace Rubberduck.ToDoItems
{
    internal enum TaskPriority
    {
        Low,
        Medium,
        High
    }

    internal class ToDoItem
    {
        public TaskPriority Priority{ get; set; }
        public string Description { get; set; }
        public string Module { get; set; } 
        public int LineNumber { get; set; }

        public ToDoItem(TaskPriority priority, string description, CodeModule module,  int lineNumber)
        {
            this.Priority = priority;
            this.Description = description.Trim();
            this.Module = module.Parent.Name;
            this.LineNumber = lineNumber;
        }
    }
}

ToDoMarker

using System.Xml.Serialization;

namespace Rubberduck.Config
{
    [System.Runtime.InteropServices.ComVisible(false)]
    [XmlTypeAttribute(AnonymousType = true)]
    public class ToDoListSettings
    {

        [XmlArrayItemAttribute("ToDoMarker", IsNullable = false)]
        public ToDoMarker[] ToDoMarkers { get; set; }
    }

    [System.Runtime.InteropServices.ComVisible(false)]
    [XmlTypeAttribute(AnonymousType = true)]
    public class ToDoMarker
    {
        [XmlAttributeAttribute()]
        public string text { get; set; }

        [XmlAttributeAttribute()]
        public byte priority { get; set; }

        public ToDoMarker() { }

        public ToDoMarker(string text, byte priority)
        {
            this.text = text;
            this.priority = priority;
        }
    }
}

Answer 1

I was told that using a Model View Presenter pattern would help me out with that, but I don't see how I can decouple the control from the Module object that it parses and uses to jump to a specific line of code on double click.

The view is already decoupled: it's in the todoItemsGridView and refreshButton fields that are just magically there in your class, ready to stick event listeners on.

To decouple the model, I think you'd have to extract RefreshTaskList (and consequently TryGetMarker) to a different class dedicated to manage the task list, including the logic of parsing lines for TODO markers.

I'm also not very happy with the private TryGetMarker method. I just can't shake the feeling that there's a better way to return a marker.

It seems quite fine to me, though maybe partly because I've seen you do this in other answers.

In this particular example, it might be better to change the method to GetMarker, make it return ToDoMarker or null if not found. Then the caller would change to a null check instead of a boolean check. The implementation will be shorter and just as good.

---

This is not great:

var priority = (TaskPriority)marker.priority;

Since the available markers don't change during runtime, it feels odd to cast again and again. More importantly, if the config file contains an invalid value, you will get an error when a marker is used, which is a bit late. I would prefer to get errors as soon as possible: if not at compile time, then during startup at runtime.

A solution to correct both of these is to wrap the parsing of the configuration in a cleaned object that performs type checking. This way if something's wrong you'll get an error during startup, and you can eliminate the ugly cast in the middle of your model's logic.

---

upCasedLine.Contains(marker.text)

This assumes that marker.text is always uppercase. But there's nothing in the code to guarantee that, and such, it will have to depend on good documentation that tells users to use uppercase names. It might be a good idea to uppercase marker.text. Again, this could be done in the config wrapper I mentioned above, to perform only once during startup.