14
\$\begingroup\$

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<int> xposlijf = new List<int>();
            List<int> yposlijf = new List<int>();
            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< screenwidth; i++)
                {
                    Console.SetCursorPosition(i, 0);
                    Console.Write("■");
                }
                for (int i = 0; i < screenwidth; i++)
                {
                    Console.SetCursorPosition(i, screenheight -1);
                    Console.Write("■");
                }
                for (int i = 0; i < screenheight; i++)
                {
                    Console.SetCursorPosition(0, i);
                    Console.Write("■");
                }
                for (int i = 0; i < screenheight; i++)
                {
                    Console.SetCursorPosition(screenwidth - 1, i);
                    Console.Write("■");
                }
                Console.ForegroundColor = ConsoleColor.Green;
                if (berryx == hoofd.xpos && berryy == hoofd.ypos)
                {
                    score++;
                    berryx = randomnummer.Next(1, screenwidth-2);
                    berryy = randomnummer.Next(1, screenheight-2);
                } 
                for (int i = 0; i < xposlijf.Count(); i++)
                {
                    Console.SetCursorPosition(xposlijf[i], yposlijf[i]);
                    Console.Write("■");
                    if (xposlijf[i] == hoofd.xpos && yposlijf[i] == hoofd.ypos)
                    {
                        gameover = 1;
                    }
                }
                if (gameover == 1)
                {
                    break;
                }
                Console.SetCursorPosition(hoofd.xpos, hoofd.ypos);
                Console.ForegroundColor = hoofd.schermkleur;
                Console.Write("■");
                Console.SetCursorPosition(berryx, berryy);
                Console.ForegroundColor = ConsoleColor.Cyan;
                Console.Write("■");
                tijd = DateTime.Now;
                buttonpressed = "no";
                while (true)
                {
                    tijd2 = DateTime.Now;
                    if (tijd2.Subtract(tijd).TotalMilliseconds > 500) { break; }
                    if (Console.KeyAvailable)
                    {
                        ConsoleKeyInfo toets = Console.ReadKey(true);
                        //Console.WriteLine(toets.Key.ToString());
                        if (toets.Key.Equals(ConsoleKey.UpArrow) && movement != "DOWN" && buttonpressed == "no")
                        {
                            movement = "UP";
                            buttonpressed = "yes";
                        }
                        if (toets.Key.Equals(ConsoleKey.DownArrow) && movement != "UP" && buttonpressed == "no")
                        {
                            movement = "DOWN";
                            buttonpressed = "yes";
                        }
                        if (toets.Key.Equals(ConsoleKey.LeftArrow) && movement != "RIGHT" && buttonpressed == "no")
                        {
                            movement = "LEFT";
                            buttonpressed = "yes";
                        }
                        if (toets.Key.Equals(ConsoleKey.RightArrow) && movement != "LEFT" && buttonpressed == "no")
                        {
                            movement = "RIGHT";
                            buttonpressed = "yes";
                        }
                    }
                }
                xposlijf.Add(hoofd.xpos);
                yposlijf.Add(hoofd.ypos);
                switch (movement)
                {
                    case "UP":
                        hoofd.ypos--;
                        break;
                    case "DOWN":
                        hoofd.ypos++;
                        break;
                    case "LEFT":
                        hoofd.xpos--;
                        break;
                    case "RIGHT":
                        hoofd.xpos++;
                        break;
                }
                if (xposlijf.Count() > score)
                {
                    xposlijf.RemoveAt(0);
                    yposlijf.RemoveAt(0);
                }
            }
            Console.SetCursorPosition(screenwidth / 5, screenheight / 2);
            Console.WriteLine("Game over, Score: "+ score);
            Console.SetCursorPosition(screenwidth / 5, screenheight / 2 +1);
        }
        class pixel
        {
            public int xpos { get; set; }
            public int ypos { get; set; }
            public ConsoleColor schermkleur { get; set; }
        }
    }
}
//¦
\$\endgroup\$
2
  • 1
    \$\begingroup\$ The code seems amazingly similar to that which I am about to link to. Study the code and the answers there for some good tips: codereview.stackexchange.com/questions/45011/small-c-pong-game/… \$\endgroup\$ Commented May 4, 2016 at 19:50
  • \$\begingroup\$ A good place to look is a book called "Clean Code', or just follow the author's blog (blog.cleancoder.com). If there is not a good answer by tonight, I'll put some time into giving you a good example. \$\endgroup\$
    – Jeff.Clark
    Commented May 4, 2016 at 21:30

5 Answers 5

8
\$\begingroup\$

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<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";

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 accidentally put "NODIRECTION" as a viable direction.

