12
\$\begingroup\$

I've made a small "rover game" similar to this one, except made in C#, using OOP, and prettier menus. Here's how it works:

There is one Rover class which is instantiated into an array containing all rovers. This class contains certain actions that a rover might need like, MoveRover, or RoverDead. There is also a RoverManager class where actions such as updating rovers are executed by looping through the array where the rover data is stored. This is also where rover addition and deletion is managed as well.

using System;
using System.Collections.Generic;

namespace Rovers
{
    /* A class for creating a rover */
    class Rover
    {
        public long x;
        public long y;
        public string name;
        public int lifeTime;

        public Rover(long x, long y, string name, int lifeTime)
        {
            this.x = x;
            this.y = y;
            this.name = name;
            this.lifeTime = lifeTime;
        }

        /* Update the rover lifetime */
        public void UpdateRoverLifeTime(int xChange, int yChange)
        {
            this.lifeTime -= Math.Abs(xChange - yChange);
        }

        /* Move the rover a desired amount */
        public void MoveRover(int xChange, int yChange)
        {
            if (xChange <= 5 && yChange <= 5 && xChange >= -5 && yChange >= -5)
            {
                this.x += xChange;
                this.y += yChange;
                UpdateRoverLifeTime(xChange, yChange);
            }
            else
            {
                Console.WriteLine("\nPOSITION CHANGE MUST BE <= 5 or >= -5.");
            }
        }

        /* Get input for position changes of the rover */
        public void GetRoverPositionChange()
        {
            Console.WriteLine("\n-- Rover Position Changer Menu. --");

            Console.Write("xChange: ");
            int xChange = Int32.Parse(Console.ReadLine());

            Console.Write("yChange: ");
            int yChange = Int32.Parse(Console.ReadLine());

            MoveRover(xChange, yChange);
        }

        /* Output rover statistics */
        public void OutputRoverStats()
        {
            Console.WriteLine("\n-- Rover Stats Menu. --");
            Console.WriteLine("Rover: {0}", this.name);
            Console.WriteLine("x: {0}, y: {1}", this.x, this.y);
            Console.WriteLine("Life: {0}", this.lifeTime);
        }

        /* Is the rover dead? :( https://xkcd.com/695/ */
        public bool RoverDead()
        {
            return this.lifeTime > 0;
        }

        /* Main rover update "loop" */
        public void UpdateRover()
        {
            GetRoverPositionChange();
            OutputRoverStats();
        }
    }


    /* A class containing certain methods for rover managment */
    class RoverManager
    {
        public List<Rover> roverList = new List<Rover>();

        /* Add a rover */
        public void AddRover()
        {
            Console.WriteLine("\n-- Add Rover Menu. --");

            Console.Write("startX: ");
            long startX = (long)Int32.Parse(Console.ReadLine());

            Console.Write("startY: ");
            long startY = (long)Int32.Parse(Console.ReadLine());

            Console.Write("name: ");
            string name = Console.ReadLine();

            Console.Write("startingLifeTime: ");
            int startingLifeTime = Int32.Parse(Console.ReadLine());

            this.roverList.Add(new Rover(startX, startY, name, startingLifeTime));
        }

        /* Destroy a rover if it's dead */
        public void DestroyRovers()
        {
            for (int n = roverList.Count-1; n >= 0; n--)
            {
                Rover rover = roverList[n];

                if (rover.RoverDead())
                {
                    roverList.Remove(rover);
                }
            }
        }

        /* Update each rover */
        public void UpdateRovers()
        {
            for (int n = roverList.Count-1; n >= 0; n--)
            {
                Rover rover = roverList[n];
                rover.UpdateRover();
            }
        }

        /* Main rover manager loop */
        public void MainManagerLoop()
        {
            while (true)
            {
                AddRover();
                UpdateRovers();
                DestroyRovers();
            }
        }
    }


