Simon Says: "Make me a pretty game"

Question

In Memoriam

Ralph H. Baer, co-inventor of the original "Simon" game, died Saturday December 6^th 2014, at 92. With his passing, this friendly little challenge inadvertently became a memorial to the father of video game consoles. Rest in peace Mr. Baer, you have changed our lives forever.

---

I finally got my Simon Says implementation to work as I intended it to. That's great, but now I'm left with a mess and I'm not sure where to start cleaning up (full code on GitHub).

Per the specs, there was going to be 4 buttons. So I created an enum and called it SimonButton:

public enum SimonButton
{
    Green,
    Red,
    Blue,
    Yellow
}

I created a SimonSaysRound class that implements INotifyPropertyChanged, to bind with the UI, mostly to provide the player's score.

public class SimonSaysRound : INotifyPropertyChanged
{
    private const int PointsForGoodMatch = 5;

    private readonly SimonButton[] _sequence;
    private int _matches;
    private int _score;

    public SimonSaysRound(IEnumerable<SimonButton> sequence, int score)
    {
        _sequence = sequence.ToArray();
        _score = score;
        _matches = 0;
    }

    public event EventHandler<SimonSaysScoreEventArgs> RoundCompleted;
    public void OnRoundCompleted()
    {
        var handler = RoundCompleted;
        if (handler != null)
        {
            var result = _matches == _sequence.Length;
            RoundCompleted(this, new SimonSaysScoreEventArgs(result, Score));
        }
    }

    public void Play(SimonButton button)
    {
        var success = _sequence[_matches] == button;
        if (success)
        {
            Score += PointsForGoodMatch;
            _matches++;
        }

        if (!success || _matches == _sequence.Length)
        {
            OnRoundCompleted();
        }
    }

    public int Round
    {
        get { return _sequence.Length; }
    }

    public int Score
    {
        get { return _score; }
        private set
        {
            if (value == _score) return;
            _score = value;
            OnPropertyChanged();
        }
    }

    public int Length { get { return _sequence.Length; } }
    public IEnumerable<SimonButton> Sequence { get { return _sequence; } }

    public event PropertyChangedEventHandler PropertyChanged;

    [NotifyPropertyChangedInvocator]
    protected virtual void OnPropertyChanged([CallerMemberName] string propertyName = null)
    {
        PropertyChangedEventHandler handler = PropertyChanged;
        if (handler != null) handler(this, new PropertyChangedEventArgs(propertyName));
    }
}

I don't normally implement my INotifyPropertyChanged like this - ReSharper suggested something, and I wanted to see what it did.. and I don't dislike it.

When the application starts, the main window is displayed, and an animation reveals a "Start" button:

When the button is clicked, the message bar collapses, and a button flashes by quickly animating a radial gradient's offset value (with accompanying sound effects):

When the player enters the correct sequence, the middle bar expands reveals a "round completed" message that shows the player their score; game resumes when the player clicks /collapses the message:

The "Game Over" screen displays the player's score, and the number of completed rounds; clicking the message closes the window, and then the program ends:

The XAML/UI is reviewable here; this post is about the actual application code:

public partial class App : Application
{
    private readonly MainWindow _mainWindow = new MainWindow();
    private SimonSaysRound _currentRound;

    private readonly IDictionary<SimonButton, string> _sounds;

    private readonly int _seed;

    public App()
    {
        _seed = new Random().Next();

        var folder = Path.GetDirectoryName(GetType().Assembly.Location);
        _sounds = Enum.GetValues(typeof (SimonButton))
                      .Cast<SimonButton>()
                      .ToDictionary(button => button,
                                    button => Path.Combine(folder ?? string.Empty, "Resources", button + ".wav"));
    }

    protected override void OnStartup(StartupEventArgs e)
    {
        _mainWindow.SimonButtonClicked += OnSimonButtonClick;
        _mainWindow.PlayNextRound += _mainWindow_PlayNextRound;
        _mainWindow.ShowDialog();
    }

    private void _mainWindow_PlayNextRound(object sender, EventArgs e)
    {
        PlayNextRound();
    }

    private void PlayNextRound()
    {
        var sequenceLength = 1;
        var score = 0;
        if (_currentRound != null)
        {
            sequenceLength = _currentRound.Length + 1;
            score = _currentRound.Score;
        }

        _currentRound = new SimonSaysRound(GenerateSequence(sequenceLength), score);
        _currentRound.RoundCompleted += _currentRound_RoundCompleted;
        _mainWindow.DataContext = _currentRound;
        _mainWindow.DataContext = _currentRound;
        PlaySequence();
    }

