Bit shifting and masking

Question

I was wondering if someone could review this simple bit of code that originally was in C++, but now converted to C# as practice in using bit-shifting and creating a sort of a quasi-array functionality. Consider the partition size as a size of our array which can only hold a specific size limit for said partition. It was assumed that input is always valid, and thus no fail safes were included. The usage of unsigned integers is intentional as we want only positive values.

If you're wondering about the liberal usage of type casting, it is because it appears that C# is far stricter than C++ with respect to data types.

using System;
namespace bitShifting
{
 class Program
 {
 uint bitSize, shiftCount, mask, partionSize;
 void setValue(ref uint var, uint k, uint i, uint val)
 {
 bitSize = sizeof(uint) * 8;
 partionSize = (uint)(bitSize / k);
 shiftCount = partionSize * i;
 mask = (uint)~(((1 << (int)partionSize) - 1) << (int)shiftCount);
 var = var & mask;
 val = val << (int)shiftCount;
 var = var | val;
 }
 uint getValue(uint var, uint k, uint i)
 {
 bitSize = sizeof(uint) * 8;
 partionSize = (bitSize / k);
 shiftCount = partionSize * i;
 mask = (uint)(((1 << (int)partionSize) - 1) << (int)shiftCount);
 var = var & mask;
 var = (var >> (int)shiftCount);
 return var;
 }
 static void Main(string[] args)
 {
 Program p = new Program();
 uint k;
 uint value;
 uint maxValue;
 uint var = 0;
 Console.WriteLine("C++ Lab to C#, Practice Using Pass by Ref");
 Console.WriteLine("Valid Partition K size are: 1, 2, 4, 8, 16, 32.");
 Console.WriteLine("Enter Partition Size");
 k = Convert.ToUInt32(Console.ReadLine());
 maxValue = (uint)((1 << (int)(sizeof (uint) * 8 / k)) - 1);
 if(k == 1)
 {
 maxValue = 2147483647;
 }
 Console.WriteLine("Maximal Value for Partion of Size {0} is 0 to {1}", k, maxValue);
 bool getValue = true;
 for(uint i = 0; i < k; i++)
 {
 Console.WriteLine("Input a Value");
 value = Convert.ToUInt32(Console.ReadLine());
 if (k == 1)
 {
 Console.WriteLine("Value at Index 0 is: {0}", value);
 getValue = false;
 }
 else
 {
 p.setValue(ref var, k, i, value);
 }
 }
 for(uint i = 0; i < k; i++)
 {
 if (getValue)
 {
 value = p.getValue(var, k, i);
 Console.WriteLine("Value At Index {0} is: {1}", i, value); 
 }
 }
 }
 }
}

Answer 1

Layout

• Put your main method on the top, since it tells main purpose of the application. And, C# isn't like C++, you don't have to define the method ahead, before calling them.

  class Program
  {
   uint bitSize, shiftCount, mask, partionSize;
   static void Main(string[] args);
   void setValue(ref uint var, uint k, uint i, uint val);
   uint getValue(uint var, uint k, uint i);
  }
  

  ---

   // Even in c++, you can declare a method signature first
   // This part could also go into header file
   void setValue(PUINT var, UINT k, UINT i, UINT val);
   uint getValue(UINT var, UINT k, UINT i);
   int main(int argc, char *argv[]) { /* ... */ }
   // And, implement them later
   void setValue(PUINT var, UINT k, UINT i, UINT val) { /*...*/ }
   uint getValue(UINT var, UINT k, UINT i) { /*...*/ }
  

• Mark the class variables and methods static, so you can call them directly.

  // Instead of awkward hack/patch like this
  Program p = new Program();
  p.setValue(...);
  

• Declare the local variables as you need them. Except when the assignment is inside a code block, but you also need to use them outside of that.

Naming

• PascalCasing for any method, class, public property. And, camelCasing for private and local variable.
• Avoid using keywords as variable name, especially var the most used one.
• Avoid single letter variable, other than for for loop, lambda expression...

Misc

• Avoid magic numbers.

  // What is 8? numbers of bits in a byte
  //var maxValue = (uint)((1 << (int)(sizeof(uint) * 8 / k)) - 1);
  const uint BitsPerByte = 8;
  //maxValue = 2147483647;
  maxValue = int.MaxValue;
  

