debugcsharpMinor
Try/catch in a Tic-Tac-Toe game
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 GetMoveCoordinatesSolution
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
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
I can see why you might have done this (invalid input sets
Change it to
Braces around if statements
Some will argue this but to avoid misconceptions or problems with future changes: always place
Don't overassign
Your
No typos
Yuck.
Don't repeat yourself
It seems to me that both
Useful comments
I know that is the end of
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:
Now it's easy to see what you can do with that
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.