5
\$\begingroup\$

I'm looking at my code and it doesn't look very efficient. I'm simply trying to catch exceptions or prevent them from being thrown in the first place. Can you please take a look and tell me if there is a way to make my working code more efficient?

public static void GetMoveCoordinates(ref int XCoordinate, ref int YCoordinate)
    {
        bool invalidCoordinate = false;

        do
        {
            try
            {

                Console.Write("Enter x coordinate: ");
                XCoordinate = int.Parse(Console.ReadLine());
                if (XCoordinate < 1 || XCoordinate > 3)
                {
                    invalidCoordinate = true;
                    Console.WriteLine("Invalid coordiantes, please try again.");
                }
                else
                    invalidCoordinate = false;

                if (!invalidCoordinate)
                {
                    Console.Write("Enter y coordinate: ");
                    YCoordinate = int.Parse(Console.ReadLine());
                    if (YCoordinate < 1 || YCoordinate > 3)
                    {
                        invalidCoordinate = true;
                        Console.WriteLine("Invalid coordiantes, please try again.");
                    }
                    else
                        invalidCoordinate = false;
                }
            }

            catch
            {
                Console.WriteLine("Invalid coordiantes, please try again.");
                invalidCoordinate = true;
            }
                Console.WriteLine();



        } while (invalidCoordinate);


    }  // end of GetMoveCoordinates
\$\endgroup\$

3 Answers 3

6
\$\begingroup\$

Alright, first some styling remarks.

Indent correctly

Opening braces of your method should be at the same level below of the method signature, not indented. You seem to have done it correctly elsewhere so that might be a copying error.

Follow naming conventions

Parameters and local variables are lowerCamelCase. This means XCoordinate becomes xCoordinate and likewise for YCoordinate.

Positive beats negative beats battlestar galactica

Always prefer using "positive" expressions rather than "negative". If I'd want to use your code to say the coordinate is valid, I'd have to use if(!invalidCoordinate). Nobody likes double negation.

I can see why you might have done this (invalid input sets invalidCoordinate to true) but in that case I would just use a break; statement to exit your loop.

Change it to validCoordinate and your loop becomes while(!validCoordinate).

Braces around if statements

Some will argue this but to avoid misconceptions or problems with future changes: always place {} brackets around statements that can have it to very clearly mark their scope. Your else statements don't have this which makes it prone to logical errors (forgetting to add the braces when the else block actually needs more than just that statement).

Don't overassign

Your while loop already stops when invalidCoordinate is true. There is no point in explicitly setting it to false.

No typos

Yuck.

Don't repeat yourself

It seems to me that both x and y have to be between 1 and 3? Let's put that in a method instead of copy-pasting it and changing where needed.

Useful comments

I know that is the end of GetMoveCoordinates because your favorite awesome best-in-the-galaxy IDE - Visual Studio - will show you that. Place your cursor behind the closing brace and it will popup the method definition at the top of your editor window.

As has been noted in the comments: make sure that your comments explain why you do something. It is not clear why things have to be between 1 and 3 exactly so this is a good place to start.


After applying all this, we have this situation:

public static void GetMoveCoordinates(ref int xCoordinate, ref int yCoordinate)
{
   bool validCoordinate = true;

   do
   {
       try
       {
            Console.Write("Enter x coordinate: ");
            GetCoordinate(ref validCoordinate, ref xCoordinate);

            if (validCoordinate)
            {
                Console.Write("Enter y coordinate: ");
                GetCoordinate(ref validCoordinate, ref yCoordinate);
            }
       }

       catch
       {
           Console.WriteLine("Invalid coordinates, please try again.");
           break;
       }
       Console.WriteLine();
   } while (!validCoordinate);
} 

private static void GetCoordinate(ref bool validCoordinate, ref int coordinate)
{
    coordinate = int.Parse(Console.ReadLine());
    if (coordinate < 1 || coordinate > 3)
    {
        validCoordinate = false;
        Console.WriteLine("Invalid coordinates, please try again.");
    } 
}

Now it's easy to see what you can do with that try block: you move it to the helper method! And the result is this:

public static void GetMoveCoordinates(ref int xCoordinate, ref int yCoordinate)
{
   bool validCoordinate = true;

   do
   {
    Console.Write("Enter x coordinate: ");
    GetCoordinate(ref validCoordinate, ref xCoordinate);

    if (validCoordinate)
    {
        Console.Write("Enter y coordinate: ");
        GetCoordinate(ref validCoordinate, ref yCoordinate);
    }

    Console.WriteLine();
   } while (!validCoordinate);
} 