Refactoring

I noticed you use this exact pattern of code a lot :

SetCursorPosition (head.XPos, head.YPos);
ForegroundColor = head.ScreenColor;
Write ("■");

When you notice that you're repeating a piece of code more than twice, you know you should create a method. Let's call it DrawPixel(Pixel pixel) . It takes a pixel as input and draws it.

static void DrawPixel (Pixel pixel)
{
    SetCursorPosition (pixel.XPos, pixel.YPos);
    ForegroundColor = pixel.ScreenColor;
    Write ("■");
}

Now you can alter the declaration of the berry from:

var xPosBerry = rand.Next (0, WindowWidth);
var yPosBerry = rand.Next (0, WindowHeight);

To (after correcting the numbers to avoid the berry ending up in the walls):

var berry = new Pixel (rand.Next (1, WindowWidth - 2), rand.Next (1, WindowHeight - 2), ConsoleColor.Cyan);

This allows me to change a later part of the code to

if (berry.XPos == head.XPos && berry.YPos == head.YPos)
{
    score++;
    berry = new Pixel (rand.Next (1, WindowWidth - 2), rand.Next (1, WindowHeight - 2), ConsoleColor.Cyan);
}

and to draw the berry later all you have to do is :

DrawPixel (berry);

I tested the code at this point and it seems to be still working fine with none of the functionality affected.

Creating the body

After what we did so far, you can define the body as List<Pixel>. You can define it as:

 var body = new List<Pixel> ();

Draw it like:

for (int i = 0; i < body.Count (); i++)
{
    DrawPixel (body[i]);

    if (body[i].XPos == head.XPos && body[i].YPos == head.YPos)
    {
        gameover = true;
    }
}

One cool thing I found here (thanks to some refactoring tools I have installed), is that you can replace the if statement with an assignment statement (using |=) that takes less vertical space. It adds up to the same thing, due to how the OR operator works.

for (int i = 0; i < body.Count (); i++)
{
    DrawPixel (body[i]);
    gameover |= (body[i].XPos == head.XPos && body[i].YPos == head.YPos);
}

Back to the body, you can add a Pixel to it like this:

body.Add (new Pixel (head.XPos, head.YPos, ConsoleColor.Green));

Your way of making sure the body stays within the length, while growing with more food, is ... odd .. tbh. It took me a while to wrap my head around it. You're using the score variable for control. I felt it was clever enough so I left it as is.

Small Things

I am sure there are more possible improvements, but this is what I had the time to do currently.

  • added a little Console.ReadKey(); in the end so the Console app does not quit immediately.
  • rephrased the inner while loop with a condition that is more immediately readable.
  • put the content of the inner while loop into a separate method for readability.
  • removed buttonPressed as it is not needed, thanks to else if.
  • removed usage of System.Linq. The list type has its own running tally without calling a method.
  • changed some variables to more descriptive names
  • corrected your score display
  • removed annoying cursor next to the berry
  • used Stopwatch instead of crude DateTime.Now

Final code

without further ado:

using System;
using System.Collections.Generic;
using System.Diagnostics;
using static System.Console;

namespace Snake
{
    class Program
    {
        static void Main ()
        {
            WindowHeight = 16;
            WindowWidth = 32;

            var rand = new Random ();

            var score = 5;

            var head = new Pixel (WindowWidth / 2, WindowHeight / 2, ConsoleColor.Red);
            var berry = new Pixel (rand.Next (1, WindowWidth - 2), rand.Next (1, WindowHeight - 2),     ConsoleColor.Cyan);

            var body = new List<Pixel> ();

            var currentMovement = Direction.Right;

            var gameover = false;

            while (true)
            {
                Clear ();

                gameover |= (head.XPos == WindowWidth - 1 || head.XPos == 0 || head.YPos ==     WindowHeight - 1 || head.YPos == 0);

                DrawBorder ();

                if (berry.XPos == head.XPos && berry.YPos == head.YPos)
                {
                    score++;
                    berry = new Pixel (rand.Next (1, WindowWidth - 2), rand.Next (1, WindowHeight -     2), ConsoleColor.Cyan);
                }

                for (int i = 0; i < body.Count; i++)
                {
                    DrawPixel (body[i]);
                    gameover |= (body[i].XPos == head.XPos && body[i].YPos == head.YPos);
                }

                if (gameover)
                {
                    break;
                }

                DrawPixel (head);
                DrawPixel (berry);

                var sw = Stopwatch.StartNew();
                while (sw.ElapsedMilliseconds <= 500)
                {
                    currentMovement = ReadMovement (currentMovement);
                }

                body.Add (new Pixel (head.XPos, head.YPos, ConsoleColor.Green));

                switch (currentMovement)
                {
                    case Direction.Up:
                        head.YPos--;
                        break;
                    case Direction.Down:
                        head.YPos++;
                        break;
                    case Direction.Left:
                        head.XPos--;
                        break;
                    case Direction.Right:
                        head.XPos++;
                        break;
                }

                if (body.Count > score)
                {
                    body.RemoveAt (0);
                }
            }
            SetCursorPosition (WindowWidth / 5, WindowHeight / 2);
            WriteLine ($"Game over, Score: {score - 5}");
            SetCursorPosition (WindowWidth / 5, WindowHeight / 2 + 1);
            ReadKey ();
        }