    public class MainProgram
    {
        private static RoverManager roverManager = new RoverManager();

        public static void Main(string[] args)
        {
            roverManager.MainManagerLoop();
        }
    }
}
\$\endgroup\$

4 Answers 4

14
\$\begingroup\$
public List<Rover> roverList = new List<Rover>();

This field can (and should) be made private. We don't want strangers messing around with our rovers!

While we're at it, let's change the name from roverList to rovers -- we don't need the name to reflect the fact that it's a list of rovers.

Finally, we can make the field readonly.


public void DestroyRovers()
{
    for (int n = roverList.Count-1; n >= 0; n--)
    {
        Rover rover = roverList[n];

        if (rover.RoverDead())
        {
            roverList.Remove(rover);
        }
    }
}

Here we're removing all the dead rovers from the list. There's a method RemoveAll that can do this for us.

public void DestroyRovers()
{
    this.rovers.RemoveAll(rover => rover.RoverDead());
}

rover.RoverDead() looks strange to me; I think IsDead would be a better name. IsDead is a good candidate for a property since we're just returning the result of a quick comparison, so let's change this

public bool RoverDead()
{
    return this.lifeTime > 0;
}

to this

public bool IsDead
{
    get { return this.lifeTime > 0; }
}

Now our method looks better:

public void DestroyRovers()
{
    this.rovers.RemoveAll(rover => rover.IsDead);
}

public void UpdateRovers()
{
    for (int n = roverList.Count-1; n >= 0; n--)
    {
        Rover rover = roverList[n];
        rover.UpdateRover();
    }
}

We can use Enumerable.Reverse to do the reversing for us. It's not much of a change, but I think it makes it a little easier for the reader to see exactly what is happening:

public void UpdateRovers()
{
    foreach (var rover in Enumerable.Reverse(this.rovers))
    {
        rover.Update();
    }
}
\$\endgroup\$
8
  • 2
    \$\begingroup\$ The reverse iteration in UpdateRovers looks like a copy-paste from DestroyRovers. The reverse in DestroyRovers is because the collection is being modified while it's being iterated. If it actually requires reversed iteration it should, in my opinion, be delegated to the storage. \$\endgroup\$
    – Johnbot
    Commented Apr 10, 2015 at 9:28
  • \$\begingroup\$ I like that you mention the LINQ ideas. I just don't think they are beginner material. Yes it is easier to read, yes it is easier to write, yes it is more powerful. But does the OP understand extension methods, or Func? We don't know the answer to that, but we can assume not since it was not used. Just my two cents. Good coverage though! \$\endgroup\$ Commented Apr 10, 2015 at 12:32
  • \$\begingroup\$ I don't like the change roverList to rovers idea. Just adding the 's' doesn't make it as obvious that it is a collection of rovers as roverList does. If you want to change roverList to roverCollection or similar then fine, but using simply 'rovers' makes the code less readable. You are also making the assumption that using the word 'List' means that a 'List' class is actually used, when that isn't necessarily true. My definition of readable is that you don't have to 'pause' and think about what a particular name means while reading the code. \$\endgroup\$
    – Dunk
    Commented Apr 10, 2015 at 13:17
  • 3
    \$\begingroup\$ @Dunk I prefer to use a plural term rather than describe the enumeration type. I'm not certain why you feel that using a plural term to describe a typed List/Collection/Enumerable would require a 'pause' to think about whether it would contain just one or potentially many. \$\endgroup\$
    – psaxton
    Commented Apr 10, 2015 at 17:50
  • 2
    \$\begingroup\$ @RobertSnyder I don't see a need to limit CodeReview responses to 'beginner material'. The OP does indicate that they are new to the language and asks to have advanced topics explained, not avoided. \$\endgroup\$
    – psaxton
    Commented Apr 10, 2015 at 17:53
13
\$\begingroup\$

The answer of mjolka is already showing several good points. Here are some additional remarks:

Fields => Properties

