6
\$\begingroup\$

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:

Checkers Board Editor

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:

<Page
    x:Class="CheckersUI.Pages.BoardEditor"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
    xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
    xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
    xmlns:vms="using:CheckersUI.VMs"
    xmlns:customControls="using:CheckersUI.CustomControls"
    xmlns:converters="using:CheckersUI.Converters"
    mc:Ignorable="d" d:DataContext="{d:DesignInstance vms:BoardEditorViewModel}">
    <Page.Resources>
        <converters:VariantToDisplayStringConverter x:Key="VariantToDisplayString" />
        <converters:VariantsToDisplayStringsConverter x:Key="VariantsToDisplayStrings" />
    </Page.Resources>
    <Grid Margin="0,10,0,0">
        <Grid.RowDefinitions>
            <RowDefinition Height="640" />
            <RowDefinition />
        </Grid.RowDefinitions>
        <Grid.ColumnDefinitions>
            <ColumnDefinition Width="110" />
            <ColumnDefinition Width="640" />
            <ColumnDefinition />
        </Grid.ColumnDefinitions>
        <Grid Width="640" 
              Height="640"
              Grid.Column="1"
              Grid.Row="0"
              VerticalAlignment="Top"
              Name="BoardGrid"
              PointerPressed="Canvas_PointerPressed">
            <customControls:EightPieceBoard Orientation="{Binding Orientation}" Board="{Binding Board}" />
        </Grid>
        <StackPanel Grid.Column="1" Grid.Row="1" Orientation="Horizontal" VerticalAlignment="Top" Margin="0,20,0,0">
            <TextBlock FontWeight="Medium"
                       FontSize="18"
                       VerticalAlignment="Center"
                       Text="FEN:" />
            <TextBox Width="560"
                     IsReadOnly="True"
                     Margin="5,0,0,0"
                     BorderThickness="1"
                     AcceptsReturn="False"
                     Background="#CCCCCC"
                     Text="{Binding FenString}" />
            <Border BorderThickness="1" BorderBrush="Black">
                <Button Content="&#xe16f;"
                        Background="White"
                        FontFamily="Segoe UI Symbol"
                        ToolTipService.ToolTip="Copy FEN"
                        Foreground="Black"
                        BorderThickness="1"
                        BorderBrush="Black"
                        Height="30"
                        Command="{Binding CopyFenCommand}" />
            </Border>
        </StackPanel>
        <Grid Grid.Row="0"
              Grid.Column="2"
              Margin="10,0,0,0">
            <Grid.RowDefinitions>
                <RowDefinition Height="75" />
                <RowDefinition Height="75" />
                <RowDefinition Height="75" />
            </Grid.RowDefinitions>
            <StackPanel Grid.Row="0">
                <TextBlock FontWeight="Bold"
                           Margin="0,0,0,5"
                           Text="Variant:" />
                <ComboBox Width="200"
                          Margin="0,0,0,10"
                          ItemsSource="{Binding Variants, Mode=OneTime, Converter={StaticResource VariantsToDisplayStrings}}"
                          SelectedItem="{Binding Variant, Mode=TwoWay, Converter={StaticResource VariantToDisplayString}}" />
            </StackPanel>
            <StackPanel Grid.Row="1">
                <TextBlock FontWeight="Bold"
                           Margin="0,0,0,5"
                           Text="First move:" />
                <ComboBox Width="200"
                          Margin="0,0,0,10"
                          ItemsSource="{Binding Players, Mode=OneTime}"
                          SelectedItem="{Binding Player, Mode=TwoWay}" />
            </StackPanel>
            <StackPanel Grid.Row="2">
                <TextBlock FontWeight="Bold"
                           Margin="0,0,0,5"
                           Text="Set board position:" />
                <ComboBox Width="200"
                          ItemsSource="{Binding Positions, Mode=OneTime}"
                          SelectedItem="{Binding Position, Mode=TwoWay}" />
            </StackPanel>
        </Grid>
        <Canvas Name="Canvas"
                Grid.Row="0"
                Grid.Column="0"
                Grid.ColumnSpan="2"
                PointerPressed="Canvas_PointerPressed"
                PointerReleased="Canvas_PointerReleased"
                PointerMoved="Canvas_PointerMoved" />
        <Grid HorizontalAlignment="Right"
              Margin="0,0,10,0">
            <Grid.RowDefinitions>
                <RowDefinition />
                <RowDefinition />
                <RowDefinition />
                <RowDefinition />
            </Grid.RowDefinitions>
            <StackPanel Grid.Row="0">
                <Image Name="BlackChecker"
                       Source="../Assets/SteelTheme/BlackChecker.png"
                       Width="80"
                       PointerPressed="Image_PointerPressed" />
                <TextBlock HorizontalAlignment="Center">Black Checker</TextBlock>
            </StackPanel>
            <StackPanel Grid.Row="1">
                <Image Name="BlackKing"
                       Source="../Assets/SteelTheme/BlackKing.png"
                       Width="80"
                       PointerPressed="Image_PointerPressed" />
                <TextBlock HorizontalAlignment="Center">Black King</TextBlock>
            </StackPanel>
            <StackPanel Grid.Row="2">
                <Image Name="WhiteChecker"
                       Source="../Assets/SteelTheme/WhiteChecker.png"
                       Width="80"
                       PointerPressed="Image_PointerPressed" />
                <TextBlock HorizontalAlignment="Center">White Checker</TextBlock>
            </StackPanel>
            <StackPanel Grid.Row="3">
                <Image Name="WhiteKing"
                       Source="../Assets/SteelTheme/WhiteKing.png"
                       Width="80"
                       PointerPressed="Image_PointerPressed" />
                <TextBlock HorizontalAlignment="Center">White King</TextBlock>
            </StackPanel>
        </Grid>
    </Grid>
