Checkers Board Editor

I created a board editor for my Checkers game. This allows the user to create any board position and copy the FEN string representing that board, which can then be used to create a game from that position:

I think this is pretty good code, but I'm not sure if the way I communicate with the view model from the view is OK, and if that can be cleaned up at all.

This is the XAML:

    
        
        
    
    
        
            
            
        
        
            
            
            
        
        
            
        
        
            
            
            
                
            
        
        
            
                
                
                
            
            
                
                
            
            
                
                
            
            
                
                
            
        
        
        
            
                
                
                
                
            
            
                
                Black Checker
            
            
                
                Black King
            
            
                
                White Checker
            
            
                
                White King
            
        
    

And this is the code behind that manages the dragging and dropping:

```
public sealed partial class BoardEditor
{
private Image _draggedImage;
private Piece _piece;
private readonly ApplicationDataContainer _roamingSettings = ApplicationData.Current.RoamingSettings;

private BoardEditorViewModel ViewModel => (BoardEditorViewModel)DataContext;

public BoardEditor()
{
InitializeComponent();

_currentTheme = (string)_roamingSettings.Values["Theme"];
ApplicationData.Current.DataChanged += Current_DataChanged;
LoadImages();
}

private string _currentTheme;
private void Current_DataChanged(Applica

Code Behind

LoadImages

Load images can be shortened easily without losing any of the meaning.

private void LoadImages()
{
    WhiteChecker.Source = new BitmapImage(GetPieceUri(Piece.WhiteChecker));
    WhiteKing.Source = new BitmapImage(GetPieceUri(Piece.WhiteKing));
    BlackChecker.Source = new BitmapImage(GetPieceUri(Piece.BlackChecker));
    BlackKing.Source = new BitmapImage(GetPieceUri(Piece.BlackKing));
}

GetPieceUrl

Can be made simpler by creating a data-oriented mapping from Piece to image:

IDictionary pieceImageFilenames = new Dictionary()
{
  {Piece.WhiteChecker, "WhiteChecker.png"},
  {Piece.WhiteKing, "WhiteKing.png"},
  //etc.
}

private Uri GetPieceUri(Piece piece)
{
    if (piece == null) { return null; }

    return new Uri($"ms-appx:///Assets/{_roamingSettings.Values["Theme"]}Theme/{pieceImageFilenames[piece]}", UriKind.Absolute);
}

This will throw a KeyNotFoundException if no matching key is found, which makes more sense than a MissingMemberException which is dynamically accessing class members, and not for record retrieval problems.

Alternatively, don't be afraid to make your own exceptions. They can save a lot of time in debugging later.

This also assumes that you've overridden your GetHashCode implementation when you overrode Equals. If you haven't you may specify an equality comparer in the Dictionary's constructor, but overriding GetHashCode would be better.

GetPiece

Again this is a simple mapping operation that would benefit from a dictionary. Use the same pattern as in the previous section.

Canvas_PointerMoved

You use a pattern of checking objects and returning out of the method if the conditions are invalid throughout the code behind, but don't in this one event handler.

private void Canvas_PointerMoved(object sender, PointerRoutedEventArgs e)
{
    if (_draggedImage == null) {return;}

    SetPosition(e.GetCurrentPoint(Canvas).Position);
}

ViewModel

AddPiece and RemovePiece

Both of these methods copy the original board, make the change, and then paste the new board over the original one.

This is pretty bad for both memory reasons (creating new objects each move that need to be GC'd), and also for the fact that you'll raise a notification for the whole game, leading to each square repainting when you should be just notifying the changed squares.

I can't see your model, but you should probably implement INotifyPropertyChanged in the Board class.

Properties

In general your notification changed event raising could be improved by first checking if the property has actually changed before raising the event.

private Board _board;
public Board Board
{
    get { return _board; }
    set
    {
        if(_board == value) {return;}
        _board = value;
        OnPropertyChanged();
    }
}

Board Position

Switches aren't always but can often be a code smell, indicating that a new type should be made and the branched logic be put in there.

In this case I'm inclined to agree. You don't need to enumerate your board types, you need a board factory interface and a number of implementations.

public interface IBoardFactory
{
  Board Create();
}

public class InitialBoardFactory : IBoardFactory
{
  public Board Create() => new Board();
}

public class EmptyBoardFactory : IBoardFactory
{
  public Board Create() => new EmptyBoard();
}

The property then becomes:

private IBoardFactory _boardFactory;
    public IBoardFactory BoardFactory
    {
        get { return _boardFactory; }
        set
        {
            if (_boardFactory == value) { return; }

            _boardFactory = value;

            Board = value.Create();

            OnPropertyChanged();
        }
    }

You can then hand your view the list of all IBoardFactory implementations by checking the current assembly using reflection. Now if you add a new IBoardFactory it'll automatically be available in the view.

CopyFenCommand

You're recreating the delegate each time the setter is called. Instead, create a readonly delegate in the constructor and return this each time.

StackExchange Code Review Q#152961, answer score: 6