Problem
I have a simple program that calculates and outputs the results of all the bit operators. I’m planning to send it to my friend and to explain some stuff about them to him. Before I do that, I want to know if I can make any improvements to my program.
internal class Program
{
private static int a;
private static int b;
private static byte byteOperation;
private static short int16Operation;
private static int int32Operation;
private static long int64Operation;
private static void Main()
{
while (true)
{
bool numberA = int.TryParse(Console.ReadLine(), out a);
bool numberB = int.TryParse(Console.ReadLine(), out b);
if (!numberA || !numberB) Console.WriteLine("Input can be only numbers !");
else
{
Console.WriteLine();
byteOperation = (byte) (a & b);
int16Operation = (short) (a & b);
int32Operation = a & b;
int64Operation = a & b;
WriteLine("Binary AND", "&");
byteOperation = (byte) (a | b);
int16Operation = (short) (a | b);
int32Operation = a | b;
int64Operation = a | b;
WriteLine("Binary OR", "|");
byteOperation = (byte) (a ^ b);
int16Operation = (short) (a ^ b);
int32Operation = a ^ b;
int64Operation = a ^ b;
WriteLine("Binary XOR Operator", "^");
byteOperation = (byte) (a << b);
int16Operation = (short) (a << b);
int32Operation = a << b;
int64Operation = a << b;
WriteLine("Binary Left Shift", "<<");
byteOperation = (byte) (a >> b);
int16Operation = (short) (a >> b);
int32Operation = a >> b;
int64Operation = a >> b;
WriteLine("Binary Right Shift", ">>");
byteOperation = (byte) (a << b | a >> 8 - b);
int16Operation = (short) (a << b | a >> 16 - b);
int32Operation = a << b | a >> 32 - b;
int64Operation = a << b | a >> 64 - b;
WriteLine("Circular Shift", "<< | >>", "<<", "|", ">>");
}
}
}
private static void WriteLine(string text, string ch)
{
Console.ForegroundColor = ConsoleColor.Green;
Console.WriteLine(text + " : " + ch);
Console.WriteLine();
Console.ForegroundColor = ConsoleColor.Red;
Console.WriteLine("Byte : {0} " + ch + " {1} = {2}", a, b, byteOperation);
Console.WriteLine("Int16 : {0} " + ch + " {1} = {2}", a, b, int16Operation);
Console.WriteLine("Int32 : {0} " + ch + " {1} = {2}", a, b, int32Operation);
Console.WriteLine("Int64 : {0} " + ch + " {1} = {2}", a, b, int64Operation);
Console.WriteLine();
}
private static void WriteLine(string text, string chFull, string ch1, string ch2, string ch3)
{
Console.ForegroundColor = ConsoleColor.Green;
Console.WriteLine(text + " : " + chFull);
Console.WriteLine();
Console.ForegroundColor = ConsoleColor.Red;
Console.WriteLine("Byte : {0} " + ch1 + " {1} " + ch2 + " {0} " + ch3 + " 8 - {1}" + " = {2}", a, b, byteOperation);
Console.WriteLine("Int16 : {0} " + ch1 + " {1} " + ch2 + " {0} " + ch3 + " 16 - {1}" + " = {2}", a, b, int16Operation);
Console.WriteLine("Int32 : {0} " + ch1 + " {1} " + ch2 + " {0} " + ch3 + " 32 - {1}" + " = {2}", a, b, int32Operation);
Console.WriteLine("Int64 : {0} " + ch1 + " {1} " + ch2 + " {0} " + ch3 + " 64 - {1}" + " = {2}", a, b, int32Operation);
Console.WriteLine();
}
}
Solution
private static int a;
private static int b;
private static byte byteOperation;
private static short int16Operation;
private static int int32Operation;
private static long int64Operation;
That whole chunk of static instance fields should be passed around as parameters.
They’re only static so that you can use them from static context. static is like plague, a virus; it creeps and expands its tentacles across your entire code base if you let it.
Since this is just a learning exercise, I’ll let it slip this time and won’t say a thing about all the static methods being called from Main. But the fields shouldn’t exist.
Now this is confusing:
bool numberA = int.TryParse(Console.ReadLine(), out a);
bool numberB = int.TryParse(Console.ReadLine(), out b);
numberA and numberB are Boolean variables, but if you don’t pay attention, it’s very easy – WAY too easy, to glance at the code and think they’re numbers… that’s what their names say!
if (!numberA || !numberB) Console.WriteLine("Input can be only numbers !");
else
That’s a very confusing and dangerous way to write conditional code. Always use braces around scopes. – this would be much, much, much clearer and less error-prone:
if (!numberA || !numberB)
{
Console.WriteLine("Input can be only numbers !");
}
else
What you really need, is a separate method that gets the two numbers and only returns once you have valid input. You also need a way to break out of the loop and sanely quit the program! How many concerns is that? How many methods should it be written with?
private static Tuple<int,int> PromptForTwoNumbers()
{
var value1 = PromptForValidNumber();
if (value1 == null)
{
return null;
}
var value2 = PromptForValidNumber();
if (value2 == null)
{
return null;
}
return Tuple.Create(value1, value2);
}
private static int? PromptForValidNumber()
{
while (true)
{
Console.WriteLine("Please enter a number:");
int value;
var input = Console.ReadLine();
if (IsUserTiredOfThis(input))
{
return null;
}
if (int.TryParse(input, out value))
{
return value;
}
Console.WriteLine("Input must be a number!");
}
}
private static bool IsUserTiredOfThis(string input)
{
return string.IsNullOrEmpty(input) || input == "quit"
}
Now, all xxxxOperation variables are misnamed. They’re not operations, they’re results.
Your WriteLine method is also confusing: until you get to the actual implementation, there’s no way of telling that method is going to actually be writing 7 lines to the console.
The way you’re writing is awkward:
Console.WriteLine("Byte : {0} " + ch1 + " {1} " + ch2 + " {0} " + ch3 + " 8 - {1}" + " = {2}", a, b, byteOperation);
Console.WriteLine("Int16 : {0} " + ch1 + " {1} " + ch2 + " {0} " + ch3 + " 16 - {1}" + " = {2}", a, b, int16Operation);
Console.WriteLine("Int32 : {0} " + ch1 + " {1} " + ch2 + " {0} " + ch3 + " 32 - {1}" + " = {2}", a, b, int32Operation);
Console.WriteLine("Int64 : {0} " + ch1 + " {1} " + ch2 + " {0} " + ch3 + " 64 - {1}" + " = {2}", a, b, int32Operation);
Don’t mix string-formatting and + concatenations. And name things for Pete’s sake! “ch2” rings like “channel 2” in my mind, and tells me nothing about what that thing is supposed to be.
Your Int64 stuff is operating on Int32‘s. In fact, none of your operations are operating with the types they’re claiming to be:
byteOperation = (byte) (a | b);
Should be:
byteResult = (byte)((byte)a | (byte)b); // yuck!
You’re just working off Int32‘s and casting the result to a given type. Which means… you’re probably missing an unchecked scope around these operations to let them overflow without blowing everything up.
Or was overflowing your “clean way out” of the app?