You messed up big here...

1. A uint should be capable to store uint.MaxValue and not int.MaxValue.

2. You are mixing up partition size and partition count. With a partition size of 1 (k = 1), you should be able to enter 32 values (BitSize / k = 32 / 1).

   // And, not just 1 (`k`) value
   for(uint i = 0; i < k; i++)
   

3. The hard coded conditions k == 1 and getValue are huge red flags!

   // why?
   if(k == 1)
   {
    maxValue = 2147483647;
   }
   Console.WriteLine("Maximal Value for Partion of Size {0} is 0 to {1}", k, maxValue);
   // hack..
   bool getValue = true;
   for(uint i = 0; i < k; i++)
   {
    Console.WriteLine("Input a Value");
    value = Convert.ToUInt32(Console.ReadLine());
    // more hack
    if (k == 1)
    {
    Console.WriteLine("Value at Index 0 is: {0}", value);
    getValue = false;
    }
    else
    {
    p.setValue(ref var, k, i, value);
    }
   }
   for(uint i = 0; i < k; i++)
   {
    // hack++
    if (getValue)
    {
    value = p.getValue(var, k, i);
    Console.WriteLine("Value At Index {0} is: {1}", i, value); 
    }
   }
   

Let's fix this...

class Program
{
 const int BitsPerByte = 8, BitSize = sizeof(uint) * BitsPerByte;
 static void Main(string[] args)
 {
 Console.WriteLine("C++ Lab to C#, Practice Using Pass by Ref");
 Console.WriteLine("Valid Partition K size are: 1, 2, 4, 8, 16, 32.");
 Console.WriteLine("Enter Partition Size :");
 var size = int.Parse(Console.ReadLine());
 // using 1L (long) for computation to avoid overflow
 var maxValue = (uint)((1L << size) - 1);
 Console.WriteLine("A valid value for a Partion of Size {0} is between 0 and {1}.", size, maxValue);
 var data = default(uint);
 for (var i = 0; i < BitSize / size; i++)
 {
 Console.WriteLine("Input a Value :");
 var value = uint.Parse(Console.ReadLine());
 SetValue(ref data, size, i, value);
 }
 for (var i = 0; i < BitSize / size; i++)
 {
 Console.WriteLine("Value At Index {0} is {1}", i, GetValue(data, size, i));
 }
 }
 static void SetValue(ref uint data, int size, int index, uint value)
 {
 data |= value << (size * index);
 }
 static uint GetValue(uint data, int size, int index)
 {
 var mask = (uint)((1L << size) - 1);
 return data >> size * index & mask;
 }
}

EDIT: Explanation on the bit operation :

static void SetValue(ref uint data, int size, int index, uint value)
{
 /* :: Given that size = 8 and index = 2 (3rd), value = 10000001, data = 10101010 00000000 00000000 00000000
 00000000 00000000 00000000 10000001 // value
 00000000 10000001 00000000 00000000 // value left-shifted by 16 (size * index)
 00000000 10000001 00000000 00000000 // value left-shifted by 16 (size * index)
 10101010 00000000 00000000 00000000 // data
 ----------------------------------- // OR
 10101010 10000001 00000000 00000000 // result */
 data |= value << (size * index);
}
static uint GetValue(uint data, int size, int index)
{
 var mask = (uint)((1L << size) - 1);
 /* :: Given that size = 8 and index = 2 (3rd), value = 0b10000001, data = 10101010 10000001 00000000 00000000
 00000000 00000000 00000001 00000000 // (1L << size)
 00000000 00000000 00000000 11111111 // (1L << size) - 1 -> mask
 10101010 10000001 00000000 00000000 // data
 00000000 00000000 10101010 10000001 // data right-shifted by 16 (size * index)
 00000000 00000000 10101010 10000001 // data right-shifted by 16 (size * index) 
 00000000 00000000 00000000 11111111 // mask
 ----------------------------------- // AND
 00000000 00000000 00000000 10000001 // result */
 return data >> size * index & mask;
}

• SomeStudent: Thank you very much for the clear and well explained code review. I never knew C# had the equivalent of += for logic operators.
• CodesInChaos: Strictly speaking var isn't a keyword. But of course that doesn't make the advice to avoid it as a name for a local variable any less valid.