    private IEnumerable<SimonButton> GenerateSequence(int length)
    {
        var random = new Random(_seed);
        for (var i = 0; i < length; i++)
        {
            yield return (SimonButton)random.Next(Enum.GetValues(typeof(SimonButton)).GetLength(0));
        }
    }

    private void _currentRound_RoundCompleted(object sender, SimonSaysRoundCompletedEventArgs e)
    {
        if (e.Success)
        {
            _mainWindow.OnRoundSuccessful();
        }
        else
        {
            _mainWindow.OnGameOver();
        }
    }

    private async Task PlaySequence()
    {
        foreach (var button in _currentRound.Sequence)
        {
            await OnSimonButtonClickAsync(null, new SimonButtonEventArgs(button));
            Thread.Sleep(300);
        }
    }

    private void OnSimonButtonClick(object sender, SimonButtonEventArgs e)
    {
        OnSimonButtonClickAsync(sender, e);
    }

    private async Task OnSimonButtonClickAsync(object sender, SimonButtonEventArgs e)
    {
        using (var player = new SoundPlayer(_sounds[e.Button]))
        {
            player.Play();
        }

        if (sender != null)
        {
            _currentRound.Play(e.Button);
        }

        await _mainWindow.HighlightSimonButton(e.Button);
    }
}

And the code-behind for the main window:

public partial class MainWindow : Window
{
    private readonly IDictionary<SimonButton, Border> _buttons;

    public MainWindow()
    {
        InitializeComponent();
        _buttons = new Dictionary<SimonButton, Border>
            {
                { SimonButton.Green, Green },
                { SimonButton.Red, Red },
                { SimonButton.Yellow, Yellow },
                { SimonButton.Blue, Blue }
            };

        RegisterName(MessageBar.Name, MessageBar);
        foreach (var button in _buttons)
        {
            RegisterName(button.Value.Name, button.Value);
        }

        DisableButtons();

        MouseDown += MainWindow_MouseDown;
        Activated += MainWindow_Activated;
    }

    private async void MainWindow_Activated(object sender, EventArgs e)
    {
        GameScoreLabel.Visibility = Visibility.Collapsed;
        GameButton.Text = "Start!";

        await AnimateMessageBand(36);
        Activated -= MainWindow_Activated;

        GameButton.MouseDown += GameButtonStartGame;
    }

    private void MainWindow_MouseDown(object sender, MouseButtonEventArgs e)
    {
        if (e.ChangedButton == MouseButton.Left)
        {
            DragMove();
        }
    }

    public event EventHandler<SimonButtonEventArgs> SimonButtonClicked;
    public async Task OnSimonButtonClicked(SimonButton button)
    {
        var handler = SimonButtonClicked;
        if (handler != null)
        {
            handler.Invoke(this, new SimonButtonEventArgs(button));
        }
    }

    public async Task OnGameOver()
    {
        GameButton.Text = string.Format("Oops! {0} rounds completed.", ((SimonSaysRound)DataContext).Round - 1);
        GameScoreLabel.Visibility = Visibility.Visible;
        await AnimateMessageBand(56);

        DisableButtons();

        GameButton.MouseDown += GameButtonEndGame;
    }

    private void DisableButtons()
    {
        Green.MouseDown -= Green_MouseDown;
        Red.MouseDown -= Red_MouseDown;
        Yellow.MouseDown -= Yellow_MouseDown;
        Blue.MouseDown -= Blue_MouseDown;
    }

    public void EnableButtons()
    {
        Green.MouseDown += Green_MouseDown;
        Red.MouseDown += Red_MouseDown;
        Yellow.MouseDown += Yellow_MouseDown;
        Blue.MouseDown += Blue_MouseDown;
    }

    public async Task OnRoundSuccessful()
    {
        DisableButtons();
        GameButton.Text = string.Format("Round {0} completed! Ready?", ((SimonSaysRound)DataContext).Round);
        GameScoreLabel.Visibility = Visibility.Visible;
        await AnimateMessageBand(56);
        GameButton.MouseDown += GameButtonNextRound;
    }

    private async Task AnimateMessageBand(double height)
    {
        var animation = new DoubleAnimation(height, new Duration(TimeSpan.FromMilliseconds(200)));

        Storyboard.SetTargetName(animation, MessageBar.Name);
        Storyboard.SetTargetProperty(animation, new PropertyPath("Height"));

        var story = new Storyboard();
        story.Children.Add(animation);
        await story.BeginAsync(MessageBar);

        story.Remove();
    }

