# Calculating the results of bit operations

Posted on

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

int value;
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?