# Implementation of undoable arrange (SendToBack, SendBackward, …) operations

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 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();
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();

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?

• Your CreateBringToFront action seems pointless. Are you trying to edit the original collection? – Denis Jun 19 '17 at 23:12
• 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. – JanDotNet Jun 20 '17 at 6:07

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();

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.

• 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. – JanDotNet Jun 20 '17 at 8:33
• @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 ;-) – t3chb0t Jun 20 '17 at 8:41
• @JanDotNet I find It's ok to mix both styles but it needs be clean. – t3chb0t Jun 20 '17 at 8:41

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 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());
}

• Looks much cleaner now and is really easy to follow but why are the sorters nested classes? – t3chb0t Jun 20 '17 at 11:20
• To hide them ;) Its internal stuff that should not be visible to the rest of the code. – JanDotNet Jun 20 '17 at 11:26
• 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 ;-] – t3chb0t Jun 20 '17 at 11:31
• 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 – JanDotNet Jun 20 '17 at 11:52