Problem
Some time ago I had to make a memory game as a college project. I spent around 6hrs coding this since I was also busy juggling exams. I’m not experienced in C# so there are probably a lot of things I could’ve done better, and I am also bad at conjuring algorithms. So please let me know on what I did right and what I did wrong here and how to prevent these mistakes in my future projects.
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Text;
using System.Windows.Forms;
using System.Security.Cryptography;
using System.IO;
using System.Reflection;
using System.Xml.Serialization;
namespace Memory_Game
{
public partial class MemoryGame : Form
{
private const int CARDS = 8;
private const int DUPLICATE_COUNT = 2;
private const int UNFLIPS = 2;
private const string SCORES_FILENAME = "scores.xml";
private RNGCryptoServiceProvider provider = new RNGCryptoServiceProvider(); // rng
private int flipCount = 0; // I could've probably used cardsToCheck.Count instead of making another variable but I don't quite trust myself
private List<Card> cardsToCheck = new List<Card>(); // List which takes flipped cards so that only they are checked if they are pairs, instead of traversing the whole board
// also Card class is nothing more than an inherited button class with two boolean properties (Flipped, Paired)
private bool cardsMatch = false; // I have no idea why this is global, it's only used in one place
private int matchCount = 0; // counts the pairs which is then displayed in game
private int gameTime = 0; // counts game time in seconds and is then formatted and displayed in game
private int unflipsLeft = 0; // unflips - bonus "feature" which allows the player to un-flip a flipped card(basically undo), this came about from my lack of knowledge of memory games
private bool paused = false; // pretty self explanatory
private int totalFlipsCounter = 0; // also self explanatory, counts total flips made in the game
private string playerName = "noname";
private Timer gameTimer = new Timer();
// All game images. I had originally planned on having an XML file and allow players to add their own images, but time constraints and
// lack of experience led me to scrap that
#region images
System.Drawing.Bitmap triangle = Properties.Resources.triangle;
System.Drawing.Bitmap circle = Properties.Resources.circle;
System.Drawing.Bitmap square = Properties.Resources.square;
System.Drawing.Bitmap hexagon = Properties.Resources.hexagon;
System.Drawing.Bitmap diamond = Properties.Resources.diamond;
System.Drawing.Bitmap star = Properties.Resources.star;
System.Drawing.Bitmap halfCircle = Properties.Resources.halfCircle;
System.Drawing.Bitmap crescent = Properties.Resources.crescent;
#endregion
public MemoryGame()
{
InitializeComponent();
this.tableLayoutPanel.Visible = false;
this.bttnPause.Enabled = false;
this.menuGamePause.Enabled = false;
this.bttnRestart.Text = "Start";
gameTimer.Tick += new EventHandler(gameTimer_onTick);
gameTimer.Interval = 1000; //ms
}
private void gameTimer_onTick(Object sender, EventArgs e)
{
// I believe I'm doing way too much in this event however I have no idea of any other way I could do all those checks
// checking for game over on click would a lot more mess to already messy on click event method imho
this.lblTimer.Text = formatTime(gameTime);
if (matchCount == CARDS)
{
this.gameTimer.Stop();
this.bttnPause.Enabled = false;
this.menuGamePause.Enabled = false;
int score = calculateScore();
MessageBox.Show("You finished in: " + formatTime(gameTime)
+ "n With " + totalFlipsCounter + " total flips"
+ "n Total Score: " + score, "Finished!", MessageBoxButtons.OK);
XmlSerializer serializer = new XmlSerializer(typeof(List<PlayerScore>));
if (File.Exists(SCORES_FILENAME))
{
List<PlayerScore> scoreList = new List<PlayerScore>();
using (FileStream stream = File.OpenRead(SCORES_FILENAME))
{
scoreList = (List<PlayerScore>)serializer.Deserialize(stream);
}
if (scoreList.Count < 10)
{
bool alreadyAdded = false;
for (int i = 0; i < scoreList.Count; i++)
{
if (score > scoreList[i].Score)
{
EnterNameForm dialogEnterName = new EnterNameForm();
dialogEnterName.Owner = this;
dialogEnterName.ShowDialog();
scoreList.Insert(i, new PlayerScore(playerName, score));
alreadyAdded = true;
break;
}
}
if (!alreadyAdded)
{
EnterNameForm dialogEnterName = new EnterNameForm();
dialogEnterName.Owner = this;
dialogEnterName.ShowDialog();
scoreList.Add(new PlayerScore(playerName, score));
}
using (FileStream stream = File.OpenWrite(SCORES_FILENAME))
{
serializer.Serialize(stream, scoreList);
}
}
else
{
for (int i = 0; i < scoreList.Count; i++)
{
if (score > scoreList[i].Score)
{
EnterNameForm dialogEnterName = new EnterNameForm();
dialogEnterName.Owner = this;
dialogEnterName.ShowDialog();
scoreList.RemoveAt(scoreList.Count - 1);
scoreList.Insert(i, new PlayerScore(playerName, score));
break;
}
}
File.Delete(SCORES_FILENAME);
using (FileStream stream = File.Create(SCORES_FILENAME))
{
serializer.Serialize(stream, scoreList);
}
}
}
else
{
using (FileStream stream = File.Create(SCORES_FILENAME))
{
List<PlayerScore> scoreList = new List<PlayerScore>();
EnterNameForm dialogEnterName = new EnterNameForm();
dialogEnterName.Owner = this;
dialogEnterName.ShowDialog();
scoreList.Add(new PlayerScore(playerName, score));
serializer.Serialize(stream, scoreList);
}
}
}
gameTime++;
}
protected internal void getPlayerName(string name)
{
playerName = name;
}
private string formatTime(int timeInSeconds)
{
TimeSpan time = TimeSpan.FromSeconds(timeInSeconds);
return string.Format("{0:D2}:{1:D2}:{2:D2}", time.Hours, time.Minutes, time.Seconds);
}
private int calculateScore()
{
double timeScore = 100f * (20f/(double)gameTime);
int unflipBonusScore = 20 * unflipsLeft;
double flipScore = 500f * (20f / (double)totalFlipsCounter);
return (int)(timeScore + unflipBonusScore + flipScore);
}
private void Pause()
{
if (!this.tableLayoutPanel.Visible) return;
if (this.gameTimer.Enabled) this.gameTimer.Enabled = false;
foreach (Control card in this.tableLayoutPanel.Controls)
{
if (((Card)card).Enabled) ((Card)card).Enabled = false;
}
this.paused = true;
this.bttnPause.Text = "Continue";
}
private void UnPause()
{
if (!this.tableLayoutPanel.Visible) return;
if (!this.gameTimer.Enabled) this.gameTimer.Enabled = true;
foreach (Control card in this.tableLayoutPanel.Controls)
{
if (!((Card)card).Enabled) ((Card)card).Enabled = true;
}
this.paused = false;
this.bttnPause.Text = "Pause";
}
private void Restart()
{
// Also somewhat messy
// Restart is responsible for randomly generating cards and their positions
// It's done by creating a string list which contains all card names
// That list is then shuffled using Fisher - Yates shuffle, whose code I conveniently stole from http://stackoverflow.com/a/1262619/1182835
// afterwards those names are then added to each cards tags
// there is a glaring problem here in that while I tried making most of my code here extensible this is quite static
// since I've manually typed only 8 cards if I wanted to have more or less I'd have to refactor the code
if (!this.tableLayoutPanel.Visible) this.tableLayoutPanel.Visible = true;
if (!this.bttnPause.Enabled) this.bttnPause.Enabled = true;
if (!this.menuGamePause.Enabled) this.menuGamePause.Enabled = true;
if (this.bttnRestart.Text == "Start") this.bttnRestart.Text = "Restart";
if ((this.matchCount > 0 || this.flipCount > 0) && matchCount != CARDS)
{
Pause();
DialogResult result = MessageBox.Show("Are you sure you want to restart the current game?n You will lose all progress", "Restart?", MessageBoxButtons.YesNo);
if (result == DialogResult.No)
{
UnPause();
return;
}
}
gameTime = 0;
matchCount = 0;
unflipsLeft = UNFLIPS;
totalFlipsCounter = 0;
this.lblUnflipIndicator.Text = "Unflips: " + unflipsLeft;
this.lblFlipCounter.Text = "Flips: " + totalFlipsCounter;
List<string> boardTags = new List<string>();
for (int i = 0; i < DUPLICATE_COUNT; i++)
{
boardTags.Add("Triangle");
boardTags.Add("Circle");
boardTags.Add("Square");
boardTags.Add("Hexagon");
boardTags.Add("Diamond");
boardTags.Add("Star");
boardTags.Add("HalfCircle");
boardTags.Add("Crescent");
}
Shuffle(boardTags);
int c = 0;
foreach (Control bttn in this.tableLayoutPanel.Controls)
{
((Card)bttn).Tag = boardTags[c < boardTags.Count - 1 ? c++ : c];
((Card)bttn).Image = global::Memory_Game.Properties.Resources.cardBg;
((Card)bttn).Flipped = false;
((Card)bttn).Paired = false;
flipCount = 0;
}
this.gameTimer.Start();
if (this.paused) UnPause();
}
private void Shuffle<T>(List<T> list)
{
int n = list.Count;
while (n > 1)
{
byte[] box = new byte[1];
do provider.GetBytes(box);
while (!(box[0] < n * (Byte.MaxValue / n)));
int k = (box[0] % n);
n--;
T value = list[k];
list[k] = list[n];
list[n] = value;
}
}
private void card_Clicked(object sender, EventArgs e)
{
if (((Card)sender).Paired) return;
// In this method i check if 2 cards are flipped twice!
// This first one is a check when there are two cards flipped and the player is trying to flip a third card
// in that case the previous two flipped cards are reverted granted that they weren't paired before
// cause you will see further down that when two cards are paired they are removed from the check list
if (flipCount == DUPLICATE_COUNT && !((Card)sender).Paired && !((Card)sender).Flipped)
{
flipCount = 0;
foreach (Card card in cardsToCheck)
{
card.Image = global::Memory_Game.Properties.Resources.cardBg;
card.Flipped = false;
card.Paired = false;
}
cardsToCheck.Clear();
}
if (!((Card)sender).Flipped && flipCount < DUPLICATE_COUNT)
{
((Card)sender).Flipped = true;
flipCount++;
totalFlipsCounter++;
switch (((Card)sender).Tag.ToString()) //Here is where the card gets its image according to its tag
{
case "Triangle": ((Card)sender).Image = triangle; break;
case "Circle": ((Card)sender).Image = circle; break;
case "Square": ((Card)sender).Image = square; break;
case "HalfCircle": ((Card)sender).Image = halfCircle; break;
case "Hexagon": ((Card)sender).Image = hexagon; break;
case "Star": ((Card)sender).Image = star; break;
case "Crescent": ((Card)sender).Image = crescent; break;
case "Diamond": ((Card)sender).Image = diamond; break;
// this is missing a default case, if card by any chance didn't get a tag we'd encounter some problems
}
cardsToCheck.Add(((Card)sender));
}
else if (((Card)sender).Flipped && unflipsLeft > 0)
{
((Card)sender).Flipped = false;
((Card)sender).Image = global::Memory_Game.Properties.Resources.cardBg;
flipCount--;
totalFlipsCounter--;
cardsToCheck.Remove(((Card)sender));
unflipsLeft--;
this.lblUnflipIndicator.Text = "Unflips: " + unflipsLeft;
}
if (flipCount == DUPLICATE_COUNT) // I hate having to do this check twice
{
cardsMatch = checkForMatch();
if (cardsMatch)
{
cardsToCheck[0].Paired = true;
cardsToCheck[1].Paired = true;
matchCount++;
cardsToCheck.Clear();
flipCount = 0;
}
}
this.lblFlipCounter.Text = "Flips: " + totalFlipsCounter;
}
private bool checkForMatch()
{
if (cardsToCheck.Count != 2) return false;
if (cardsToCheck[0].Tag == cardsToCheck[1].Tag) return true;
return false;
}
private void bttnRestart_Click(object sender, EventArgs e)
{
Restart();
}
private void bttnPause_Click(object sender, EventArgs e)
{
if (this.paused) UnPause();
else Pause();
}
private void menuGameExit_Click(object sender, EventArgs e)
{
this.Close();
}
private void showHighScoresToolStripMenuItem_Click(object sender, EventArgs e)
{
if (this.bttnRestart.Text != "Start" && matchCount != CARDS) Pause();
HighScoresList hsForm = new HighScoresList();
hsForm.Owner = this;
hsForm.ShowDialog();
}
}
}
Solution
how to prevent these mistakes in my future projects.
Rewrite your program from scratch
- Rewrite, from scratch, several times, as you progress in your skill and knowledge.
- There is no absolutely correct code solution. Your goal in re-writing is not to make it perfect but see how the code evolves – how it’s easier to write, understand, change – as you apply good Object Oriented Programming practices and principles.
- I guarantee the before and after picture will be an eye opener.
- It’s the journey, not the end.
Let Reality be Your Guide
- Build your code to refelct the real world of your problem. It will be more understandable and intuitive.
- If the realities of writing detail code, how you have to write in the language to make things work, obscure what it’s doing in reality then hide it in other methods and classes that do a better job of saying things in real terms.
Classes
- Go overboard on making classes. Do the opposite of your sample code. The longer term goal is to see how lots of well focused classes makes the program better. Better to build, better to understand, better to modify.
- Possibly making too many classes is fine for now. This is a learning process. Learn to think in terms of objects.
- Work hard on puting things into the proper class. Very hard.
- If it’s a noun, make a class.
- If it’s a thing with some kind of independent identity, real or conceptual, make a class.
- Seriously, go overboard on making classes. You will find it is not as overboard as you first thought.
- Think of what you do and what you do it to as separate things, generally.
Structure Your Classes
- Everything is made of parts and so should your code.
- Use class and data structure to make manipulating things easier.
- Use class and data structure to make things reflect it’s reality.
- Knowing classic data structures – Arrays, Collections, trees, etc. is absolutely indespensible.
- Composing your “lots of” classes in a way that the parts do their individual things yet are a coherent whole is absolutely indespensible.
- The Human Body is Object Oriented. The body (a class) has clearly defined connections (interfaces) inside itself to interact with other internal parts (classes); and interfaces on the outside to interact with the outside world. The parts are specialized and fundamentally independent. And you could say that eyes, ears, mouth, etc. are Application Public Interfaces (APIs).
Get a good overview of C#
- You will run across things that give you that “A-Ha, I can use that” moments.
- Over time you will be learning how to translate your real world problems into C# code details.
Complexity and Simplicity
- We’re tackling complex problems with code. OO does not discard complexity, rather it manages complexity.
- Simplicity is complexity broken into understandable parts.
- Classes and their interaction makes the problem space manageable.
- Limiting the number of classes and their interactions for the sake of simplicity misses the point.
- Object oriented guidelines, generally, are fractal; OO principles can apply at the conditional-code, method, class, module, assembly levels.
Sample Application to Your Code
- Player, Card, CardDeck could all be classes.
- GetPlayerName() could be in the Player class.
- A Card is not a pile of Cards. A CardDeck is a pile of cards.
- The CardDeck could be a stack. Cards come off the top of the deck. This is a quintessential behavior of a stack data structure.
- A stack of cards does not shuffle, flip, etc. itself. Something or someone has to be doing that however.
- Something has to be working the interaction of the card(s) and player(s).
- “Something” and “Someone” may very well be their own classes.
What you did right:
- Good variable names
- Good use of private members
- Good method names
- Early exits on
PauseandUnPause - Reasonably consistent naming style
- Clever inheriting Button for Card
What you did not-so-right:
- As you pointed out, some of your event handlers are quite long. Even some simple extract method refactoring would go a long way to making it more readable.
- No real architecture or design here. It’s a sample project, so that may be overkill – but it’d be nice to see a few other classes besides
Card. It would give you a place to put those decomposed methods into as well. - Storing state in the UI itself without an abstraction (
.Text,.Tag, etc.). This often starts out as the “simplest possible thing” – but usually quickly ends up in a tangled mess. You may want to consider abstracting out aViewModel.
As an example of an extract method refactoring on your gameTimer_Click handler:
private void gameTimer_onTick(Object sender, EventArgs e)
{
this.lblTimer.Text = formatTime(gameTime);
if (matchCount != CARDS) return; // prefer early exit to deeply nested ifs
this.gameTimer.Stop();
this.bttnPause.Enabled = false;
this.menuGamePause.Enabled = false;
int score = calculateScore();
MessageBox.Show(
string.Format(FINISH_TEXT, totalFlipsCounter, score), // abstract out long text; makes the code cleaner and eases changing it later
MessageBoxButtons.OK
);
SaveHighScore(score);
gameTime++;
}
// saving the score is long, and a discrete op. Perfect for a method extraction.
void SaveHighScore(int newScore)
{
const int MAX_SCORES = 10;
XmlSerializer serializer = new XmlSerializer(typeof(List<PlayerScore>));
List<PlayerScore> scoreList; // don't new if you're not going to use it
if (File.Exists(SCORES_FILENAME)) // make if branches in a method as small as possible and then join back up to your main logic
{
using (FileStream stream = File.OpenRead(SCORES_FILENAME))
{
scoreList = (List<PlayerScore>)serializer.Deserialize(stream);
}
}
slse
{
scoreList = new List<PlayerScore>();
}
// It *looks* like this is supposed to add the newScore in order - kind of hard to tell
// since there's 3 almost-identical blocks.
// A SortedList would work well for this - but I don't have Intellisense handy and don't
// use it enough to know it off-hand, so I'll use LINQ (which for 10 scores won't be a problem)
scoreList.Add(new PlayerScore(GetPlayerName(), score));
var topScoresList = scoreList.OrderByDescending(p => p.Score).Take(MAX_SCORES).ToList();
using (FileStream stream = File.OpenOrCreate(SCORES_FILENAME)) // don't delete when you can just overwrite...
{
serializer.Serialize(stream, topScoresList);
}
}
string GetPlayerName()
{
using (EnterNameForm dialogEnterName = new EnterNameForm())
{
dialogEnterName.Owner = this;
dialogEnterName.ShowDialog();
}
// not sure how playerName gets set? Spooky action-at-a-distance from EnterNameForm and Owner or Form?
// Would be better for this Form to grab it from the dialog...
return playerName;
}