patterncsharpMinor
First C# program (Snake game)
Viewed 0 times
gamefirstprogramsnake
Problem
I've just started out using C#. I've used Scratch (drag and drop programming for kids) for quite some time. Since Scratch didn't have classes and methods I have a feeling this code could be a lot more streamlined, neat, efficient, more read-able, shorter and in general just better.
```
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Threading;
///█ ■
////https://www.youtube.com/watch?v=SGZgvMwjq2U
namespace Snake
{
class Program
{
static void Main(string[] args)
{
Console.WindowHeight = 16;
Console.WindowWidth = 32;
int screenwidth = Console.WindowWidth;
int screenheight = Console.WindowHeight;
Random randomnummer = new Random();
int score = 5;
int gameover = 0;
pixel hoofd = new pixel();
hoofd.xpos = screenwidth/2;
hoofd.ypos = screenheight/2;
hoofd.schermkleur = ConsoleColor.Red;
string movement = "RIGHT";
List xposlijf = new List();
List yposlijf = new List();
int berryx = randomnummer.Next(0, screenwidth);
int berryy = randomnummer.Next(0, screenheight);
DateTime tijd = DateTime.Now;
DateTime tijd2 = DateTime.Now;
string buttonpressed = "no";
while (true)
{
Console.Clear();
if (hoofd.xpos == screenwidth-1 || hoofd.xpos == 0 ||hoofd.ypos == screenheight-1 || hoofd.ypos == 0)
{
gameover = 1;
}
for (int i = 0;i 500) { break; }
if (Console.KeyAvailable)
{
ConsoleKeyInfo toets = Console.ReadKey(true);
//Console.WriteLine(toets.Key.ToString());
if (toets.Key.Equals(ConsoleKey.UpArrow) && movement != "DOWN" &
```
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Threading;
///█ ■
////https://www.youtube.com/watch?v=SGZgvMwjq2U
namespace Snake
{
class Program
{
static void Main(string[] args)
{
Console.WindowHeight = 16;
Console.WindowWidth = 32;
int screenwidth = Console.WindowWidth;
int screenheight = Console.WindowHeight;
Random randomnummer = new Random();
int score = 5;
int gameover = 0;
pixel hoofd = new pixel();
hoofd.xpos = screenwidth/2;
hoofd.ypos = screenheight/2;
hoofd.schermkleur = ConsoleColor.Red;
string movement = "RIGHT";
List xposlijf = new List();
List yposlijf = new List();
int berryx = randomnummer.Next(0, screenwidth);
int berryy = randomnummer.Next(0, screenheight);
DateTime tijd = DateTime.Now;
DateTime tijd2 = DateTime.Now;
string buttonpressed = "no";
while (true)
{
Console.Clear();
if (hoofd.xpos == screenwidth-1 || hoofd.xpos == 0 ||hoofd.ypos == screenheight-1 || hoofd.ypos == 0)
{
gameover = 1;
}
for (int i = 0;i 500) { break; }
if (Console.KeyAvailable)
{
ConsoleKeyInfo toets = Console.ReadKey(true);
//Console.WriteLine(toets.Key.ToString());
if (toets.Key.Equals(ConsoleKey.UpArrow) && movement != "DOWN" &
Solution
Ok so this is very cool code. I learned a lot looking at it myself. However, there is room for improvement. (When isn't there?) I wrote down the comments below while looking though your code and refactoring it. You can consider it my thought process in a sense.
Naming things
Let's start with the obvious: naming. .NET convention is to use
Also, use English !! It makes programming easier. Quickly looking at your code I have no idea what each variable does because it is in Dutch (apparently).
Your new
Note : I added a constructor, too. It makes creating a new
Your busy (too busy, but I am coming to that) list of declarations in the beginning becomes something like this:
You'll notice a few things (in addition to Dutch to English translation): I organized it so I grouped the related variables together. And I used
using static
You're using the
What this leads to, almost directly, is that you don't need your
Using your classes properly
Here is what I do not understand. Why did you create the
I still have not looked at the rest of your code by this point but I bet you can make it easier on yourself if you used the class you created properly. Read the rest of my review with that in mind.
Drawing the border
As other answerers kindly said, your loop is too long. Split it into useful functions so you can separate it and read it better. For example, this portion:
Is simply drawing the border. Right? Select it all, click
Since you're also using the same conditions for some of the loops, why not combine them? It becomes this:
Type safety
gameover and buttonPressed
You're using
Direction
You're using
You can then use that for your
C# gives you type safety. Use it. You don't want to a
Naming things
Let's start with the obvious: naming. .NET convention is to use
PascalCase for classes, methods, and propetries; and camelCase for variables. The most obvious culprit is your class pixel, which should be named Pixel. It sounds like a trivial thing but it is really important when reading code.Also, use English !! It makes programming easier. Quickly looking at your code I have no idea what each variable does because it is in Dutch (apparently).
Your new
Pixel class then should look something like this:class Pixel
{
public Pixel (int xPos, int yPos, ConsoleColor color)
{
XPos = xPos;
YPos = yPos;
ScreenColor = color;
}
public int XPos { get; set; }
public int YPos { get; set; }
public ConsoleColor ScreenColor { get; set; }
}Note : I added a constructor, too. It makes creating a new
Pixel cleaner. I also feel the class would be better as a struct, but let's not go there.Your busy (too busy, but I am coming to that) list of declarations in the beginning becomes something like this:
Console.WindowHeight = 16;
Console.WindowWidth = 32;
var screenWidth = Console.WindowWidth;
var screenHeight = Console.WindowHeight;
var rand = new Random ();
var score = 5;
var gameover = 0;
var head = new Pixel (screenWidth / 2, screenHeight / 2, ConsoleColor.Red);
var xPosBody = new List ();
var yPosBody = new List ();
var xPosBerry = rand.Next (0, screenWidth);
var yPosBerry = rand.Next (0, screenHeight);
var time = DateTime.Now;
var time2 = DateTime.Now;
var movement = "RIGHT";
var buttonPressed = "no";You'll notice a few things (in addition to Dutch to English translation): I organized it so I grouped the related variables together. And I used
var instead of using the full class name. The latter is a stylistic issue, but var is more readable, imo.using static
You're using the
Console class a lot. With C# 6, you can make life easier for yourself and write using static System.Console; at the top of your file. And then you don't have to suffix every call to the Console class with the class name. Less typing. Yay.What this leads to, almost directly, is that you don't need your
screenWidth and screenHeight variables any more. Personally, I think the less variables flying around the better. YMMV. I will remove them from subsequent code in this review.Using your classes properly
Here is what I do not understand. Why did you create the
Pixel class, then you refer to the body positions and the berry position as lists of numbers ? Why not use the class you already created?I still have not looked at the rest of your code by this point but I bet you can make it easier on yourself if you used the class you created properly. Read the rest of my review with that in mind.
Drawing the border
As other answerers kindly said, your loop is too long. Split it into useful functions so you can separate it and read it better. For example, this portion:
for (int i = 0; i < WindowWidth; i++)
{
Console.SetCursorPosition (i, 0);
Console.Write ("■");
}
for (int i = 0; i < WindowWidth; i++)
{
Console.SetCursorPosition (i, WindowHeight - 1);
Console.Write ("■");
}
for (int i = 0; i < WindowHeight; i++)
{
Console.SetCursorPosition (0, i);
Console.Write ("■");
}
for (int i = 0; i < WindowHeight; i++)
{
Console.SetCursorPosition (WindowWidth - 1, i);
Console.Write ("■");
}Is simply drawing the border. Right? Select it all, click
Ctrl + ., then choose Extract method. You can call it DrawBorder(). Note you dont need to use Console here at all, too, due to using static.Since you're also using the same conditions for some of the loops, why not combine them? It becomes this:
static void DrawBorder ()
{
for (int i = 0; i < WindowWidth; i++)
{
SetCursorPosition (i, 0);
Write ("■");
SetCursorPosition (i, WindowHeight - 1);
Write ("■");
}
for (int i = 0; i < WindowHeight; i++)
{
SetCursorPosition (0, i);
Write ("■");
SetCursorPosition (WindowWidth - 1, i);
Write ("■");
}
}Type safety
gameover and buttonPressed
You're using
gameover as a yes/no value. Don't use an int for that. Use a bool. Same thing with buttonPressed.Direction
You're using
string values to indicate direction. These values cannot be other than four particular cases. This is the perfect use case from an enum type.enum Direction
{
Up,
Down,
Right,
Left
}You can then use that for your
movement variable. It becomes :var movement = Direction.Right;C# gives you type safety. Use it. You don't want to a
Code Snippets
class Pixel
{
public Pixel (int xPos, int yPos, ConsoleColor color)
{
XPos = xPos;
YPos = yPos;
ScreenColor = color;
}
public int XPos { get; set; }
public int YPos { get; set; }
public ConsoleColor ScreenColor { get; set; }
}Console.WindowHeight = 16;
Console.WindowWidth = 32;
var screenWidth = Console.WindowWidth;
var screenHeight = Console.WindowHeight;
var rand = new Random ();
var score = 5;
var gameover = 0;
var head = new Pixel (screenWidth / 2, screenHeight / 2, ConsoleColor.Red);
var xPosBody = new List<int> ();
var yPosBody = new List<int> ();
var xPosBerry = rand.Next (0, screenWidth);
var yPosBerry = rand.Next (0, screenHeight);
var time = DateTime.Now;
var time2 = DateTime.Now;
var movement = "RIGHT";
var buttonPressed = "no";for (int i = 0; i < WindowWidth; i++)
{
Console.SetCursorPosition (i, 0);
Console.Write ("■");
}
for (int i = 0; i < WindowWidth; i++)
{
Console.SetCursorPosition (i, WindowHeight - 1);
Console.Write ("■");
}
for (int i = 0; i < WindowHeight; i++)
{
Console.SetCursorPosition (0, i);
Console.Write ("■");
}
for (int i = 0; i < WindowHeight; i++)
{
Console.SetCursorPosition (WindowWidth - 1, i);
Console.Write ("■");
}static void DrawBorder ()
{
for (int i = 0; i < WindowWidth; i++)
{
SetCursorPosition (i, 0);
Write ("■");
SetCursorPosition (i, WindowHeight - 1);
Write ("■");
}
for (int i = 0; i < WindowHeight; i++)
{
SetCursorPosition (0, i);
Write ("■");
SetCursorPosition (WindowWidth - 1, i);
Write ("■");
}
}enum Direction
{
Up,
Down,
Right,
Left
}Context
StackExchange Code Review Q#127515, answer score: 8
Revisions (0)
No revisions yet.