English Draughts

Question

The basic UI for my checkers app is complete, so the next step is to come here for review. I have not built my Facade over my F# game library, so some of the type usages are a little strange; this will be done as soon as I get the library in a slightly more stable state.

MainPage.xaml

This is the opening view (and only view) of my app; it does not have any code-behind:

<Page
    x:Class="CheckersUI.MainPage"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
    xmlns:local="using:CheckersUI"
    xmlns:custom="using:CheckersUI.CustomControls"
    xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
    xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
    mc:Ignorable="d">

    <Grid Background="{ThemeResource ApplicationPageBackgroundThemeBrush}">
        <StackPanel>
            <TextBlock Text="{Binding Status}"
                       HorizontalAlignment="Center"
                       Margin="0,10"
                       FontWeight="Black"
                       FontSize="20" />
            <Border Height="641"
                    Width="641"
                    BorderThickness="1"
                    BorderBrush="Black">
                <custom:EightPieceBoard Board="{Binding GameController.Board, Mode=OneWay}"
                                        Selection="{Binding Selection, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}" />
            </Border>
        </StackPanel>
    </Grid>
</Page>

MainPageViewModel.cs is the VM for MainPage:

public class MainPageViewModel : INotifyPropertyChanged
{
    private MainPage _page;

    public MainPageViewModel(MainPage page)
    {
        _page = page;
        GameController = new GameController.GameController(Board.defaultBoard, Types.Player.Black);
    }

    private string PlayerToString(Types.Player player) =>
        player.IsWhite ? nameof(Types.Player.White) : nameof(Types.Player.Black);

    private GameController.GameController _gameController;
    public GameController.GameController GameController
    {
        get
        {
            return _gameController;
        }
        set
        {
            _gameController = value;
            OnPropertyChanged();
            OnPropertyChanged(nameof(Status));

        }
    }

    private Types.Coord _selection;
    public Types.Coord Selection
    {
        get
        {
            return _selection;
        }
        set
        {
            if (_selection == null)
            {
                _selection = value;
            }
            else if (_selection != null && PublicAPI.isValidMove(_selection, value, GameController))
            {
                GameController = PublicAPI.move(_selection, value, GameController).Value;
                _selection = null;
            }
            else if (GameController.Board[value.Row][value.Column] == FSharpOption<Piece.Piece>.None)
            {
                _selection = null;
            }
            else
            {
                _selection = value;
            }
        }
    }

    public string Status
    {
        get
        {
            var winningPlayer = PublicAPI.isWon(GameController);
            return FSharpOption<Types.Player>.get_IsSome(winningPlayer)
                   ? $"{PlayerToString(winningPlayer.Value)} Won!"
                   : $"{PlayerToString(GameController.Player)}'s turn";
        }
    }

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

EightPieceBoard.xaml

To make managing the board simple, I created a custom UserControl for it. I gave it this name because it can be reused for any board with 8 pieces per side, and not all checkers variants use boards with 8 pieces per side. Note that I cannot use UniformGrid because this is a UWP project.

<UserControl
    x:Class="CheckersUI.CustomControls.EightPieceBoard"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
    xmlns:local="using:CheckersUI.CustomControls"
    xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
    xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
    mc:Ignorable="d">

    <Grid>
        <Grid Name="BoardGrid"
              Width="640"
              Height="640"
              PointerPressed="BoardGrid_PointerPressed">
            <Grid.RowDefinitions>
                <RowDefinition />
                <RowDefinition />
                <RowDefinition />
                <RowDefinition />
                <RowDefinition />
                <RowDefinition />
                <RowDefinition />
                <RowDefinition />
            </Grid.RowDefinitions>
            <Grid.ColumnDefinitions>
                <ColumnDefinition />
                <ColumnDefinition />
                <ColumnDefinition />
                <ColumnDefinition />
                <ColumnDefinition />
                <ColumnDefinition />
                <ColumnDefinition />
                <ColumnDefinition />
            </Grid.ColumnDefinitions>
            <Image Name="BoardImage"
                   Source="../Assets/Checkerboard.png"
                   Grid.RowSpan="8"
                   Grid.ColumnSpan="8" />
        </Grid>
    </Grid>
</UserControl>

EightPieceBoard.xaml.cs

This is the code-behind for the view discussed above. One note about this: I am perfectly aware that when I remove pieces, I only remove those that do not exist on the new board, and when I add pieces, I add one for each tile remaining. This will lead to stacked images, but I did it like this because removing all the tiles caused a nasty flicker, only adding those that did not already exist would have required searching the children list for each piece and checking the grid row and column of each.

public sealed partial class EightPieceBoard : INotifyPropertyChanged
{
    private static Dictionary<Piece.Piece, Uri> PieceToUriMap = new Dictionary<Piece.Piece, Uri>
        {
            {Piece.whiteChecker.Value, new Uri("ms-appx:///../Assets/WhiteChecker.png", UriKind.Absolute)},
            {Piece.whiteKing.Value, new Uri("ms-appx:///../Assets/WhiteKing.png", UriKind.Absolute)},
            {Piece.blackChecker.Value, new Uri("ms-appx:///../Assets/BlackChecker.png", UriKind.Absolute)},
            {Piece.blackKing.Value, new Uri("ms-appx:///../Assets/BlackKing.png", UriKind.Absolute)}
        };