You're not using the fields outside the class, so why make them public? You have two options, make the fields private or create a property with a private setter. I'd prefer the second choice:

public long X { get; private set; }
public long Y { get; private set; }
public string Name { get; private set; }
public int LifeTime { get; private set; }

Now you can still use them the same way like before, and only set the values from within the class. Reading the values is also possible from outside the class.

Input validation:

One of the first things that struck me is that you never validate user input. Your program will throw a System.FormatException when entering non-numeric input. You'll have to provide some way to catch this.

Change following line:

int xChange = Int32.Parse(Console.ReadLine());

to:

int xChange;
var xValid = Int32.TryParse(Console.ReadLine(), out xChange);

This also goes for the y coordinate. Now you can nicely validate against these boolean fields and ask for coordinates again if the input is not valid:

if (xValid && yValid)
{
    MoveRover(xChange, yChange);
}
else
{
    Console.WriteLine("Unknown rover input, please try again:");
    GetRoverPositionChange();
}

You must also apply this kind of validation elsewhere in your code where you ask for input.

Logical errors:

The variable name lifeTime confused me. You checked for:

public bool RoverDead()
{
    return this.lifeTime > 0;
}

According to the above method, the rover is dead from the moment the lifeTime variable is greater than 0. But when you're moving the rover, you're subtracting the change from the lifetime, making him more and more alive. But you can't make him more alive since he will be dead from the beginning when you give the rover a lifetime of any positive number.

This doesn't make sense to me. Rename the lifeTime variable to RemainingLifeTime and use the property suggested by mjolka:

public bool IsAlive
{
    get { return RemainingLifeTime > 0; }
}

Now when the remaining lifetime is greater than 0, your rover can happily move and will die when you keep subtracting the changes from its remaining lifetime. This gets me to another error in the logic. This is the formula you use to calculate the number to subtract from the lifetime:

public void UpdateRoverLifeTime(int xChange, int yChange)
{
    RemainingLifeTime -= Math.Abs(xChange - yChange);
}

If this can be applied in real life, the Mars Rover could keep going indefinitely, as long as the change of X is equal to the change of Y. Example:

  • Start X: 0
  • Start Y: 0
  • Name: test
  • Starting LifeTime: 2

Now give 5 as input for xChange and yChange and voila... infinite power for the fella!

Rethink a better way to calculate this, you could for example use a Pythagorean function:

a² = b² + c²

Your formula would become:

var deltaChange = (int)Math.Floor(Math.Sqrt(xChange * xChange + yChange * yChange));

You will have to adjust the maximum and minimum allowed change though, as for now you'll only allow very small moves.

Hope this helps!

\$\endgroup\$
2
  • \$\begingroup\$ Quick question, why are you using var to declare deltaChange when you could just use int? \$\endgroup\$ Commented Apr 10, 2015 at 19:28
  • 1
    \$\begingroup\$ @Ethan Bierlein because the type is obvious from the right hand side of the assignment. \$\endgroup\$
    – Abbas
    Commented Apr 10, 2015 at 21:30
7
\$\begingroup\$