private static void GetCoordinate(ref bool validCoordinate, ref int coordinate)
{
    try 
    {
        coordinate = int.Parse(Console.ReadLine());
    } 
    catch
    {
        Console.WriteLine("Invalid coordinates, please try again.");
        validCoordinate = false;
        return;
    }


    if (coordinate < 1 || coordinate > 3)
    {
        validCoordinate = false;
        Console.WriteLine("Invalid coordinates, please try again.");
    } 
}
\$\endgroup\$
4
  • \$\begingroup\$ Tip: Use two or three hash characters to get smaller headlines. \$\endgroup\$
    – Guffa
    Commented Nov 27, 2014 at 22:05
  • \$\begingroup\$ On the subject of comments: the only thing unclear in this short example is WHY the coordinate must be between 1 and 3. \$\endgroup\$
    – jmoreno
    Commented Nov 28, 2014 at 7:57
  • \$\begingroup\$ the array of the board is 4/4 and you can only use positions 1,3 \$\endgroup\$
    – Needham
    Commented Nov 28, 2014 at 8:00
  • \$\begingroup\$ @Needham: Jeroen quite correctly points out that your code has a useless comment, I was suggesting that he improve his answer by also pointing out where you are missing a potentially useful comment. The code is small, and except for that one detail can be understood without need for comments. BTW conditions are prime locations for useful comments, explaining why you choose one path over another \$\endgroup\$
    – jmoreno
    Commented Nov 28, 2014 at 8:42
3
\$\begingroup\$

I'm simply trying to catch exceptions or prevent them from being thrown in the first place.

Prevent exception from beeing thrown is in my opinion the best exception handling. So let us check where exception can be thrown.

  • Console.ReadLine() can throw

    • an IOException which can bubble up, as we can't prevent it.
    • an OutOfMemoryException which can bubble up, as we can't prevent it.
    • an ArgumentOutOfRangeException which can bubble up, as we can't prevent it.
  • Int32.Parse() can throw

    • an ArgumentNullException which we can easily prevent.
    • an FormatException which we can prevent but using own code would blow up our code.
    • an OverflowException which we can prevent but using own code would blow up our code.

Here the way to go is, instead of using Int32.Parse() which can throw 3 types of exceptions, to use Int32.TryParse() which doesn't throw.

This would result in @Jeroen Vannevel's method

private static void GetCoordinate(ref bool validCoordinate, ref int coordinate)
{
    validCoordinate = Int32.TryParse(Console.ReadLine(), out coordinate);

    if (!validCoordinate)
    {
        Console.WriteLine("Invalid coordinates, please try again.");
        return;
    }

    if (coordinate < 1 || coordinate > 3)
    {
        validCoordinate = false;
        Console.WriteLine("Invalid coordinates, please try again.");
    }
}

But I think it isn't necessary to have 2 ref variables here, especially if the return type is void. So let us change this

private static Boolean GetCoordinate(ref int coordinate)
{
    if (Int32.TryParse(Console.ReadLine(), out coordinate))
    {
        return (coordinate > 0 && coordinate < 4);
    }
    Console.WriteLine("Invalid coordinates, please try again.");
    return false;
}  

and we also have removed some more code duplication.

The GetMoveCoordinates() method needs than to be changed to

public static void GetMoveCoordinates(ref int xCoordinate, ref int yCoordinate)
{
    bool validCoordinate = true;

    do
    {
        Console.Write("Enter x coordinate: ");
        if (validCoordinate = GetCoordinate(ref xCoordinate))
        {
            Console.Write("Enter y coordinate: ");
            validCoordinate = GetCoordinate(ref yCoordinate);
        }

        Console.WriteLine();
    } while (!validCoordinate);
}  

But I still don't like some of this.

  • again having 2 ref variables
  • why should the user enter the x coordinate again if only the y coordinate had been wrong
  • I prefer a While loop over a Do..while because if I scan the code I do it from top to bottom and therefor I see the ending condition first. But that is a matter of taste.

So instead of having 2 ref variables let us introduce a small MoveCoordinate class.

public class MoveCoordinate
{
    public Int32 X { get; set; }
    public Int32 Y { get; set; }
}

and change the GetMoveCoordinates() method to

private static MoveCoordinate ReadMoveCoordinate()
{
    MoveCoordinate moveCoordinate = new MoveCoordinate();

    moveCoordinate.X = GetCoordinate("Enter x coordinate: ");
    moveCoordinate.Y = GetCoordinate("Enter y coordinate: ");

    return moveCoordinate;
}

