Delegates as Properties: Bad Idea?

Question

Consider the following control (snipped for brevity):

public partial class ConfigurationManagerControl : UserControl
{

    public Func<string, bool> CanEdit { get; set;}
    public Func<string, bool> CanDelete { get; set; }

    public Dictionary<string, string> Settings
    {
        get { return InnerSettings; }
        set
        {
            InnerSettings = value;
            BindData();
        }
    }
    private Dictionary<string, string> InnerSettings;

    private void OnListIndexChanged(object sender, EventArgs e)
    {
        this.EditButton.Enabled = false;
        this.DeleteButton.Enabled = false;

        var indices = this.List.SelectedIndices;
        if (indices.Count != 1)
        {
            return;
        }

        var index = indices[0];
        var item = this.List.Items[index];

        if (this.CanEdit != null)
        {
            this.EditButton.Enabled = this.CanEdit(item.Text);
        }

        if (this.CanDelete != null)
        {
            this.DeleteButton.Enabled = this.CanDelete(item.Text);
        }

    }
}

There's more to this control, but suffice it to say that it allows a user to add, edit, and delete the entries in a Dictionary<string, string>. In order to determine whether or not it should allow the user to edit or delete the entries, it uses the delegate method properties, CanDelete and CanEdit, which are provided by the form or control that hosts it:

public class SetupWizard : Form
{
    public SetupWizard()
    {
        InitializeComponent();

        this.SettingManager.CanEdit = CanEditSetting;
        this.SettingManager.CanDelete = CanDeleteSetting;
    }

    private static bool CanEditSetting(string item)
    {
        var lockedSettings = new[] { "LicenseHash", "ProductHash" };
        return !lockedSettings.Contains(item.ToLower());
    }

    private static bool CanDeleteSetting(string item)
    {
        var lockedSettings = new[] {
                                        "LicenseHash",
                                        "ProductHash", 
                                        "UserName", 
                                        "CompanyName"
                                    };
        return !lockedSettings.Contains(item.ToLower());
    }
}

I find that this design is both satisfactory and worrisome at the same time. On the one hand, it seems to solve the problem using the simplest solution that works (it certainly separates the concerns nicely). On the other hand, I have this nagging concern that I am using delegates improperly and should be using an event, instead (even though I do not need multiple listeners, and only need the caller to tell me if the item is editable).

And then, on the other other hand, there's the chance that there's a completely different design that I haven't even considered that might solve the problem in a vastly superior way.

So. Is this design technically correct, maintainable, and flexible? Or should I be doing something better?

Statius

Seems like this question would be better suited for codereview.se. – Tychonn

You should proably take a look at (but not use) RoutedCommands in WPF. If you are on WPF it is a diffferent story... – Dey

Sadly, not on WPF yet. Still on classic WinForms. – Statius

Answer 1

I suggest the use of an interface with these two methods. That's a lot cleaner:

interface ICantThinkOfAGoodName
{
    bool CanEdit(string item);
    bool CanDelete(string item);
}

You could create something similar to the RelayCommand used in many MVVM frameworks:

public class RelayObject : ICantThinkOfAGoodName
{
    public RelayObject() : this(null, null) {}
    public RelayObject(Func<string, bool> canEdit, Func<string, bool> canDelete)
    {
        if(canEdit == null) canEdit = s => true;
        if(canDelete == null) canDelete = s => true;

        _canEdit = canEdit;
        _canDelete = canDelete;
    }

    public bool CanEdit(string item)
    {
        return _canEdit(item);
    }
    public bool CanDelete(string item)
    {
        return _canDelete(item);
    }
}

Use it like this:

public SetupWizard()
{
    InitializeComponent();

    this.SettingManager.PropertyName = new RelayObject(CanEditSetting, 
                                                       CanDeleteSetting);
    // or (all can be deleted)
    this.SettingManager.PropertyName = new RelayObject(CanEditSetting, null);
    // or (all can be edited)
    this.SettingManager.PropertyName = new RelayObject(null, CanDeleteSetting);
    // or (all can be edited and deleted)
    this.SettingManager.PropertyName = new RelayObject();

}

BTW: I am using Property injection here, because it is a control. Normally, I would pass the ICantThinkOfAGoodName dependency in the constructor of the ConfigurationManagerControl.

Monograph

I actually considered that path. The problem then is that I'd have to retrofit any form or control that wanted to host the SettingControl to implement ISettingManager. That didn't seem too optimal to me. The advantage of the separate delegate methods is that the container doesn't have to implement either method if he doesn't need them. If CanDelete isn't there, all items are deletable. Can't get away with that with an interface. – Statius

Incidentally, that doesn't mean I'm right--just why I ruled it out! :) – Statius

@Mike Hofer You could still wrap the delegates into an object to ensure that they are bundled into one place. In your example, if CanDelete isn't there, the button remains disabled. Seems like the defaulting is up to the form and in that case, retrofitting to inherit might not be that bad of an idea. – Theosophy

@Daniel Hilgarth: I have to say, that RelayObject combined with the interface is pretty darned slick. It's the best of both worlds, and satisfies my sense of code minimalism while maintaining separation of concerns. I think we have a winner! – Statius

@MikeHofer: Glad I could help. That's SRP applied :-) – Monograph

Kudos for the design flaw pointed when cited ICantThinkOfAGoodName ! – Haplite

Answer 2

It may be this is what @Daniel Hilgarth is suggesting when he says "use an interface" (n.b. - his answer now reflects a more general/flexible approach to implementing the interface). Instead of assigning delegates to your method directly, why not give the control a property, such as DataState or whatever you want to call it, using an interface that encapsulates the information you need, and leave it up to the owner to decide how to implement that.

interface IDataState
{
    bool CanEdit(string item);
    bool CanDelete(string item);
}

public partial class ConfigurationManagerControl : UserControl
{
    public IDataState DataState {get;set;}
    // your code checks DataState.CanEdit & DataState.CanDelete
}

public class SetupWizard : Form, IDataState
{
    public SetupWizard()
    {
        InitializeComponent();
        SettingManager.DataState =this;
    }
    public bool CanEdit(string item)
    { 
        ... implement directly or return from your private function
    }
    public bool CanDelete(string item)
    { 

    }
}

But this gives you the flexibility to implement that interface any way you choose, with another object, etc. and it makes it easy to also just pass the owner itself (implementing the interface).

Gentlemanatarms