Permalink
Browse files

Switch to a Visitor pattern

  • Loading branch information...
giuliohome committed Jan 8, 2017
1 parent ac19ffb commit 512e3679f175e487a0b3c726eab8dd6239fa1755
Showing with 123 additions and 63 deletions.
  1. +123 −63 TreeViewModel.cs
@@ -6,20 +6,124 @@
namespace TreeVisitor
{
/// <summary>
/// Description of TreeViewModelcs.
/// An example of functional state mgmt
/// </summary>
public class TreeViewModel: INotifyPropertyChanged
{
/// <summary>
/// Description of TreeViewModelcs.
/// An example of functional state mgmt
/// </summary>
///
abstract class ICutViewVisitor
{
internal abstract void visit(PasteState state);

This comment has been minimized.

@nuttycom

nuttycom Jan 10, 2017

Hi @giuliohome! This looks interesting. My only question is, have you considered the possibility of instead of making these methods void-returning, making the visitor interface polymorphic and then, rather than mutating the internal state of the model, returning a new immutable ICutViewState value? One of the hardest things in object-oriented programing is keeping a good mental model of what state mutable objects are in; if you have the opportunity to make such a model immutable, I'd take it!

This comment has been minimized.

@giuliohome

giuliohome Jan 11, 2017

Owner

Hi @nuttycom ! Well, thank you for reading! At the beginning I had some difficulties with the Visitor, because I'm not practical with it, but then I resolved the initial issue. Now, I think that I understand what you suggest: "returning a new immutable ICutViewState value". Ok, maybe I should try, theoretically it is correct, but really the thing is that this state (as I immagine it now) contains nothing but a reference to the ViewModel... so your point could be translated to "returning a new immutable ViewModel value/instance"... And again, maybe yes, but it involves rethinking the whole class (this is only a little exercise/demo for sure and I have other urgent programs to do at work, anyway let's assume this were a real project, for the sake of the discussion) ... so it's a bit complicated, isn't it? And is it worth it? Agreed, I'll try... just give me some days to close my current priorities :-)

This comment has been minimized.

@nuttycom

nuttycom Jan 11, 2017

I'm happy to help out; I really believe this is an important technique to understand - one of the most important, and least understood, in all of OO programming, but I completely understand time constraints. I warn you though, once you really get familiar with the technique, you may find yourself using it everywhere - I certainly couldn't write correct code without it these days. :)

If ICutViewVisitor is instead ICutViewVisitor<A> then it is easy to write

class CutViewVisitor : ICutViewVisitor<ViewModel>

and then you will just return a new ViewModel from each visit implementation. Even if your state type remains mutable, simply writing state.model = state.accept(new CutViewVisitor()) will be a step in the right direction - it gets mutability out of one level.

This comment has been minimized.

@giuliohome

giuliohome Jan 11, 2017

Owner

Thanks for your help! While I understand the main objective of your suggestion, there are some details that make me think that the complete change is impossible to do. Of course I'm happy if this is a wrong conclusion. The first detail (this one I believe is fixable): the view should "reload" a new View Model according to the MVVM pattern. Let's say I can fix this by adding a new wrapping level to the view bindings (a bit lengthy and not tested yet, but I imagine it could work). Returning a new ViewModel for each visit implementation is the crucial, critical thing. At that point I would not even need a state any longer... because that visitor itself would be the equivalent of the state. But I 'm afraid it's impossible.. let's try. The View Model is a big class with all the view logic and having 3 immutable instances of it for each state makes little sense for all the stateless functions... So there should be an abstract stateless ViewModel and 3 implementations with the state dependent behaviors... But how can I concretely set the next state in this configuration? I don't see an easy answer... maybe a recursive call to the visitor... ? I 'm unsure how to proceed. Do you have a similar wpf example? Anyway thanks, as I promised, I'll go on with this demo next days

This comment has been minimized.

@giuliohome

giuliohome Jan 11, 2017

Owner

Wait! It is not an abstract VM with 3 concrete VMs because all the properties have mutable fields that should be set to their current values... The only solution I can see is that the State should be the group of the state-dependent functionalities of the VM (with an additional state level in the view's binding).
And yes, the visit/accept methods - returning an immutable state - should be the mechanism to set the next state indeed.
Let me quickly check if I can make it work...

This comment has been minimized.

@giuliohome

giuliohome Jan 11, 2017

Owner

New functional state updates committed!

et voilà!!! It is working :-)

@nuttycom I would be happy to know your opinion about my changes! Let me have your review and thanks for your support))