</Page>

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(ApplicationData sender, object args)
    {
        if ((string)_roamingSettings.Values["Theme"] == _currentTheme)
        {
            return;
        }

        Dispatcher.RunAsync(CoreDispatcherPriority.Normal, () =>
        {
            _currentTheme = (string)_roamingSettings.Values["Theme"];
            LoadImages();
        });
    }

    private void LoadImages()
    {
        var whiteCheckerBitmapImage = new BitmapImage(GetPieceUri(Piece.WhiteChecker));
        WhiteChecker.Source = whiteCheckerBitmapImage;

        var whiteKingBitmapImage = new BitmapImage(GetPieceUri(Piece.WhiteKing));
        WhiteKing.Source = whiteKingBitmapImage;

        var blackCheckerBitmapImage = new BitmapImage(GetPieceUri(Piece.BlackChecker));
        BlackChecker.Source = blackCheckerBitmapImage;

        var blackKingBitmapImage = new BitmapImage(GetPieceUri(Piece.BlackKing));
        BlackKing.Source = blackKingBitmapImage;
    }

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

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

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

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

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

        throw new MissingMemberException("Piece not found");
    }

    private Piece GetPiece(Image image)
    {
        if (image == WhiteChecker)
        {
            return Piece.WhiteChecker;
        }
        else if (image == WhiteKing)
        {
            return Piece.WhiteKing;
        }
        else if (image == BlackChecker)
        {
            return Piece.BlackChecker;
        }
        else
        {
            return Piece.BlackKing;
        }
    }

    private void PlacePiece(Point point)
    {
        var row = (int)Math.Floor(point.Y / (BoardGrid.ActualHeight / 8));
        var column = (int)Math.Floor(point.X / (BoardGrid.ActualWidth / 8));

        // todo let user set variant they are working with
        if (!Board.IsValidSquare(ViewModel.Variant, row, column)) { return; }

        ViewModel.AddPiece(_piece, row, column);
    }

    private void Image_PointerPressed(object sender, PointerRoutedEventArgs e)
    {
        var source = ((Image) sender).Source;
        _draggedImage = new Image {Source = source};
        _piece = GetPiece((Image) sender);

        Canvas.Children.Add(_draggedImage);
        SetPosition(e.GetCurrentPoint(Canvas).Position);
        Canvas.CapturePointer(e.Pointer);
    }

    private void Canvas_PointerReleased(object sender, PointerRoutedEventArgs e)
    {
        PlacePiece(e.GetCurrentPoint(BoardGrid).Position);
        Canvas.Children.Remove(_draggedImage);
        Canvas.ReleasePointerCapture(e.Pointer);

        _draggedImage = null;
        _piece = null;

        ViewModel.UpdateFen();
    }

    private void SetPosition(Point point)
    {
        Canvas.SetLeft(_draggedImage, point.X - (_draggedImage.ActualWidth / 2));
        Canvas.SetTop(_draggedImage, point.Y - (_draggedImage.ActualHeight / 2));
    }

    private void Canvas_PointerMoved(object sender, PointerRoutedEventArgs e)
    {
        if (_draggedImage != null)
        {
            SetPosition(e.GetCurrentPoint(Canvas).Position);
        }
    }

    private void Canvas_PointerPressed(object sender, PointerRoutedEventArgs e)
    {
        var point = e.GetCurrentPoint(BoardGrid).Position;
        var row = (int)Math.Floor(point.Y / (BoardGrid.ActualHeight / 8));
        var column = (int)Math.Floor(point.X / (BoardGrid.ActualWidth / 8));
        if (!Board.IsValidSquare(ViewModel.Variant, row, column)) { return; }

        var piece = ViewModel.Board[row, column];
        if (piece == null) { return; }

        Canvas.CapturePointer(e.Pointer);
        var bitmapImage = new BitmapImage(GetPieceUri(piece));
        var image = new Image {Source = bitmapImage};
        _draggedImage = image;
        _piece = piece;

        ViewModel.RemovePiece(row, column);
        Canvas.Children.Add(_draggedImage);
        SetPosition(point);
    }
}