        static Direction ReadMovement (Direction movement)
        {
            if (KeyAvailable)
            {
                var key = ReadKey (true).Key;

                if (key == ConsoleKey.UpArrow && movement != Direction.Down)
                {
                    movement = Direction.Up;
                }
                else if (key == ConsoleKey.DownArrow && movement != Direction.Up)
                {
                    movement = Direction.Down;
                }
                else if (key == ConsoleKey.LeftArrow && movement != Direction.Right)
                {
                    movement = Direction.Left;
                }
                else if (key == ConsoleKey.RightArrow && movement != Direction.Left)
                {
                    movement = Direction.Right;
                }
            }

            return movement;
        }

        static void DrawPixel (Pixel pixel)
        {
            SetCursorPosition (pixel.XPos, pixel.YPos);
            ForegroundColor = pixel.ScreenColor;
            Write ("■");
            SetCursorPosition (0, 0);
        }

        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 ("■");
            }
        }

        struct 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; }
        }

        enum Direction
        {
            Up,
            Down,
            Right,
            Left
        }
    }
}

Please let me know if there is anything unclear in the comments. My version is far from perfect and I am sure I made some pitfalls myself.

\$\endgroup\$
6
\$\begingroup\$

First up:

Good job on this game. Your logic is good. Yes, there are some improvements in the code and so on, and all of the above comments are spot on, so I'm not going to hammer on the same points.

Simple Enhancements:

When I ran the game I found it a bit laggy. The constant Console.Clear(); operations made the screen flicker quite a bit. Also, the drawing of the border was a bit long winded. For example, you are on a row at 0, 0... You can write the entire horizontal bar in one go with one Console.Write("■■■■■■■■■■■■■■"); statement. The same applies for the bottom row of the border.

On that note: The border doesn't change. Right? So we don't have to keep on drawing the border. We can draw it at the beginning and "pseudo-clear" the console. What I mean by that: the console is already black, so instead of saying Console.Clear(); we can just create a method named ClearConsole(); that writes black over the inside of the frame.