    public async Task HighlightSimonButton(SimonButton button)
    {
        var border = _buttons[button];
        var animation = new DoubleAnimation(0, 0.75, new Duration(TimeSpan.FromMilliseconds(100)));

        Storyboard.SetTargetName(animation, button.ToString());
        Storyboard.SetTargetProperty(animation, new PropertyPath("Background.GradientStops[1].Offset"));

        var story = new Storyboard();
        story.Children.Add(animation);
        await story.BeginAsync(border);

        story.Remove();
    }

    private void Blue_MouseDown(object sender, MouseButtonEventArgs e)
    {
        OnSimonButtonClicked(SimonButton.Blue);
        e.Handled = true;
    }

    private void Yellow_MouseDown(object sender, MouseButtonEventArgs e)
    {
        OnSimonButtonClicked(SimonButton.Yellow);
        e.Handled = true;
    }

    private void Green_MouseDown(object sender, MouseButtonEventArgs e)
    {
        OnSimonButtonClicked(SimonButton.Green);
        e.Handled = true;
    }

    private void Red_MouseDown(object sender, MouseButtonEventArgs e)
    {
        OnSimonButtonClicked(SimonButton.Red);
        e.Handled = true;
    }

    private async void GameButtonStartGame(object sender, MouseButtonEventArgs e)
    {
        await AnimateMessageBand(0);
        e.Handled = true;

        GameButton.MouseDown -= GameButtonStartGame;

        var handler = PlayNextRound;
        if (handler != null)
        {
            handler(this, EventArgs.Empty);
        }
        EnableButtons();
    }

    public event EventHandler PlayNextRound;
    private async void GameButtonNextRound(object sender, MouseButtonEventArgs e)
    {
        await AnimateMessageBand(0);
        e.Handled = true;

        var handler = PlayNextRound;
        if (handler != null)
        {
            handler(this, EventArgs.Empty);
        }

        GameButton.MouseDown -= GameButtonNextRound;
        EnableButtons();
    }

    private void GameButtonEndGame(object sender, MouseButtonEventArgs e)
    {
        Close();
        e.Handled = true;
    }
}

I'm using this extension method I borrowed on Stack Overflow (had to tweak it a little), to make Storyboard animations awaitable and avoid having all animations running simultaneously:

public static class StoryboardExtensions
{
    public static Task BeginAsync(this Storyboard storyboard, FrameworkElement containingObject)
    {
        var source = new TaskCompletionSource<bool>();
        if (storyboard == null)
            source.SetException(new ArgumentNullException());
        else
        {
            EventHandler onComplete = null;
            onComplete = (sender, args) =>
            {
                storyboard.Completed -= onComplete;
                source.SetResult(true);
            };
            storyboard.Completed += onComplete;
            containingObject.Dispatcher.Invoke(() => storyboard.Begin(containingObject));
        }
        return source.Task;
    }
}

Answer 1

1. Well, first questions is: why don't you use MVVM? Contents of your App class look like something that should be implemented on model/view model level.

2. This looks like a lot of copy-pasting

   private void Blue_MouseDown(object sender, MouseButtonEventArgs e)
   {
       OnSimonButtonClicked(SimonButton.Blue);
       e.Handled = true;
   }
   
   private void Yellow_MouseDown(object sender, MouseButtonEventArgs e)
   {
       OnSimonButtonClicked(SimonButton.Yellow);
       e.Handled = true;
   }
   
   ....
   

You should use single event handler for your buttons. For example, you can set button type to FrameworkElement.Tag in XAML, and access it as ((FrameworkElement)sender).Tag in code behind. You can also refactor those into commands, if you were to use bindings instead of events.

3. I don't like your EnableButtons and DisableButtons methods, they feel wrong. I would bind IsEnabled property of your buttons to some bool dependency property (or viewmodel property), and set it to true/false instead. And I would then check this property in the event handler.

4. You should use some static method to do this:

   var handler = myEvent;
   if (handler != null)
   {
       handler(this, myArgs);
   }
   

5. I don't know the whole story, but I don't quite understand why do you need to await for animations. It only makes your code more complex. Also, I think that managing and defining storyboards is a lot easier in XAML than in code-behind, but, it might be a matter of taste.

Answer 2

• Magic numbers, you have at least one of them in class App: private async Task PlaySequence() you should extract this to either a constant or better to a property, so that the speed, aka less sleep, can be adjusted.

• I don't see the point of the _seed variable. Yeah, I know why you use it, but it just forces you, each time the GenerateSequence() method is called, to recreate the IEnumerable<SimonButton>. Consider to append the "new" item for the sequence to the end of the sequence.