private static Int32 GetCoordinate(String description)
{
    int coordinate = 0;
    do
    {
        Console.Write(description);
    } while (!GetCoordinate(ref coordinate));

    return coordinate;
}

in this way the user only needs to enter the coordinate again which he/she did wrong.

In respect of jmoreno's good comment we change form ref to out and also add a String coordinateLetter parameter.

private const String inpuDescription = "Enter {0} coordinate: ";
private const String errorDescription = "Invalid {0} coordinate, please try again.";
private static Int32 GetCoordinate(String coordinateLetter)
{
    int coordinate = 0;
    String inputMessage = String.Format(inpuDescription, coordinateLetter);
    do
    {
        Console.Write(inputMessage);
    } while (!GetCoordinate(out coordinate, coordinateLetter));

    return coordinate;
}

private static Boolean GetCoordinate(out int coordinate, String coordinateLetter)
{
    if (Int32.TryParse(Console.ReadLine(), out coordinate))
    {
        return (coordinate > 0 && coordinate < 4);
    }
    Console.WriteLine(String.Format(errorDescription, coordinateLetter));
    return false;
}  

and call it

private static MoveCoordinate ReadMoveCoordinate()
{
    MoveCoordinate moveCoordinate = new MoveCoordinate();

    moveCoordinate.X = GetCoordinate("x");
    moveCoordinate.Y = GetCoordinate("y");

    return moveCoordinate;
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Just two comments. I would suggest that since you are rewriting the way errors are handled already, that the "invalid coordinates" message be customized "invalid x coordinates" or "invalid x coordinates". You can do this by passing in a coordinate description of "x" or "y" and then passing that along to the method with the ref parameter -- which by the way, should be an out parameter not ref. \$\endgroup\$
    – jmoreno
    Commented Nov 28, 2014 at 7:52
2
\$\begingroup\$

Consider the following version, which preserves the same experience (for example, if X is valid, Y is invalid, your original logic will still ask for X again)

public static void GetMoveCoordinates(ref int XCoordinate, ref int YCoordinate)
{
    while (true)
    {
        Console.WriteLine();
        Console.Write("Enter x coordinate: ");

        if (!int.TryParse(Console.ReadLine(), out XCoordinate) ||
            XCoordinate < 1 ||
            XCoordinate > 3)
        {
            Console.WriteLine("Invalid x coordiantes, please try again.");

            continue;
        }

        Console.Write("Enter y coordinate: ");

        if (!int.TryParse(Console.ReadLine(), out YCoordinate) ||
            YCoordinate < 1 ||
            YCoordinate > 3)
        {
            Console.WriteLine("Invalid y coordiantes, please try again.");

            continue;
        }

        break;
    }
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Whilst this preserves line space, I find it harder to read due to the compression of many steps into one (reading input, checking input validity etc). This will also be harder to debug/step through what's going on, should the functionality be broken in any way. Further: I was always taught that using while(true), with continue; and break; to control the loop's flow was bad practice. \$\endgroup\$
    – Robotnik
    Commented Nov 28, 2014 at 0:34
  • \$\begingroup\$ @Robotnik I have tweaked the formatting a little bit. I hope it has improved readability compared to previous version. I agree with you that compression of many steps in many cases is not good. But IMHO, there are few cases, where the logic is so simple and small, that I do prefer it, in order to avoid introducing extra variables etc (for example, Console.ReadLine above). Because when you have a little too many intermediate variables, at least my mind thinks. oh where all this variable is being used. and then you end up with much more to track. \$\endgroup\$ Commented Nov 28, 2014 at 0:45
  • \$\begingroup\$ As for while(true), I have been a purist before, and started the same way, thinking the same thing.. but eventually I realized same thing as above.. If it really is very simple and small, I don't mind it as exception. In addition to the two intermediate variables for Console.ReadLine, my intent is to avoid yet another one for managing the loop. Cutting down on them is what makes this code so small (and efficient), and IMHO it becomes easy to see the core logic, and personally I like that. \$\endgroup\$ Commented Nov 28, 2014 at 0:49
  • \$\begingroup\$ Sorry.. one more comment - About debugging - If I had to debug this, I wouldn't worry about checking what Console.ReadLine is returning.. I know it will return what I enter on the Console. Granted, that there will be many cases where this may not be true.. but then this answer is for the specific scenario in question ("Please can you take a look and tell me if there is a way to make my working code more efficient."), and is certainly not a general recommendation. \$\endgroup\$ Commented Nov 28, 2014 at 0:57

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.