HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

First C# program (Snake game)

Submitted by: @import:stackexchange-codereview··
0
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" &

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