Implementation of undoable arrange (SendToBack, SendBackward, ...) operations

Question

I am developing a WPF designer where visual elements (rectangles, ellipses, ...) can be inserted, moved, resized and so on. Similar to a simple PowerPoint control.

All the actions should be undo-able, so all modifications to the drawing area's state are implemented as Operations. An operation looks like:

public abstract class Operation
{
    protected Operation(string title) 
    {
        Title = title;
    }

    internal abstract void Do();
    internal abstract void Undo();
    public string Title { get; }
}

There are other classes used in the code below:

• The IFormDesigner interface is the abstraction of the desinger surface
• The FormItem class represents the model for one element on the designer and has (beside some other properties) an Order property of type Int32 which determines the element's Z order.

I would like you to take a look on the arrange operation (SendToBack, SendBackward, BringToFront, BringForward). My first approach was, to implement a class ArrangeOperation with an abstract method CalculateNewOrder followed by 4 derived classes (SendToBack, SendBackward, BringToFront, BringForward) that implemented the abstract method. Because the method was called in contructor, I ran into ReSharper's virtual member call in constructor warning. Actually, the warning wasn't a problem in my case, but I want to avoid it anyway because it may become a problem if the code grows...

Finally I end up with a more functional solution:

internal class ArrangeOperation : Operation
{
    private readonly IFormDesigner myDesigner;
    private readonly Dictionary<FormItem, int> myOrdersBefore;
    private readonly Dictionary<FormItem, int> myOrdersAfter;

    private ArrangeOperation(IFormDesigner designer, string title, Action calculateNewOrder) : base(title)
    {
        myDesigner = designer;
        myOrdersBefore = designer.FormElements.ToDictionary(e => e.Model, e => e.Model.Order);
        calculateNewOrder();
        myOrdersAfter = designer.FormElements.ToDictionary(e => e.Model, e => e.Model.Order);
    }

    internal override void Do()
    {
        ApplyOrders(myOrdersAfter);
        myDesigner.InvalidateOrders();
    }

    internal override void Undo()
    {
        ApplyOrders(myOrdersBefore);
        myDesigner.InvalidateOrders();
    }

    private static void ApplyOrders(Dictionary<FormItem, int> elementOrderDictionary)
    {
        foreach (var element in elementOrderDictionary)
            element.Key.Order = element.Value;
    }

    public static Operation CreateSendToBack(IFormDesigner designer)
    {
        var formItems = designer.Selection.Select(i => i.Model).ToArray();
        return new ArrangeOperation(designer, Resources.OperationSendToBack,
            () =>
            {
                var otherItems = designer.FormElements
                    .Where(i => !formItems.Contains(i.Model))
                    .OrderBy(i => i.Model.Order)
                    .Select(f => f.Model)
                    .ToArray();

                var newOrderdItems = formItems.OrderBy(i => i.Order).ToList();
                newOrderdItems.AddRange(otherItems);
                for (int i = 0; i < newOrderdItems.Count; i++)
                    newOrderdItems[i].Order = i + 1;
            });
    }

    public static Operation CreateSendBackward(IFormDesigner designer)
    {
        var formItems = designer.Selection.Select(i => i.Model).ToArray();
        return new ArrangeOperation(designer, Resources.OperationSendBackward,
            () =>
            {
                var allFormItems = designer.FormElements.Select(f => f.Model).ToArray();
                for (int i = 1; i < allFormItems.Length; i++)
                {
                    if (formItems.Contains(allFormItems[i]))
                        allFormItems.Swap(i, i - 1);
                }
                for (int i = 0; i < allFormItems.Length; i++)
                    allFormItems[i].Order = i + 1;
            });
    }

    public static Operation CreateBringToFront(IFormDesigner designer)
    {
        var formItems = designer.Selection.Select(i => i.Model).ToArray();
        return new ArrangeOperation(designer, Resources.OperationBringToFront,
            () =>
            {
                var newOrderdItems = designer.FormElements
                    .Where(i => !formItems.Contains(i.Model))
                    .OrderBy(i => i.Model.Order)
                    .Select(f => f.Model).ToList();

                newOrderdItems.AddRange(formItems.OrderBy(i => i.Order));

                for (int i = 0; i < newOrderdItems.Count; i++)
                    newOrderdItems[i].Order = i + 1;
            });
    }

    public static Operation CreateBringForward(IFormDesigner designer)
    {
        var formItems = designer.Selection.Select(i => i.Model).ToArray();
        return new ArrangeOperation(designer, Resources.OperationBringForward,
            () =>
            {
                var allFormItems = designer.FormElements.Select(f => f.Model).ToArray();
                for (int i = allFormItems.Length - 2; i >= 0; i--)
                {
                    if (formItems.Contains(allFormItems[i]))
                        allFormItems.Swap(i, i + 1);
                }
                for (int i = 0; i < allFormItems.Length; i++)
                    allFormItems[i].Order = i + 1;
            });
    }
}

What do you think about the class design and the the logic for calculating the new order?

Answer 1

