1

I have a code that is something like this in a class

string method x (){ 
  foreach(a in alist){
    //do something
  }
  return string;
}
integer method y (){ 
  foreach(a in alist){
    //do something
  }
  return integer;
}
double method z (){ 
  foreach(a in alist){
    //do something
  }
  return double;
}

I sense a code smell here in the multiple for loops on the same object. But I am not sure whether it is real or not. Is there any way I can refactor this code? so that I only have one place for the for loop?

  • Can you explain exactly why this is allegedly a code smell? It isn't, but understanding your reasoning allows for writing an answer that addresses the misunderstanding. – Flater Feb 1 at 1:26
  • I feel like going through the same object different methods is a code smell. But as I mentioned earlier I am not sure about that. What if change the name of the alist variable in the future? I would have to make change at least 3 places. That's why I believe this is a code smell. Is there any way I can extract the foreach to a common place and get the same desired output? – Sumodh S Feb 1 at 1:43
  • As an aside, method is not a recognized C# keyword. Are you sure the C# tag is correct? – Flater Feb 1 at 2:02
  • Consider it as psudo code.. – Sumodh S Feb 1 at 4:00
  • In Python you have a numeric library (Numpy), which has a decorator called vectorize, which allows to replace the foreach loop with a decorator. Less repetition and in my opinion cleaner and clearer than your solution. Since I don’t know if such a solution is valid for C#, I leave this as a comment instead of an answer. For anyone with C# knowledge: please feel free to take this idea and post a proper answer. – agtoever Feb 1 at 8:20
2

It's not inherently a code smell

There is nothing wrong with having a foreach in multiple methods, unless you always run these three methods consecutively, at which point you can simplify it to:

public void xyz()
{
    foreach(a in alist)
    {
        x(a);
        y(a);
        z(a);
    }  
}

But if you do not always call these methods in the same succession (which I'm suspecting is the case), then this does not apply.

I feel like going through the same object different methods is a code smell

If you were calculating a value based on the same input parameters, I'd agree. However, this is not the case here. Each method requires its own enumerator to do its own enumeration.

What if change the name of the alist variable in the future? I would have to make change at least 3 places.

This would apply to every variable, method, class name, or namespace you would ever use. It would literally render you unable to write any code that wouldn't allegedly smell.

With the right IDE (or extension), this isn't a problem, since you can refactor names, which will change all references to that name across the codebase.

In the case of Visual Studio, that's done by pressing F2 while your cursor is on the name (variable/method/class/...).

Is there any way I can extract the foreach to a common place and get the same desired output?

Not meaningfully so. I mean, you could abstract it, but it would add complexity without actually improving performance or maintainability. It would detract from readability, not just because of the complexity increase, but also because you change a well known concept to something homebrewed, which requires additional knowledge for a developer to follow when they read the code.

Several drawbacks, no benefits. No reason to do this.


The only reasonable case for a code smell would not be for the enumeration itself, but any preliminary filtering logic. For example:

public string x ()
{ 
    foreach(a in alist.Where(a => a.Foo == "Bar"))
    {
        //do something
    }
    return string;
}

// And the same Where() for y() and z()

Here, the filtering logic itself can be abstracted to avoid needless repetition:

private IEnumerable<A> aBarList => alist.Where(a => a.Foo == "Bar");

public string x ()
{ 
    foreach(a in aBarList)
    {
        //do something
    }
    return string;
}

// And the same Where() for y() and z()

This is just one of many possible implementations. Your vague example code makes it impossible to judge the best implementation for your current case.

| improve this answer | |
  • Thanks for the quick reply. I have been scratching my head over this from yesterday evening – Sumodh S Feb 1 at 2:00
  • Also, in a strongly typed language like C#, you have to have separate loops. – user949300 Feb 1 at 4:27
0

It's not a code smell. In fact Martin Fowler over at [refactoring.com][1] argues you should repeat a loop if it adds value because of the trivial computing cost of repeating said loop.

You should make alist a function argument and make the function static (if possible in the real world)

| improve this answer | |

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.