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

Try/catch in a Tic-Tac-Toe game

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
toeticcatchtacgametry

Problem

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

Solution

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  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  3)
    {
        validCoordinate = false;
        Console.WriteLine("Invalid coordinates, please try again.");
    } 
}

Code Snippets

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

Context

StackExchange Code Review Q#71023, answer score: 6

Revisions (0)

No revisions yet.