patterncsharpModerate
Calculating the results of bit operations
Viewed 0 times
theoperationsbitcalculatingresults
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 Right Shift", ">>");
byteOperation = (byte) (a > 8 - b);
int16Operation = (short) (a > 16 - b);
int32Operation = a > 32 - b;
int64Operation = a > 64 - b;
WriteLine("Cir
```
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 Right Shift", ">>");
byteOperation = (byte) (a > 8 - b);
int16Operation = (short) (a > 16 - b);
int32Operation = a > 32 - b;
int64Operation = a > 64 - b;
WriteLine("Cir
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 !");
elseThat'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 !");
}
elseWhat 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 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?
Code Snippets
private static int a;
private static int b;
private static byte byteOperation;
private static short int16Operation;
private static int int32Operation;
private static long int64Operation;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 !");
elseif (!numberA || !numberB)
{
Console.WriteLine("Input can be only numbers !");
}
elseprivate 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"
}Context
StackExchange Code Review Q#122513, answer score: 10
Revisions (0)
No revisions yet.