4
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ Your CreateBringToFront action seems pointless. Are you trying to edit the original collection? \$\endgroup\$ – Denis Jun 19 '17 at 23:12
  • \$\begingroup\$ The code creates a temporary collection with the correct order and the last for loop resets the Order property based on the order of the items. \$\endgroup\$ – JanDotNet Jun 20 '17 at 6:07
3
\$\begingroup\$
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.

\$\endgroup\$
  • \$\begingroup\$ Hmm, I think you are right. After reflecting it again, the OOP approach (encapsulate logic in separate classes) seems to fit better even it required more boilerplate code. I am often torn between the succinct functional style and the bloated but more meaningfully OOP style. In the end, I often prefer OOP because that is the way I have learned to think... I am wonder if it just my personal preference or a general one of the human's brain :D. \$\endgroup\$ – JanDotNet Jun 20 '17 at 8:33
  • 2
    \$\begingroup\$ @JanDotNet this is no longer functional because you have statements there. If you encapsulated everything and just do F1().F2(x => x.F3()) etc then it would be funcitonal but the newOrderdItems.AddRange and the for-loop break the pattern. If you had only this query designer.FormElements then I would say it's funcitonal but not properly ecapsulated, currently it's a mix of both. I think it's not just you. Somehow it's hard do see the flaws in your own code like my stupid dictionary for grouping. If I saw this by someone else I would screem haha ;-) \$\endgroup\$ – t3chb0t Jun 20 '17 at 8:41
  • \$\begingroup\$ @JanDotNet I find It's ok to mix both styles but it needs be clean. \$\endgroup\$ – t3chb0t Jun 20 '17 at 8:41
2
\$\begingroup\$

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());
}
\$\endgroup\$
  • \$\begingroup\$ Looks much cleaner now and is really easy to follow but why are the sorters nested classes? \$\endgroup\$ – t3chb0t Jun 20 '17 at 11:20
  • \$\begingroup\$ To hide them ;) Its internal stuff that should not be visible to the rest of the code. \$\endgroup\$ – JanDotNet Jun 20 '17 at 11:26
  • \$\begingroup\$ It worked, I almost didn't find them :-P put them in a namespace like Sorters. Each class should have its own file (in most cases) - there are of course exceptions but this isn't one of them ;-] \$\endgroup\$ – t3chb0t Jun 20 '17 at 11:31
  • \$\begingroup\$ The project has enough files and the classes are not used outside the ArrangeOperation class. Therefore I don't see a reason to put them in its own files / namespace. There is also a special "magic key" called F12 that can be used to find them ;P \$\endgroup\$ – JanDotNet Jun 20 '17 at 11:52

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.