I've made those minor changes, and the game runs a lot smoother, without the flickering on the screen. Here is the modified code, with your variable names and all of that intact:

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<int> xposlijf = new List<int>();
            List<int> yposlijf = new List<int>();
            int berryx = randomnummer.Next(0, screenwidth);
            int berryy = randomnummer.Next(0, screenheight);
            DateTime tijd = DateTime.Now;
            DateTime tijd2 = DateTime.Now;
            string buttonpressed = "no";

            // We only draw the border once. It doesn't change.
            DrawBorder(screenwidth, screenheight);

            while (true)
            {
                ClearConsole(screenwidth, screenheight);
                if (hoofd.xpos == screenwidth - 1 || hoofd.xpos == 0 || hoofd.ypos == screenheight - 1 || hoofd.ypos == 0)
                {
                    gameover = 1;
                }

                Console.ForegroundColor = ConsoleColor.Green;
                if (berryx == hoofd.xpos && berryy == hoofd.ypos)
                {
                    score++;
                    berryx = randomnummer.Next(1, screenwidth - 2);
                    berryy = randomnummer.Next(1, screenheight - 2);
                }
                for (int i = 0; i < xposlijf.Count(); i++)
                {
                    Console.SetCursorPosition(xposlijf[i], yposlijf[i]);
                    Console.Write("¦");
                    if (xposlijf[i] == hoofd.xpos && yposlijf[i] == hoofd.ypos)
                    {
                        gameover = 1;
                    }
                }
                if (gameover == 1)
                {
                    break;
                }
                Console.SetCursorPosition(hoofd.xpos, hoofd.ypos);
                Console.ForegroundColor = hoofd.schermkleur;
                Console.Write("■");
                Console.SetCursorPosition(berryx, berryy);
                Console.ForegroundColor = ConsoleColor.Cyan;
                Console.Write("■");
                Console.CursorVisible = false;
                tijd = DateTime.Now;
                buttonpressed = "no";
                while (true)
                {
                    tijd2 = DateTime.Now;
                    if (tijd2.Subtract(tijd).TotalMilliseconds > 500) { break; }
                    if (Console.KeyAvailable)
                    {
                        ConsoleKeyInfo toets = Console.ReadKey(true);
                        //Console.WriteLine(toets.Key.ToString());
                        if (toets.Key.Equals(ConsoleKey.UpArrow) && movement != "DOWN" && buttonpressed == "no")
                        {
                            movement = "UP";
                            buttonpressed = "yes";
                        }
                        if (toets.Key.Equals(ConsoleKey.DownArrow) && movement != "UP" && buttonpressed == "no")
                        {
                            movement = "DOWN";
                            buttonpressed = "yes";
                        }
                        if (toets.Key.Equals(ConsoleKey.LeftArrow) && movement != "RIGHT" && buttonpressed == "no")
                        {
                            movement = "LEFT";
                            buttonpressed = "yes";
                        }
                        if (toets.Key.Equals(ConsoleKey.RightArrow) && movement != "LEFT" && buttonpressed == "no")
                        {
                            movement = "RIGHT";
                            buttonpressed = "yes";
                        }
                    }
                }
                xposlijf.Add(hoofd.xpos);
                yposlijf.Add(hoofd.ypos);
                switch (movement)
                {
                    case "UP":
                        hoofd.ypos--;
                        break;
                    case "DOWN":
                        hoofd.ypos++;
                        break;
                    case "LEFT":
                        hoofd.xpos--;
                        break;
                    case "RIGHT":
                        hoofd.xpos++;
                        break;
                }
                if (xposlijf.Count() > score)
                {
                    xposlijf.RemoveAt(0);
                    yposlijf.RemoveAt(0);
                }
            }
            Console.SetCursorPosition(screenwidth / 5, screenheight / 2);
            Console.WriteLine("Game over, Score: " + score);
            Console.SetCursorPosition(screenwidth / 5, screenheight / 2 + 1);
        }

        private static void ClearConsole(int screenwidth, int screenheight)
        {
            var blackLine = string.Join("", new byte[screenwidth - 2].Select(b => " ").ToArray());
            Console.ForegroundColor = ConsoleColor.Black;
            for (int i = 1; i < screenheight - 1; i++)
            {
                Console.SetCursorPosition(1, i);
                Console.Write(blackLine);
            }
        }

        private static void DrawBorder(int screenwidth, int screenheight)
        {
            var horizontalBar = string.Join("", new byte[screenwidth].Select(b => "■").ToArray());

            Console.SetCursorPosition(0, 0);
            Console.Write(horizontalBar);
            Console.SetCursorPosition(0, screenheight - 1);
            Console.Write(horizontalBar);

            for (int i = 0; i < screenheight; i++)
            {
                Console.SetCursorPosition(0, i);
                Console.Write("■");
                Console.SetCursorPosition(screenwidth - 1, i);
                Console.Write("■");
            }
        }

        class pixel
        {
            public int xpos { get; set; }
            public int ypos { get; set; }
            public ConsoleColor schermkleur { get; set; }
        }
    }
}
\$\endgroup\$
4
\$\begingroup\$

Okay,

I'd move your while loops to their own functions, for two reasons.

  1. It's easier to read, if you know exactly what that function doing, because it is holding a while loop. Even though your while loops are infinite (more on that later)
  2. The loops are long, and generally it looks like the loops can be self contained. When code looks like it is its own little 'island' as it were, it can move to a self contained function.

Naming is key, variables with the names like "hoofd" is kinda hard to read, generally you want your variables to be short but descriptive. I'd prefer pixelPlaceHolder or pixelCursor over something short but harder to read. I know that is a preference. But I've heard fewer people complain about a long var name, then a hard to read name.

Also using a bool instead of yes or no would make your program easier to read. You don't have to use a string equator. So for buttonpressed, I would rename it to bool buttonPressed. So all you have to do is make buttonPressed, true or false. example:

Change this code:

if (toets.Key.Equals(ConsoleKey.UpArrow) 
    && movement != "DOWN" 
    && buttonpressed == "no"
){
    movement = "UP";
    buttonpressed = "yes";
}

To this:

 if (toets.Key.Equals(ConsoleKey.UpArrow) 
     && movement != "DOWN" 
     && !buttonPressed
 ){
    //do something
 }

