2
\$\begingroup\$

A simple game of TicTacToe.

Please review for style and other.

Is this a proper ViewModel?

namespace TicTacToe3
{
    public partial class MainWindow : Window
    {
        private ViewModel viewModel;
        public MainWindow()
        {
            InitializeComponent();
            viewModel = new ViewModel();
            this.DataContext = viewModel;
        }

        private void clickSQ(object sender, RoutedEventArgs e)
        {
            Button button = sender as Button;
            int tag = int.Parse(button.Tag.ToString());
            viewModel.TicTacSqs[tag].XO = viewModel.XOturn;
            viewModel.FlipXOturn();
        }

        private void clickNew(object sender, RoutedEventArgs e)
        {
            viewModel.NewGame();
        }
    }
    public class ViewModel: INotifyPropertyChanged
    {
        public event PropertyChangedEventHandler PropertyChanged;
        private void NotifyPropertyChanged([CallerMemberName] String propertyName = "")
        {
            if (PropertyChanged != null)
            {
                PropertyChanged(this, new PropertyChangedEventArgs(propertyName));
            }
        }
        public xo XOturn { get; private set; } = xo.X;
        public void FlipXOturn()
        {
            XOturn = XOturn == xo.X ? xo.O : xo.X;
            NotifyPropertyChanged("XOturn");
            Winner = CheckForWinner();
        }
        private xo? CheckForWinner()
        {
            if(TicTacSqs.Where(x => !x.NotSelected).Count() > 4)
            {
                if(Won(xo.X))
                {
                    LockSq();
                    return xo.X;
                }
                if(Won(xo.O))
                {
                    LockSq();
                    return xo.O;
                }
                if (TicTacSqs.Where(x => !x.NotSelected).Count() == 9)
                {
                    LockSq();
                    return xo.Tie;
                }
            }
            return null;
        }
        private void LockSq()
        {   
            foreach(TicTacSq sq in TicTacSqs.Where(x => x.XO == null))
            {
                sq.XO = xo._;
            }
        }
        private bool Won(xo Side)
        {
            return (   (TicTacSqs[0].XO == Side && TicTacSqs[1].XO == Side && TicTacSqs[2].XO == Side)
                    || (TicTacSqs[3].XO == Side && TicTacSqs[4].XO == Side && TicTacSqs[5].XO == Side)
                    || (TicTacSqs[6].XO == Side && TicTacSqs[7].XO == Side && TicTacSqs[8].XO == Side)
                    || (TicTacSqs[0].XO == Side && TicTacSqs[3].XO == Side && TicTacSqs[6].XO == Side)
                    || (TicTacSqs[1].XO == Side && TicTacSqs[4].XO == Side && TicTacSqs[7].XO == Side)
                    || (TicTacSqs[2].XO == Side && TicTacSqs[5].XO == Side && TicTacSqs[8].XO == Side)
                    || (TicTacSqs[0].XO == Side && TicTacSqs[4].XO == Side && TicTacSqs[8].XO == Side)
                    || (TicTacSqs[2].XO == Side && TicTacSqs[4].XO == Side && TicTacSqs[6].XO == Side)
                   );
        }
        private xo? winner = null;
        public xo? Winner
        {
            get { return winner; }
            set
            {
                if(winner != value)
                {
                    winner = value;
                    NotifyPropertyChanged();
                }
            }
        }
        public void NewGame()
        {
            foreach(TicTacSq sq in TicTacSqs.Where(x => !x.NotSelected))
            {
                sq.XO = null;             
            }
            Winner = null;
        }
        public TicTacSq[] TicTacSqs { get; } =  { new TicTacSq(0), new TicTacSq(1), new TicTacSq(2),
                                                  new TicTacSq(3), new TicTacSq(4), new TicTacSq(5),
                                                  new TicTacSq(6), new TicTacSq(7), new TicTacSq(8) };
    }
    public enum xo { X, O, Tie, _ }
    public class TicTacSq : INotifyPropertyChanged
    {
        public event PropertyChangedEventHandler PropertyChanged;
        private void NotifyPropertyChanged([CallerMemberName] String propertyName = "")
        {
            if (PropertyChanged != null)
            {
                PropertyChanged(this, new PropertyChangedEventArgs(propertyName));
            }
        }
        public int ID { get; }
        private xo? xo = null;
        public xo? XO
        {
            get { return xo; }
            set
            {
                if(xo != value)
                {
                    xo = value;
                    NotifyPropertyChanged();
                    NotifyPropertyChanged("NotSelected");
                }
            }
        }
        public bool NotSelected
        {
            get { return !(XO == TicTacToe3.xo.O || XO == TicTacToe3.xo.X || XO == TicTacToe3.xo._); }
        }
        public TicTacSq(int id)
        {
            ID = id;
        }
    }
}