And finally, this is my VM:

public enum BoardPosition { Initial, Empty }

public class BoardEditorViewModel : INotifyPropertyChanged
{
    public BoardEditorViewModel(Board board)
    {
        Orientation = Player.White;
        Board = board;

        Player = Player.Black;
    }

    public void AddPiece(Piece piece, int row, int column)
    {
        var newBoard = Board.Copy();
        newBoard.GameBoard[row, column] = piece;
        Board = newBoard;
    }

    public void RemovePiece(int row, int column)
    {
        var newBoard = Board.Copy();
        newBoard.GameBoard[row, column] = null;
        Board = newBoard;
    }

    public void UpdateFen() => OnPropertyChanged(nameof(FenString));

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

    public List<Player> Players =>
        Enum.GetValues(typeof(Player)).Cast<Player>().ToList();

    private Player _player;
    public Player Player
    {
        get { return _player; }
        set {
            if (_player != value)
            {
                _player = value;
                OnPropertyChanged();
                OnPropertyChanged(nameof(FenString));
            }
        }
    }

    private Player _orientation;
    public Player Orientation
    {
        get { return _orientation; }
        set
        {
            if (_orientation != value)
            {
                _orientation = value;
                OnPropertyChanged();
            }
        }
    }

    public string FenString
    {
        get
        {
            var controller = new GameController(Variant, Board, Player);
            return controller.Fen;
        }
    }

    public List<BoardPosition> Positions =>
        Enum.GetValues(typeof(BoardPosition)).Cast<BoardPosition>().ToList();

    private BoardPosition _position;
    public BoardPosition Position
    {
        get { return _position; }
        set
        {
            if (_position == value) { return; }

            _position = value;
            switch (value)
            {
                case BoardPosition.Initial:
                    Board = new Board();
                    break;
                case BoardPosition.Empty:
                    Board = Board.EmptyBoard();
                    break;
                default:
                    throw new ArgumentException(nameof(value));
            }

            OnPropertyChanged();
        }
    }

    public List<Variant> Variants =>
        Enum.GetValues(typeof(Variant)).Cast<Variant>().ToList();

    private Variant _variant;
    public Variant Variant
    {
        get { return _variant; }
        set
        {
            if (_variant != value)
            {
                _variant = value;
                OnPropertyChanged();
            }
        }
    }

    private DelegateCommand _copyFenCommand;
    public DelegateCommand CopyFenCommand
    {
        get
        {
            if (_copyFenCommand != null)
            {
                return _copyFenCommand;
            }

            _copyFenCommand = new DelegateCommand(param => SetClipboardContent(FenString));
            return _copyFenCommand;
        }
    }

    private void SetClipboardContent(string content)
    {
        var dataPackage = new DataPackage();
        dataPackage.SetText(content);
        Clipboard.SetContent(dataPackage);
    }

    public event PropertyChangedEventHandler PropertyChanged;
    protected virtual void OnPropertyChanged([CallerMemberName] string propertyName = null) =>
        PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
}
\$\endgroup\$
6
\$\begingroup\$

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<Piece, string> pieceImageFilenames = new Dictionary<Piece, string>()
{
  {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.

\$\endgroup\$

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.