current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

This code is about manipulating each value in a 2D array. My method below is called based on how I want the values in the array to be manipulated (process).

For the first method in the switch statement - as an example, I am calling AllCaps() method to manipulate the value.

What I am trying to avoid is repeating statements when I can put it in a method instead and just call it when needed. With my code below, though, it seems like I am making the program work too hard by performing a check on each iteration - which could be in the thousands.

Is it effective to call a method for processing each 2D array value inside the nested for loop? Or call the method first and perform the nested for loop inside each method? 

private void ProcessSelectedRange(string process)
{
    selectedRange = GetSelectedRange();

    // initialize and populate 2d array with values from selected range
    curValue = new object[,] { };
    curValue = selectedRange.Value;

    // possibly more statements

    // loop through curValue 2d array
    for (int i = 1; i <= curValue.GetLength(0); i++)
    {
        for (int j = 1; j <= curValue.GetLength(1); j++)
        {
            string str = string.Empty;

            // continue if curValue[i, j] is null or empty
            try
            {
                str = curValue[i, j].ToString();
                if (string.IsNullOrEmpty(str)) continue;
            }
            catch { continue; }


            //********************************************************************

            // call method depending on what we want to do with the value
            switch (process)
            {
                case "AllCaps": AllCaps(str); break;
                case "Process2": Process2(str); break;
                case "Process3": Process3(str); break;
                case "Process4": Process4(str); break;
                // possibly more methods
                default: Process5(str); break;
            }

            //********************************************************************
        }
    }
}
share|improve this question

2 Answers 2

up vote 3 down vote accepted

Exception

Avoid getting them being thrown if possible, as they are costly.

if (curValue[i, j] == null)
    continue;

var str = curValue[i, j].ToString();
if (string.IsNullOrEmpty(str))
    continue;

Loop

The condition will be evaluated every cycle. You do not need to GetLength on each of the loop. The result can be cached : 

// could be named rowCount and columnCount; I am not sure which is which
var length0 = curValue.GetLength(0), length1 = curValue.GetLength(1)

// when you have 2 for-loop like that, you don't need to intend all of them
// this is mostly a style choice, but it helps to prevent building a pyramid
for (int i = 1; i <= length0; i++)
for (int j = 1; j <= length1; j++)
{
   // ...
}

The switch part can be moved out of the loop, as the process doesn't change during the loop. This can save us from some string comparisons.

Action<string> processString;
switch (process)
{
    case "AllCaps": processString = AllCaps; break;
    case "Process2": processString = Process2; break;
    case "Process3": processString = Process3; break;
    case "Process4": processString = Process4; break;
    // possibly more methods
    default: processString = Process5;
}

for (...)
for (...)
{
    // ...
    processString(str);
}
share|improve this answer
    
Caching the lengths may not be necessary. The compiler should inline and cache them for you as it does with Array.Length, IIRC. –  Cole Johnson Aug 24 at 6:48
2  
Please use bracing and indenting in the for loops. Building a pyramid is prevented by refactoring, not be not indenting –  Caridorc Aug 24 at 7:46
    
I like this. Thank you for pointing out the Exception. With Loop, I try not to use a variable if I am only using the value once. I think I remember convincing myself that I may be saving some memory allocation by minimizing the use of variables - for one time use. In this case though I understand your point. Action<>. Now this is interesting. This may also be the answer to one of the programs I am working on. This is enlightening. Thank you. –  BDot17 Aug 24 at 13:29
up vote 3 down vote

Removing the check on each iteration is definitely a good idea. My advice: have a separate method for each process. E.G. 

SelectedRange = GetSelectedRange()

AllCapsRange(SelectedRange)
Process2Range(SelectedRange)

then, if you need to select a case, you only need to do it once, and not \$n^2\$ times.

As a general rule, the ONLY things that should go inside an iterative loop are things which could change during the iteration. Anything else should be declared/set beforehand.

share|improve this answer
    
Thank you for sharing your view Zak, and I like your general rule. I'll keep that in mind. –  BDot17 Aug 24 at 13:31

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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