(This is in addition to mjolka's review.)


What are these, fields of properties? If they're fields they should be private; if they're properties they should have a getter/setter.

public long x;
public long y;
public string name;
public int lifeTime;

Putting a lot of trust in your user here:

Int32.Parse(Console.ReadLine());

What happens if he/she doesn't enter an int?


Same issue as before: is this a field or a property?

public List<Rover> roverList = new List<Rover>();

Why parse the input to an int (also: again you're putting a lot of trust in your user) and then cast it to a long?

long startX = (long)Int32.Parse(Console.ReadLine());

I know, your Rover expects a long. But why is that necessary?

\$\endgroup\$
2
  • 2
    \$\begingroup\$ Hehe... A field of properties. ++ I like a good pun. \$\endgroup\$
    – RubberDuck
    Commented Apr 10, 2015 at 11:46
  • 2
    \$\begingroup\$ @RubberDuck D'oh, that's a typo. My finger must have slipped from the r to the f key below. Well, now I'm leaving it in. ;-) \$\endgroup\$
    – BCdotWEB
    Commented Apr 10, 2015 at 11:48
5
\$\begingroup\$

I'm going to go out on a limb and assume you come from a java world based on your comments over your methods, and the use of your fields instead of properties. Neither of these things are bad, just not what we are used to seeing in the C# world. If you are using Visual Studio you can type a triple forwards slash and it will make a xml style comment that intellisense can and will use. example use would be

    /// <summary>
    /// Add a rover
    /// </summary>
    public void AddRover()

    /// <summary>
    /// Is the rover dead?
    /// </summary>
    /// <returns>returns true if the rover is dead</returns>
    /// <remarks>https://xkcd.com/695/</remarks>
    public bool RoverDead()

    /// <summary>
    /// Move the rover a desired amount
    /// </summary>
    /// <param name="xChange">the amount to move the rover on the x coordinate</param>
    /// <param name="yChange">the amount to move the rover on the y coordinate</param>
    public void MoveRover(int xChange, int yChange)

Another helpful thing to learn about is the power of some of the testing frameworks at your disposal. As it sits right now NUnit is probably the most popular. MSTest isn't horrible. I've never used xUnit so I can't speak for it. whichever you choose is up to you. In terms of speed and easiest to setup MSTest will be the way to go. I also recommend using Fluent assertions (search for it using Nuget) with it, but that is more of a personal preference then anything else.

Now I'm not going to get into the debate of what style of testing to use be it test-after (write a test after you've written your production code) TDD, BDD or a combination there of. I will say though that if you write your test first and try to make your tests pass your code will be more testable. Just like writing production code has a certain art to it, so does writing good tests. It all takes practice.

With that said I will focus now on a comment made in a post up above by Abbas

If this can be applied in real life, the Mars Rover could keep going indefinitely, as long as the change of X is equal to the change of Y. Example:

Start X: 0
Start Y: 0
Name: test
Starting LifeTime: 2

Now give 5 as input for xChange and yChange and voila... infinite power for the fella!

Instead of typing in those values that he speaks of a the console window you can write a test to assume certain things.

[TestClass]
public class RoverTests
{
    [TestMethod]
    public void AfterMovingRoverLifeDecreases()
    {
        var rover = new Rover(0, 0, "test", 6);
        rover.MoveRover(2, 4);
        Assert.AreEqual(0, rover.lifeTime, "rover moved 6, so life should be 0");
    }

    [TestMethod]
    public void AbbasFoundThisBug()
    {
        var rover = new Rover(0, 0, "test", 2);
        rover.MoveRover(5, 5);
        Assert.AreEqual(-8, rover.lifeTime, "rover moved 10, so life should be -8");
    }
}

those tests fails currently (I did that on purpose). It took 17ms to run on my PC and said the following.

Test Name:  AfterMovingRoverLifeDecreases
Test Outcome:   Failed
Test Duration:  0:00:00.017907
Result Message: Assert.AreEqual failed. Expected:<0>. Actual:<4>. rover moved 6, so life should be 0

Test Name:  AbbasFoundThisBug
Test Outcome:   Failed
Test Duration:  0:00:00.0006728
Result Message: Assert.AreEqual failed. Expected:<-8>. Actual:<2>. rover moved 10, so life should be -8

(I guessed at the assertions for how much life should be remaining and how to calculate life)

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Nice elaboration on the unit testing. May I only point out that my name is Abbas and not Abbus. :D \$\endgroup\$
    – Abbas
    Commented Apr 10, 2015 at 19:13
  • 1
    \$\begingroup\$ @Abbas nice :) thank you for finding that. It is corrected \$\endgroup\$ Commented Apr 10, 2015 at 19:44

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.