    public static readonly DependencyProperty BoardProperty =
        DependencyProperty.Register(nameof(Board),
            typeof(IEnumerable<IEnumerable<FSharpOption<Piece.Piece>>>),
            typeof(EightPieceBoard),
            new PropertyMetadata(null, new PropertyChangedCallback((sender, e) => ((EightPieceBoard)sender).LoadPieces())));

    public static readonly DependencyProperty SelectionProperty =
        DependencyProperty.Register(nameof(Selection), typeof(Types.Coord), typeof(EightPieceBoard), null);

    public EightPieceBoard()
    {
        InitializeComponent();
    }

    public IEnumerable<IEnumerable<FSharpOption<Piece.Piece>>> Board
    {
        get { return (IEnumerable<IEnumerable<FSharpOption<Piece.Piece>>>)GetValue(BoardProperty); }
        set { SetValue(BoardProperty, value); }
    }

    public Types.Coord Selection
    {
        get { return (Types.Coord)GetValue(SelectionProperty); }
        set { SetValue(SelectionProperty, value); }
    }

    private void BoardGrid_PointerPressed(object sender, PointerRoutedEventArgs e)
    {
        var point = e.GetCurrentPoint((Grid)sender).Position;

        var row = (int)Math.Floor(point.Y / 80);
        var column = (int)Math.Floor(point.X / 80);

        Selection = new Types.Coord(row, column);
    }

    private void ClearPieces(IEnumerable<IEnumerable<FSharpOption<Piece.Piece>>> board)
    {
        foreach (Image element in BoardGrid.Children.ToList())
        {
            if (element != BoardImage)
            {
                var row = Grid.GetRow(element);
                var column = Grid.GetColumn(element);
                var uri = ((BitmapImage)element.Source).UriSource.AbsolutePath;

                if (Board.ElementAt(row).ElementAt(column) == FSharpOption<Piece.Piece>.None || PieceToUriMap[Board.ElementAt(row).ElementAt(column).Value].AbsolutePath != uri)
                {
                    BoardGrid.Children.Remove(element);
                }
            }
        }
    }

    private void PlaceChecker(Piece.Piece piece, int row, int column)
    {
        var bitmapImage = new BitmapImage();
        bitmapImage.UriSource = PieceToUriMap[piece];

        var image = new Image();
        image.Source = bitmapImage;
        Grid.SetRow(image, row);
        Grid.SetColumn(image, column);
        BoardGrid.Children.Add(image);
    }

    public void LoadPieces()
    {
        ClearPieces(Board);

        for (var rowIndex = 0; rowIndex < Board.Count(); rowIndex++)
        {
            for (var colIndex = 0; colIndex < Board.ElementAt(rowIndex).Count(); colIndex++)
            {
                var piece = Board.ElementAt(rowIndex).ElementAt(colIndex);
                if (piece == FSharpOption<Piece.Piece>.None)
                {
                    continue;
                }

                PlaceChecker(piece.Value, rowIndex, colIndex);
            }
        }
    }

    public event PropertyChangedEventHandler PropertyChanged;
    private void OnPropertyChanged([CallerMemberName] string propertyName = null) =>
        PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
}

Answer 1

A couple of things:

<Grid Background="{ThemeResource ApplicationPageBackgroundThemeBrush}">
    <StackPanel>
        ...

Putting the StackPanel as single child of the grid is wasteful. Add RowDefinitions to the grid. As a rule of thumb StackPanel is almost never right. After writing WPF for a couple of years you will probably find yourself using Grid ~95% of the time. Grid is a bit verbose but it is the panel you will not have to refactor away from. Also Grid makes reasoning about layout simpler. You can try this to bring down the verbosity a bit.

You can add BindsTwoWayByDefault to EightPieceBoard.Selection but I don't mind being expliclit about it.

The viewmodel should not know about the view: public MainPageViewModel(MainPage page) it makes testing harder and is smelly in general. Not a clean separation of responsibilites between view & viewmodel.

<Grid>
    <Grid Name="BoardGrid"

Same as above, don't add the grid as single child to the outer grid. This can hurts performance and is noisy to read. Better with a grid than a stackpanel though :)

Answer 2

I've spotted...

private string PlayerToString(Types.Player player) =>
    player.IsWhite ? nameof(Types.Player.White) : nameof(Types.Player.Black);

I find the Player should have a property that is one of the two enum values

enum PlayerColor
{
    Black,
    White,
}

With this you don't need the IsWhite boolean and the conversion method because you can either use .ToString() on it or in many cases it will be implicitly converted.

---

Piece.Piece

This is a code smell. The class and the namespace shouldn't have the same names.

The namespace should probably by Pieces.

---

       OnPropertyChanged();
       OnPropertyChanged(nameof(Status));

You should not need the second call to OnPropertyChanged. Without parameters it should already trigger an update to all properties.

---

private void ClearPieces(IEnumerable<IEnumerable<FSharpOption<Piece.Piece>>> board)

This method requires a parameter but you don't seem to use it anywhere.