patterncsharpMinor
VSTO Write Dictionary To Range Extension Method
Viewed 0 times
methodrangewriteextensionvstodictionary
Problem
I was hoping to receive some criticism on this block of code. I wrote it to write the contents of a dictionary to excel range in VSTO. It works well. I am interested to see how people could improve it. What would you do differently to make it more expressive, efficient?
public static void WriteToRange(this Dictionary DicToRange, excel.Range toRange)
{
object[,] storeDictionaryData = new object[DicToRange.Max(maxValues=> maxValues.Value.Length),DicToRange.Count];
int keyCounter=0;
int valueCounter =0;
foreach (var key in DicToRange.Keys)
{
if (keyCounter < storeDictionaryData.GetLength(1))
{
foreach (var value in DicToRange[key])
{
if (valueCounter < storeDictionaryData.GetLength(0))
{
storeDictionaryData[valueCounter, keyCounter] = value;
valueCounter++;
}
}
keyCounter++;
valueCounter = 0;
}
}
dynamic rangeToFill=toRange.get_Resize(storeDictionaryData.GetLength(0), storeDictionaryData.GetLength(1));
rangeToFill.Value = storeDictionaryData;
}Solution
You have a lot of unnecessary vertical spacing which should be removed to improve readability.
Based on the naming guidelines input parameter should be named using
Because
The
You don't need the checks
You should allow your variables to breathe by adding spaces between variables and operators.
Taking the above points into account we can refactor the method to
Based on the naming guidelines input parameter should be named using
camelCase casing. Because
WriteToRange() can be called also as a normal static method you need to check both input parameters for null. The
storeDictionaryData is basically a object[columns, rows] this should be reflected by the counter variables. You can just rename them valueCounter -> column, keyCounter -> row. You don't need the checks
keyCounter < storeDictionaryData.GetLength(1) and valueCounter < storeDictionaryData.GetLength(0) because at the point where you create the array you have taken care of the upper bounds of both dimensions. Both conditions won't evaluate to false. toRange is poorly named. Consider to name it destinationRange or simply destination. Then a good pattern would be also to rename DictToRange to source. You should allow your variables to breathe by adding spaces between variables and operators.
Taking the above points into account we can refactor the method to
public static void WriteToRange(this Dictionary source, excel.Range destination)
{
if (source == null) { throw new ArgumentNullException("source"); }
if (destination == null) { throw new ArgumentNullException("destination"); }
if (source.Count == 0) { throw new ArgumentOutOfRangeException("source"); }
object[,] storeDictionaryData = new object[source.Max(maxValues => maxValues.Value.Length), source.Count];
int column = 0;
int row = 0;
foreach (var key in source.Keys)
{
foreach (var value in source[key])
{
storeDictionaryData[column, row] = value;
column++;
}
row++;
column = 0;
}
dynamic rangeToFill = destination.get_Resize(storeDictionaryData.GetLength(0), storeDictionaryData.GetLength(1));
rangeToFill.Value = storeDictionaryData;
}Code Snippets
public static void WriteToRange(this Dictionary<string, object[]> source, excel.Range destination)
{
if (source == null) { throw new ArgumentNullException("source"); }
if (destination == null) { throw new ArgumentNullException("destination"); }
if (source.Count == 0) { throw new ArgumentOutOfRangeException("source"); }
object[,] storeDictionaryData = new object[source.Max(maxValues => maxValues.Value.Length), source.Count];
int column = 0;
int row = 0;
foreach (var key in source.Keys)
{
foreach (var value in source[key])
{
storeDictionaryData[column, row] = value;
column++;
}
row++;
column = 0;
}
dynamic rangeToFill = destination.get_Resize(storeDictionaryData.GetLength(0), storeDictionaryData.GetLength(1));
rangeToFill.Value = storeDictionaryData;
}Context
StackExchange Code Review Q#78719, answer score: 7
Revisions (0)
No revisions yet.