.

<Window x:Class="TicTacToe3.MainWindow"
        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:local="clr-namespace:TicTacToe3"
        mc:Ignorable="d"
        Title="TicTacToe" Height="230" Width="180">
    <Window.Resources>
        <Style TargetType="Button">
            <Setter Property="Width" Value="45"/>
            <Setter Property="Height" Value="30"/>
            <Setter Property="Margin" Value="4"/>
            <Setter Property="FontSize" Value="14"/>
            <Setter Property="FontWeight" Value="Bold"/>
            <Setter Property="VerticalAlignment" Value="Center"/>
        </Style>
        <Style TargetType="TextBlock">
            <Setter Property="FontSize" Value="14"/>
            <Setter Property="Margin" Value="4"/>
            <Setter Property="VerticalAlignment" Value="Center"/>
        </Style>
    </Window.Resources>
    <Grid>
        <Grid.RowDefinitions>
            <RowDefinition Height="auto"/>
            <RowDefinition Height="auto"/>
            <RowDefinition Height="auto"/>
            <RowDefinition Height="auto"/>
            <RowDefinition Height="auto"/>
            <RowDefinition Height="auto"/>
            <RowDefinition Height="auto"/>
        </Grid.RowDefinitions>
        <Grid.ColumnDefinitions>
            <ColumnDefinition Width="auto"/>
            <ColumnDefinition Width="auto"/>
            <ColumnDefinition Width="auto"/>
        </Grid.ColumnDefinitions>
        <TextBlock Grid.Row="0" Grid.Column="0" Text="Turn" HorizontalAlignment="Right"/>
        <TextBlock Grid.Row="0" Grid.Column="1" Text="{Binding XOturn}" HorizontalAlignment="Left"/>
        <Button Grid.Row="0" Grid.Column="2" Content="New"  FontWeight="Normal" Click="clickNew"/>
        <Button Grid.Row="1" Grid.Column="0" Tag="0" Content="{Binding TicTacSqs[0].XO}" IsEnabled="{Binding TicTacSqs[0].NotSelected}" Click="clickSQ"/>
        <Button Grid.Row="1" Grid.Column="1" Tag="1" Content="{Binding TicTacSqs[1].XO}" IsEnabled="{Binding TicTacSqs[1].NotSelected}" Click="clickSQ"/>
        <Button Grid.Row="1" Grid.Column="2" Tag="2" Content="{Binding TicTacSqs[2].XO}" IsEnabled="{Binding TicTacSqs[2].NotSelected}" Click="clickSQ"/>
        <Button Grid.Row="2" Grid.Column="0" Tag="3" Content="{Binding TicTacSqs[3].XO}" IsEnabled="{Binding TicTacSqs[3].NotSelected}" Click="clickSQ"/>
        <Button Grid.Row="2" Grid.Column="1" Tag="4" Content="{Binding TicTacSqs[4].XO}" IsEnabled="{Binding TicTacSqs[4].NotSelected}" Click="clickSQ"/>
        <Button Grid.Row="2" Grid.Column="2" Tag="5" Content="{Binding TicTacSqs[5].XO}" IsEnabled="{Binding TicTacSqs[5].NotSelected}" Click="clickSQ"/>
        <Button Grid.Row="3" Grid.Column="0" Tag="6" Content="{Binding TicTacSqs[6].XO}" IsEnabled="{Binding TicTacSqs[6].NotSelected}" Click="clickSQ"/>
        <Button Grid.Row="3" Grid.Column="1" Tag="7" Content="{Binding TicTacSqs[7].XO}" IsEnabled="{Binding TicTacSqs[7].NotSelected}" Click="clickSQ"/>
        <Button Grid.Row="3" Grid.Column="2" Tag="8" Content="{Binding TicTacSqs[8].XO}" IsEnabled="{Binding TicTacSqs[8].NotSelected}" Click="clickSQ"/>
        <TextBlock Grid.Row="4" Grid.Column="0" Text="Winner"/>
        <TextBlock Grid.Row="4" Grid.Column="1" Text="{Binding Winner}"/>
    </Grid>
