6
\$\begingroup\$

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<string, object[]> 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;
    }
\$\endgroup\$
7
\$\begingroup\$

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 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<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;
}
\$\endgroup\$
  • \$\begingroup\$ Wow this is amazing!! You have given megreat information to learn from. Let me review it alltonight. Thanks Heslacher! \$\endgroup\$ – cnewb Jan 29 '15 at 0:00
5
\$\begingroup\$

Adding to @Heslacher's excellent review, I would say that there is one thing in the signature that tickles reusability and extensibility:

public static void WriteToRange(this Dictionary<string, object[]> DicToRange, excel.Range toRange)

Do you see it?

That's right: this Dictionary<string, object[]> is the main reason for missing a heartbeat here.

Extend interfaces! You're extending a specific implementation of the System.Collections.Generic.IDictionary<TKey,TValue> interface. This means I can't use this extension method on a ConcurrentDictionary<TKey,TValue>, or a Sorted<TKey,TValue>, or any other possible implementation of IDictionary<TKey,TValue> ever written - and more importantly, those not written yet, too.

By extending a concrete type, you're... missing out on the best of extension methods.

\$\endgroup\$
  • \$\begingroup\$ Again - I am so grateful for that input. I did not even know to think of that, but now I made a concrete note of it. \$\endgroup\$ – cnewb Feb 1 '15 at 2:03
  • \$\begingroup\$ so I should be doing something like this: public static void WriteToRange(this IDictionary<string, object[]> DicToRange, excel.Range toRange). \$\endgroup\$ – cnewb Feb 1 '15 at 2:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.