public static Operation CreateBringToFront(IFormDesigner designer)
{
    var formItems = designer.Selection.Select(i => i.Model).ToArray();
    return new ArrangeOperation(designer, Resources.OperationBringToFront,
        () =>
        {
            var newOrderdItems = designer.FormElements
                .Where(i => !formItems.Contains(i.Model))
                .OrderBy(i => i.Model.Order)
                .Select(f => f.Model).ToList();

            newOrderdItems.AddRange(formItems.OrderBy(i => i.Order));

            for (int i = 0; i < newOrderdItems.Count; i++)
                newOrderdItems[i].Order = i + 1;
        });
}

I like lambdas very much but putting this much code into a constructor seem like way too much. In all four cases you should encpasulate them in separate funcitons/classes and pass them to the constructor like a strategy pattern. This would make the logic easier to test (I guess) without having to be a part of an ArrangeOperation.

You already pass the designer to the constructor so there's no need for closures here. The Action could take one more parameter to work with later.

Answer 2

As suggested from t3chb0t, I refactored the code to a more OOP style solution which seems to fit better :):

internal class ArrangeOperation : Operation
{
    private abstract class Sorter
    {
        public void Sort(IFormDesigner designer)
        {
            var selectedItems  = designer.Selection.Select(s => s.Model).ToArray();
            var sortedItems = GetSortedItems(designer, selectedItems);
            for (int i = 0; i < sortedItems.Count; i++)
                sortedItems[i].Order = i + 1;
        }
        protected abstract List<FormItem> GetSortedItems(IFormDesigner designer, FormItem[] selectedItems);
    }

    private class SendToBackSorter : Sorter
    {
        protected override List<FormItem> GetSortedItems(IFormDesigner designer, FormItem[] selectedItems)
        {
            var otherItems = designer.FormElements
                .Where(i => !selectedItems.Contains(i.Model))
                .OrderBy(i => i.Model.Order)
                .Select(f => f.Model)
                .ToArray();

            return selectedItems
                .OrderBy(i => i.Order)
                .Concat(otherItems)
                .ToList();
        }
    }

    private class SendBackwardSorter : Sorter
    {
        protected override List<FormItem> GetSortedItems(IFormDesigner designer, FormItem[] selectedItems)
        {
            var allFormItems = designer.FormElements.Select(f => f.Model).ToList();
            for (int i = 1; i < allFormItems.Count; i++)
                if (selectedItems.Contains(allFormItems[i]))
                    allFormItems.Swap(i, i - 1);
            return allFormItems;
        }
    }

    private class BringToFrontSorter : Sorter
    {
        protected override List<FormItem> GetSortedItems(IFormDesigner designer, FormItem[] selectedItems)
        {
            return designer.FormElements
                .Where(i => !selectedItems.Contains(i.Model))
                .OrderBy(i => i.Model.Order)
                .Select(f => f.Model)
                .Concat(selectedItems.OrderBy(i => i.Order))
                .ToList();
        }
    }

    private class BringForwardSorter : Sorter
    {
        protected override List<FormItem> GetSortedItems(IFormDesigner designer, FormItem[] selectedItems)
        {
            var allFormItems = designer.FormElements.Select(f => f.Model).ToList();
            for (int i = allFormItems.Count - 2; i >= 0; i--)
                if (selectedItems.Contains(allFormItems[i]))
                    allFormItems.Swap(i, i + 1);
            return allFormItems;
        }
    }

    private readonly IFormDesigner myDesigner;
    private readonly Dictionary<FormItem, int> myOrdersBefore;
    private readonly Dictionary<FormItem, int> myOrdersAfter;

    private ArrangeOperation(IFormDesigner designer, string title, Sorter sorter) : base(title)
    {
        myDesigner = designer;
        myOrdersBefore = designer.FormElements.ToDictionary(e => e.Model, e => e.Model.Order);
        sorter.Sort(designer);
        myOrdersAfter = designer.FormElements.ToDictionary(e => e.Model, e => e.Model.Order);
    }

    internal override void Do()
    {
        ApplyOrders(myOrdersAfter);
        myDesigner.InvalidateOrders();
    }

    internal override void Undo()
    {
        ApplyOrders(myOrdersBefore);
        myDesigner.InvalidateOrders();
    }

    private static void ApplyOrders(Dictionary<FormItem, int> elementOrderDictionary)
    {
        foreach (var element in elementOrderDictionary)
            element.Key.Order = element.Value;
    }

    public static Operation CreateSendToBack(IFormDesigner designer)
        => new ArrangeOperation(designer, Resources.OperationSendToBack, new SendToBackSorter());

    public static Operation CreateSendBackward(IFormDesigner designer)
        => new ArrangeOperation(designer, Resources.OperationSendBackward, new SendBackwardSorter());

    public static Operation CreateBringToFront(IFormDesigner designer)
        => new ArrangeOperation(designer, Resources.OperationBringToFront, new BringToFrontSorter());

    public static Operation CreateBringForward(IFormDesigner designer)
        => new ArrangeOperation(designer, Resources.OperationBringForward, new BringForwardSorter());
}