• each time calling Enum.GetValues(typeof(SimonButton)).GetLength(0) at creating the random is not necessary either. Extract it to a variable and reuse it, it won't change ;-)
• assigning the DataContext two times might be a copy & paste error
• The whole GenerateSequence() method should live in the SimonSaysRound class. You wouldn't need to access Score and Length properties.
• SimonSaysRound could also get rid of the SimonButton[] because it could use an IEnumerable<> instead. This will also result in Length property getting obsolete.

Refactoring the SimonSaysRound class by implementing the points above will lead to

public class SimonSaysRound : INotifyPropertyChanged
{
    private const int PointsForGoodMatch = 5;
    private readonly int _buttonCount = Enum.GetValues(typeof(SimonButton)).GetLength(0);

    private IEnumerable<SimonButton> _sequence = Enumerable.Empty<SimonButton>();
    private IEnumerator<SimonButton> _enumerator;

    private int _matches=0;
    private int _score=0;
    private int _currentlength = 0;

    private Random _random = new Random();

    /// <summary>
    /// Use this method, to start a new game.
    /// </summary>
    public void Reset()
    {
        _matches = 0;
        _score = 0;
        _currentlength = 0;
        _sequence = Enumerable.Empty<SimonButton>();
    }

    public event EventHandler<SimonSaysScoreEventArgs> RoundCompleted;
    public void OnRoundCompleted()
    {
        var handler = RoundCompleted;
        if (handler != null)
        {
            var result = _matches == _currentlength;
            RoundCompleted(this, new SimonSaysScoreEventArgs(result, Score));
        }
    }

    public void Play(SimonButton button)
    {
        _enumerator.MoveNext();
        var success = _enumerator.Current == button;
        if (success)
        {
            Score += PointsForGoodMatch;
            _matches++;
        }

        if (!success || _matches == _currentlength)
        {
            OnRoundCompleted();
        }
    }

    public int Round
    {
        get { return _currentlength; }
    }

    public int Score
    {
        get { return _score; }
        private set
        {
            if (value == _score) return;
            _score = value;
            OnPropertyChanged();
        }
    }

    public IEnumerable<SimonButton> Sequence { get { return _sequence; } }

    public event PropertyChangedEventHandler PropertyChanged;

    [NotifyPropertyChangedInvocator]
    protected virtual void OnPropertyChanged([CallerMemberName] string propertyName = null)
    {
        PropertyChangedEventHandler handler = PropertyChanged;
        if (handler != null) handler(this, new PropertyChangedEventArgs(propertyName));
    }

    public void PlayNextRound()
    {
        _matches = 0;
        _currentlength += 1;
        _sequence = _sequence.Concat(GenerateNextSequenceItem());
        _enumerator = _sequence.GetEnumerator();

    }

    private IEnumerable<SimonButton> GenerateNextSequenceItem()
    {
        yield return (SimonButton)_random.Next(_buttonCount);
    }      
}

and on the calling side

private void PlayNextRound()
{
    if (_currentRound == null)
    {
        _currentRound = new SimonSaysRound();
    }
    _currentRound.PlayNextRound();
    _currentRound.RoundCompleted += _currentRound_RoundCompleted;

    _mainWindow.DataContext = _currentRound;

    PlaySequence();
}

You should consider to initialize the _currentRound on class level making the null check obsolete.

• combining the start of a game and the start of the next round by raising the same event without to distinguish between them by using different event arguments, is IMHO not very good.

• you have duplicate code in the GameButtonStartGame() and GameButtonNextRound() methods. Either remove one of the methods and change the eventhandler or better extract the code to a separate method which is called form both handlers.

Answer 3

The naming is inconsistent with the rest of the code in the borrowed extension method:

    EventHandler onComplete = null;
    onComplete = (sender, args) =>
    {
        storyboard.Completed -= onComplete;
        source.SetResult(true);
    };
    storyboard.Completed += onComplete;
    containingObject.Dispatcher.Invoke(() => storyboard.Begin(containingObject));

OnSimonButtonClicked and every single OnEventName method you've ever written calls the handler handler:

    var handler = SimonButtonClicked;
    if (handler != null)
    {
        handler.Invoke(this, new SimonButtonEventArgs(button));
    }

This would be more consistent then - notice the = null assignment is redundant and has been removed:

    EventHandler handler;
    handler = (sender, args) =>
    {
        storyboard.Completed -= handler;
        source.SetResult(true);
    };
    storyboard.Completed += handler;
    containingObject.Dispatcher.Invoke(() => storyboard.Begin(containingObject));