internal abstract void visit(NormalState state);
internal abstract void visit(CutState state);
}
class CutViewVisitor : ICutViewVisitor
{
override internal void visit(PasteState state)
{
state.model.viewState = state.model.normalState;
state.model.Operation = "Select a node to cut";
state.model.HiddenStatus = "Choices";
}
override internal void visit(NormalState state)
{
if (state.model.selectedItem.Name.Equals("Choices"))
{
return;
}
state.model.cutFrom = state.model.selectedItem.Name;
state.model.fromParent = state.model.selectedItem.parent;
state.model.viewState = state.model.cutState;
state.model.Operation = "Select a node to paste to";
}
override internal void visit(CutState state)
{
if (state.model.selectedItem.Name.Equals(state.model.cutFrom))
{
return;
}
state.model.viewState = state.model.pasteState;
state.model.pasteTo = state.model.selectedItem.Name;
if (state.model.fromParent != null)
{
var foundFrom = state.model.fromParent.TreeItems.First(t => t.Name.Equals(state.model.cutFrom));
if (foundFrom == null)
{
state.model.Operation = "Error! From " + state.model.cutFrom + " to " + state.model.pasteTo;
}
else
{
state.model.fromParent.TreeItems.Remove(foundFrom);
state.model.selectedItem.TreeItems.Add(foundFrom);
foundFrom.parent = state.model.selectedItem;
state.model.Operation = "Done! From " + state.model.cutFrom + " to " + state.model.pasteTo;
}
}
else
{
state.model.Operation = "Error! From " + state.model.cutFrom + " to " + state.model.pasteTo;
}
}
}
abstract class ICutViewState
{
abstract internal void accept(ICutViewVisitor visitor);
}
class NormalState : ICutViewState
{
internal TreeViewModel model;
override internal void accept(ICutViewVisitor visitor)
{
visitor.visit(this);
}
}
class CutState : ICutViewState
{
internal TreeViewModel model;
override internal void accept(ICutViewVisitor visitor)
{
visitor.visit(this);
}
}
class PasteState : ICutViewState
{
internal TreeViewModel model;
override internal void accept(ICutViewVisitor visitor)
{
visitor.visit(this);
}
}
public class TreeViewModel: INotifyPropertyChanged
{
public event PropertyChangedEventHandler PropertyChanged;
protected void OnPropertyChanged(string propertyName) {
if (PropertyChanged != null) {
PropertyChanged(this, new PropertyChangedEventArgs(propertyName));
}
}
internal NormalState normalState = new NormalState();
internal CutState cutState = new CutState();
internal PasteState pasteState = new PasteState();
internal CutViewVisitor cutVisitor = new CutViewVisitor();
public TreeViewModel()
{
var root = new TreeItem()
@@ -40,7 +144,10 @@ public TreeViewModel()
root.TreeItems = xTreeItems;
treeItems = new ObservableCollection<TreeItem>();
treeItems.Add(root);
viewState = CutViewState.Normal;
normalState.model = this;
cutState.model = this;
pasteState.model = this;
viewState = normalState;
operation = "Select a node to cut";
}
@@ -70,16 +177,12 @@ public TreeViewModel()
OnPropertyChanged("HiddenStatus");
}
}
private enum CutViewState {
Normal = 0,
Cut = 1,
Paste = 2
}
TreeItem fromParent;
string cutFrom;
string pasteTo;
private CutViewState viewState;
internal TreeItem fromParent;
internal string cutFrom;
internal string pasteTo;
internal ICutViewState viewState;
internal void notifySel() {
@@ -90,51 +193,8 @@ private enum CutViewState {
}
StateText = "Selected: " + selectedItem.Name;
switch (viewState) {
case TreeViewModel.CutViewState.Normal:
if (selectedItem.Name.Equals("Choices") )
{
return;
}
cutFrom = selectedItem.Name;
fromParent = selectedItem.parent;
viewState = CutViewState.Cut;
Operation = "Select a node to paste to";
break;
case TreeViewModel.CutViewState.Cut:
if (selectedItem.Name.Equals(cutFrom)) {
break;
}
viewState = CutViewState.Paste;
pasteTo = selectedItem.Name;
if (fromParent != null )
{
var foundFrom = fromParent.TreeItems.First(t => t.Name.Equals(cutFrom));
if (foundFrom == null)
{
Operation = "Error! From " + cutFrom + " to " + pasteTo;
} else
{
fromParent.TreeItems.Remove(foundFrom);
selectedItem.TreeItems.Add(foundFrom);
foundFrom.parent = selectedItem;
Operation = "Done! From " + cutFrom + " to " + pasteTo;
}
} else
{
Operation = "Error! From " + cutFrom + " to " + pasteTo;
}
break;
case TreeViewModel.CutViewState.Paste:
viewState = CutViewState.Normal;
Operation = "Select a node to cut";
HiddenStatus = "Choices";
break;
default:
throw new Exception("Invalid value for CutViewState");
}
viewState.accept(cutVisitor);
}
internal TreeItem selectedItem;

0 comments on commit 512e367

Please sign in to comment.