Also another thing to help you out, for literal like "UP" or 1 place them in const variables. like const string UP = "UP" Reason being, if anything were to change with your program in the future, and "UP" becomes "up" then you only have to change it in one spot, and not the whole program. Also 1 could mean anything. Instead, give it a name or a enum value. Like const int GAME_OVER = 1 or enum Condition { Started = 0, GameOver = 1}; This way someone sees the condition and not a number. You can call it like this while(GameState == Condition.Started) or while(GameState != GAME_OVER) Especially if you are using numbers in more then one spot. It cuts down on confusion for yourself and others. Trust me! Leave your code for a couple of weeks and come back to it... Let me know if you don't go, "what was I doing here?" :)

Infinite loops:

Debugging: to find an issue, you decide to use the debugger with a while loop == true, is fine. But generally you want to have a terminating condition in the loop. Example: gameover == 1 can easily be placed in your main 'while' loop to kill the program and exit. Or one step further, have the program return to menu and wait for the user to either: start a new game, or quit. I know some people will argue over it, but IMHO it is bad practice to use a break outside of a switch statement. Reason being, it is harder to debug a loop that has a non-natural break, IE a break inside of a loop, because it takes longer to figure out when that break is triggered. Especially in bigger programs it's hard to figure out when it will hit that statement.

Additional Note:

Another bigger issue, that is some times hard to debug, if you accidentally place a break outside a loop. The break statement will then "break" out of the function. This may cause the program to crash, or worse, the program will keep running like nothing happened. The program seems like it is operating normal, but in fact, it could be skipping a potentially important step that would cause the program to crash. Crashing is not a bad thing, because it is the most obvious way to detect an issue with your code. In fact, when something is wrong and you don't get feed back, that is when you pull out your hair.

Overall:

Your code is not bad, especially for a beginner. I know, when I started out, I was hesitant in putting code into classes or functions, if I could get away putting it all in one function like main. Once you get experience you will find, that functions make things easier to read and also program.

Take for instance your while loops. If they were in their own functions, you could pass in the condition to stop at, and pass back the result, and it will be a lot easier to read and handle the program flow. Try, making main, the Head Quarters of your program and the functions the actual controls for your program. So all main does, is kick off the action and the functions control the actual movement of the snake.

Like have the first while loop, in a function called Draw(). It takes in the changes from the switch statement, and redraws the snake based on the input. Then have another function for the other while loop. Call that function UserInput(). If you want Co locate the switch statement with that loop, or have it in its own function. When you press a button while it's in the loop it can call the switch statement to make the adjustment, and then redraw the environment by calling the draw function. You may find by doing this "refactoring" that you can rid yourself of a while loop or loops, which will reduce your Big O notation which can make your program faster. Obviously, since your program is so small this reduction in loops will not be that noticeable. If you get in the practice now of doing it, when you move to something bigger, it will make a big difference.

As one of the commentators above have mentioned, Clean Code is a really good book for learning how to refactor, and making code more efficient. The plus side of making clean code, is you can use it in more then one program. The more 'Object' you make your code the easier it is to use the code in other programs.

I hope this helps you out, or at least, gives you ideas of how to improve your program. Either way, keep up the good work. Just like anything, everyone starts somewhere and with practice you will be as good or better as many of the people you see giving examples on this site or Stack Overflow. In other words, keep coding my friend.

\$\endgroup\$
1
  • \$\begingroup\$ @Heslacher Thanks for the edit, I'm horrible with making these things look clean. Still working on my Markdown skills. \$\endgroup\$ Commented May 5, 2016 at 11:28
1
\$\begingroup\$

Split your Code

Your Main is tooo long and it is horrible to Read. Split your Code into multiple functions so other people can Read, and it becomes easier to unterstand with functions.

Naming

Try to give your variable meaningful Names so it becomes Clear to other People

You could use Enum for the Directions in your switch Statement.

Declare your Lists and other stuff globaly not in the middle of your Code.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ This is a very minimalistic answer. Care to expand on that? \$\endgroup\$
    – Mast
    Commented May 4, 2016 at 19:32
  • \$\begingroup\$ Yeah youre right a comment w ould have been better im sorry im nrw here \$\endgroup\$
    – iNCEPTiON_
    Commented May 4, 2016 at 19:36
  • 1
    \$\begingroup\$ Well, you got the basics down. If you finish what you started this could turn into an answer. \$\endgroup\$
    – Mast
    Commented May 4, 2016 at 19:37
  • 4
    \$\begingroup\$ You could make this a better answer by providing suggestions about what functions you would define. \$\endgroup\$ Commented May 4, 2016 at 19:45
-3
\$\begingroup\$

A Thread.Sleep(5) is needed, otherwise loop will go horribly fast and it will eat lots of processorpower.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ Are you sure about that? \$\endgroup\$
    – Mast
    Commented Aug 31, 2018 at 7:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.