# C#; Working code--needs review

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.

   #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
}

}

#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;
}

• What kind of review are you looking for? Are there performance problems? Are you looking at ways of doing things differently, just looking for code improvements? – dreza Jul 31 '12 at 20:20

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.

• Jeff, thanks so much for reviewing this! I really appreciate it. I think I was way too influenced by Bruce Eckel in the early "Thinking in C" days to have first gone to a Hash Table. Thank you for reminding me that its an option!;-) Again, thanks so much for looking at this. – plditallo Aug 1 '12 at 0:32

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]);
}
}
}

• Leonid, thank you for reviewing my work. I've cleaned up the variable names, implemented your print result concept, used Jeff's idea about splitting the whole or partial batch queues into separate functions--I can't believe how much better this code is!:-) – plditallo Aug 1 '12 at 0:48
• @plditallo, cool, you might want to add an edit to your question and show the final code. Jeff Vanzella's answer is great; it just does not show the final version. This is not as helpful to those who might want to learn from your problem and the solution. Once you share your "final" version, people might find additional ways to improve. Sometimes you re-factor in layers. – Leonid Aug 1 '12 at 15:41