</Window>
\$\endgroup\$
  • 1
    \$\begingroup\$ A screenshot would be great. Btw. this _ is a very weird enum value. What is this for? We use _ as a discard in C# so its very confusing there. \$\endgroup\$ – t3chb0t Jun 17 '18 at 9:08
  • \$\begingroup\$ @t3chb0t Check the code, _ is used as a discard. \$\endgroup\$ – paparazzo Jun 17 '18 at 9:43
3
\$\begingroup\$

It's been a while since I did any XAML, so I'll mostly ignore it in this review. A real MVVM pro would probably suggest using Commands instead of handling click events, but I'll let such a person speak for themselves.

The whole thing is horribly hard-coded for a 3x3 tic-tac-toe game, but I figure you're not trying to produce a work of art here.

ViewModel

I'm not an MVVM expert (or indeed any good at UI work), but I would say that no, this is not a good ViewModel. This is really just Model. The idea of a ViewModel is to enable communication between a self-contained Model, and a particular View of that Model. Because that interface is so thin, you've instinctively neglected the ViewModel, and just happen to have decided that ViewModel is a passable name for the Model.

Here, the Model is the game-state, and the logic to transition between game-states. The View is obviously the XAML and Code-behind (if any is necessary), and the ViewModel would be what re-exposes the Model with an appropriate interface for the View.

What really matters, and why MVVM is useful, is that you don't coupling your model to any particular (user) interface to the model: and you've just about achieved that, so that's ok (only it's called ViewModel). You can 'check' that this is the case by looking for external dependancies (there are none) and by contemplating how painful it would be to write a new UI, e.g. so that people could play online or something, and you'd realise that you can keep the Model (ViewModel and the rest of the game state) and ditch everything else without losing any logic.

Only there is one bit of logic which would be lost... and that is in the clickSQ method. This method breaks the first rule of being a good Model: you should always be in a consistent state. clickSQ first updates the state of a cell, and then ends the go (with the poorly named FlipXOturn method). Of course, it's the model's fault for letting it do this, but it should be a huge red flag when you actually do it. What you should instead do, is expose a single method which accepts which cell has been chosen, and performs all the logic for completing a turn (i.e. a single transition from one consistent state to another). Your View/ViewModel should disallow making illegal calls (e.g. trying to take a cell which is already taken), but that is only for the user's benefit: your Model also should prevent any such operation. Currently, I can take an instance of ViewModel (really a Model) and fill the cells however I want, producing all manner of inconsistent states.

Therefore, my main recommendation is to refactor the clickSQ method, moving the logic inside the Model. This should involve checking that the go is legal (e.g. cell not taken, game hasn't already ended), and then performing all necessary operations to complete that go, or fail (in some way, I don't really care) if it is illegal (it could throw (i.e. you trust the View/ViewModel) or it could be a TryMove method which returns a bool depending on whether it was allowed or not, and force the View/ViewModel to handle the alternative (this is the better option, obviously)). It should be impossible to 'break' the Model.

Possible API Weirdness

NewGame() starts a new game, but doesn't reset the starting player. (NewGame() also sounds to me like it would return a new instance; I'd prefer something like StartNewGame() or Reset())

xo: as @t3chbot commented, this is a bit of a weird enum. The combination of _ and xo? means I'm never sure what I'm meant to be looking for. Inline documention (\\\) would certainly help here, and as I always suggest, it's good to have it on any public type or member.

