Simon Says: "Make me a pretty game"

In Memoriam


Ralph H. Baer, co-inventor of the original "Simon" game, died Saturday December 6th 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 sequence, int score)
{
_sequence = sequence.ToArray();
_score = score;
_matches = 0;
}

public event EventHandler 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;

• 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.

-
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.

-
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.

-
You should use some static method to do this:

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

-
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.

StackExchange Code Review Q#71993, answer score: 18