C#; Working code--needs review

Question

Techies-- This routine does more once the channels + the subsets of existing batch arrays are re-distributed for processing. However, the success of that will only work as well as the initial splits. The threshold value is dubbed here, but it will come from app.config as configurable setting. Obviously the jagged string array is dubbed for review purposes. The real values will be coming from another routine and will frequently have counts > 100000. The 3 lines of comments where one will do is a product of formatting the code for StackExchange.

Please review what's here.

   #region Channel Assignment Testing

    static void ChannelAssign()
    {
        int THRESHOLD = 1;  

        string[] batch = new string[] 
            { "item1", "item2", "item3", "item4","item5","item6","item7" };
        int batchcnt = batch.Count();
        int subsetqty; // subset batch item quantity
        string[][] subsets; // Split extension returns these values
        int channelidx; // index for channels back from Split


        if (THRESHOLD != 0) //avoid accidental division by 0.
        {
            float channels = batchcnt/THRESHOLD;
            if (channels < 1) // can handle all items in existing batch based on 
                              // threshold setting.
            {
                channels = 1; // dub as a single channel
                subsetqty = batchcnt; // process all batch items in batch 
                channelidx = 1; // dub index for a single channel
                subsets = batch.Split(batchcnt); // ship the batchcount 
                                                 // to split routine.
            }
            else // we need at least 2 channels; round up from float 
                 // and decide how many.
            {
              channels =  (int)Math.Round(channels, 
                            MidpointRounding.ToEven); //determines channel# estimate
              subsetqty = batchcnt/(int)channels; // estimates # of 
                                                  // existing batch items to 
                                                  // place in each subset
              subsets = batch.Split(subsetqty); // actual subsets of batches returned
              channelidx = subsets.GetLength(0); //gets actual channel# assigned 
                                                 // by split

            }

            //distribute contents of batch into subsets for channel consumption
            for (int channel = 0; channel < channelidx; channel++)
            {
                for(int i = 0; i < subsets[channel].Length; i++)
                {
                    Console.WriteLine(" Channel:" + channel.ToString() + " ItemName: 
                      {0} ", subsets[channel][i]);
                }
            }
        }
        else
        {
            Console.WriteLine("Threshold value set to zero. This is an 
                                invalid value. Please set THRESHOLD.");
        }

    }

    #endregion

Split Extension

        public static T[][] Split<T>(this T[] arrayIn, int length)
        {
            bool even = arrayIn.Length % length == 0;
            int totalLength = arrayIn.Length / length;
            if (!even)
                totalLength++;

            T[][] newArray = new T[totalLength][];
            for (int i = 0; i < totalLength; ++i)
            {
                int allocLength = length;
                if (!even && i == totalLength - 1)
                    allocLength = arrayIn.Length % length;

                newArray[i] = new T[allocLength];
                Array.Copy(arrayIn, i * length, newArray[i], 0, allocLength);

            }

            return newArray;
        }

Answer 1

I like the formatting, use of white space and tabs.

I'm not sure what THRESHOLD is supposed to represent or where its set, but in the example, I'd make it a const.

I would do the check for 0 right below the assignment:

if (THRESHOLD == 0)
{
    ...

    return;
}

This will un-nest the main logic of your function and catch the error before any processing is done.

The logic should be moved out into their own functions:

var subsets = (channels < 1) ? ProcessWholeList(...) : ProcessPartialList(...);

This will better portray then intention of the logic and remove most of the unimportant variables out of the function cleaning it up quite a bit.

I like that you have created an extension to do the split, not many people think of that.

I would change the bool even ... into a check for odd numbers. That what you could change the

if (!even)

to

if (odd)
{
    ....
}

Again, expresses the intent a little better. I would also either move the assignment to right above the check.

This assignment:

int allocLength = length;
if (!even && i == totalLength - 1)
    allocLength = arrayIn.Length % length;

Should be changed to:

var allocLength = (!even && i == totalLength - 1) ? arrayIn.Length % length : length;

There are just a few simple changes that will clean it up quite a bit. I hope this is a good start and gives you some ideas.

I have another question, is there a reason why you are using a 2 dimensional array rather than one of the build in IEnumerables (i.e. HashTable)? I think for performance purposes, the two dimensional array would be the slowest way of doing it.

Answer 2

My biggest issue is with ambiguous variable names and excessive comments. The reason why you wrote so many comments is that the variable names and the method names do not speak to you. Here is my first attempt to clean it up:

    public static void AssignChannels()
    {
        int threshold = 1;

        // What sort of batch? You can find a better variable name here.
        string[] batch = new string[] { "item1", "item2", "item3", "item4", "item5", "item6", "item7" };

        int subsetQty; // Subset batch item quantity
        string[][] subsets; // Split extension returns these values
        int channelIndex; // Index for channels back from Split

        // This is not strictly necessary, a Debug.Assert would d as well and it will compile away in the release mode. 
        Contract.Requires(threshold != 0, "Threshold value set to zero. This is an invalid value. Please set the threshold.");

        float channelsEstimate = batch.Length / threshold;
        if (channelsEstimate < 1) // can handle all items in existing batch based on 
        // threshold setting.
        {
            channelsEstimate = 1; // dub as a single channel
            subsetQty = batch.Length; // process all batch items in batch 
            channelIndex = 1; // dub index for a single channel
            subsets = batch.Split(batch.Length); // ship the batchcount to split routine.
        }
        else
        {
            // We need at least 2 channels; round up from float and decide how many.
            channelsEstimate = (int) Math.Round(channelsEstimate, MidpointRounding.ToEven);
            subsetQty = batch.Length / (int)channelsEstimate; // Estimates # of existing batch items to place in each subset.
            subsets = batch.Split(subsetQty); // Actual subsets of batches returned
            channelIndex = subsets.GetLength(0); // Gets actual channel# assigned by split
        }

        PrintResult(subsets, channelIndex);
    }

    private static void PrintResult(string[][] subsets, int channelIndex)
    {
        // Distribute contents of batch into subsets for channel consumption
        for (int channelLoopIndex = 0; channelLoopIndex < channelIndex; channelLoopIndex++)
        {
            for (int i = 0; i < subsets[channelLoopIndex].Length; i++)
            {
                Console.WriteLine(" Channel:" + channelLoopIndex.ToString() + " ItemName: {0} ", subsets[channelLoopIndex][i]);
            }
        }
    }