FlipXOturn(): this method does more than "FlipXOturn", it also checks the winning state. I think it is a fine method, if you remove the winning stuff, and instead factor that out into a new method (CheckWinner) and call those (private methods) from the new TryClaimCell (or whatever you call it) method discussed above. It's very easy to jam new logic into existing methods where they 'will work', and to end up having no idea what your methods do, because you've not adhered to the intentions they express (obviously I'd recommend watching out for this).

NotifyPropertyChanged stuff

public xo XOturn { get; private set; } = xo.X;

I'd be inclined to transform this like you have Winner, so that it notifies about itself. This makes the surrounding code clearner, and will avoid bugs.

If you do ever need to explicitly state the name of a member, then use nameof:

NotifyPropertyChanged(nameof(XOturn));

This makes it unambigous that it's meant to refer to a code element, avoids typos, and permits easy refactoring

Encapsulation

Most everything looks well scoped, except for the Winner's setter, which I think ought to be private. (And the stuff discussed above regarding Models).

I'd argue that TicTacSq is an implementation detail of the Model, and that exposing it is bad, because it then allows you invalidate the game-state (as discussed above).

Naming

NotSelected: I use negatives in my code all the time, and I always regret it. "Selected" also sounds to me like some transient UI concern, rather than game state: I'd prefer something like "Claimed" or "Assigned", but maybe that's just me.

TicTacSq: Is Square so much to type?! I can buy that this is pretty unambiguous, but in the age of IDEs, this can only slow down coding: auto-complete will give you TicTacSquare when you type TicTacSq (or indeed TTS (thanks VS team!)), but will be unhappy if you type TicTacSqu because future you or Joe Coder doesn't remember/know that it's been truncated. Do not underestimate the importance of discoverability, and any sort of short-form hinders that. clickSQ is another variation, where you've given up on Square and now have a 2 letter initialism!

LockSq should probably be LockSquares; it sounds like it is locking a single square.

xo: I'm sure you know this, and you're free to ignore it, but the MS naming guidlines recommend ProperCamelCase for public types and members, so it feels odd reading your code (esspecially where xo is also a private field) to see xo as a type name.

The same conventions would see the Side parameter called side.

Won is also a bit of an odd name: it sounds like a statement of truth, rather than a query.

Style

You're using a combination of if( and if (. I prefer the latter, but just be consistent and no-one should complain.

A few more empty lines would help to break up your code, and make it easier to follow. In clickSQ for example:

Button button = sender as Button;
int tag = int.Parse(button.Tag.ToString());

viewModel.TicTacSqs[tag].XO = viewModel.XOturn;
viewModel.FlipXOturn();

Though I've already expressed why this a bad method, logically it is doing two things, and separating them out would be good. When you replace the last 2 lines with a call to ClaimCell, then the empty line will still make it clear that you're going from 'View' logic to 'Model' logic. Conceptual separations are just as important as statically enforcable separations. You have a few such breaks between methods in MainWindow, but then seem not to anywhere else. Again, I'd prefer a break between every method, but you've got to be consistent.

\$\endgroup\$
  • \$\begingroup\$ @ the up-voter: please read the whole review before upvoting! (I'm sorry if you can really read this much text (and form a critical opinion) in 1 minute, but I don't believe it!) \$\endgroup\$ – VisualMelon Jun 17 '18 at 10:25
  • \$\begingroup\$ Same player not go first every time seems fair to me. MVVM is not the question. \$\endgroup\$ – paparazzo Jun 17 '18 at 10:34
  • \$\begingroup\$ Haha! I don't have to read everything (immediately) in order to know it's good :-P I often upvote answers by just skimming over a text :-] and I had never thought I would get a down-vote (even a virtual one) for upvoting too quickly :-o \$\endgroup\$ – t3chb0t Jun 17 '18 at 10:39
  • \$\begingroup\$ @paparazzo fair enough. Regarding MVVM, I know I've referred to it, but nothing I say is specific to MVVM (indeed, I don't even suggest you rename the class to something more MVVMy), and it's what everyone is thinking what you ask about a "ViewModel". All the ranting about Models (i.e. an interface to a black-box which contains some consistent state) is completely general. \$\endgroup\$ – VisualMelon Jun 17 '18 at 10:42
  • \$\begingroup\$ I also cannot undestand why you don't have at least two of them already but that's a whole different story... \$\endgroup\$ – t3chb0t Jun 17 '18 at 10:44

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.