When do repetitive operations become a code smell? I read this article by Charles Petzold where he suggested this and was wondering what people thought.
- Anybody can ask a question
- Anybody can answer
- The best answers are voted up and rise to the top
The question isn't how many. 3 is not more "loop-ish" than 2.
The question is one of "For All" or "There Exists".
If the post-condition is "for all", then you must write a loop. "For all two" or "for all three" doesn't matter. All that matters is "for all".
Similarly, if the post-condition is "there exists", then you must write a loop. "There exists in these two" or "there exists in these three" doesn't matter. All that matters is "there exists."
If you want to micro-optimize and claim that a "for all" that process two elements doesn't really need a loop, you have too much free time on your hands to think about irrelevant subtleties.
If you want to micro-optimize and claim that a "there exists" that chooses among what is (currently) two choices, you're looking too deeply. Two choices is isomorphic to n choices. Just write the n choice code and move on.
Please don't micro-optimize.
3 or more of what?
"One, two, many" isn't necessarily a bad way to think, as a programmer, but the more important question really is, what are you counting? If you haven't properly defined that, then the "rule" is little more than an empty platitude.
If a class has 3 properties or more, do you refactor them all into an array? Probably not under normal circumstances, since that would just obscure the semantics of those properties. But then again, if those properties are named
Similarly, you could use this "rule of thumb" to justify replacing
Technically, you could abstract virtually all of your program logic into a set of rules and actions and plug them all into a workflow engine like WF or BizTalk. Of course, as any developer who has experienced it can tell you, overuse of those tools rarely results in a clear, robust, maintainable, or performant system.
Aphorisms and rules of thumb are no substitute for thoughtful, targeted refactoring. As a design principle, this is called Don't Repeat Yourself (DRY). In order to accomplish this to any useful degree, you have to possess a clear vision of what constitutes a fact in your domain - and this often tends to vary by domain.
That's the question you should be asking - not just now, but constantly, as you code. Am I repeating myself here? Am I stating the same facts or making the same decisions over and over again? Yes? Then refactor. Otherwise, leave it alone.
The number of instances doesn't really matter so much. You might have only one "thing", but may still decide that it is worth the effort to create an abstraction because it might become many things. Similarly, you might have 10 "things", but if you know that there is never ever going to be an 11th, then it might make no sense to refactor. Unit tests are one example of repetition that is often difficult and rarely beneficial to avoid. It depends on the situation.
So again, focus on what's being repeated, and how often it could be repeated, not just on the number of times it is repeated right now. There are no best practices.
I say no, but for reasons that are (I think) subtly different that existing answers. I also say don't worry about it.
The key point is readability and maintainability. In the past, there may also have been optimisation issues, but by now the compiler can probably optimise better (and certainly more consistently) than you can, so that's almost always a non-issue.
However, even a simple sequence of near-identical statements can be more readable and maintainable than a loop with expressions based on the loop variable. How many times is certainly relevant, but there might be special cases where I'd go as high as ten, perhaps more. Yes - I really would cut and paste.
Key point - make the repeated sequence short. If it's not a one-liner, you should probably be putting the detail into a function, and sequencing calls. This applies IMO even if you wouldn't bother for a short loop, because it helps draw attention visually to the repeating pattern (and any exceptions to that pattern).
The readability benefit is that the values are there explicitly. Depending on the context, the expressions may be more readable, or the values may be more readable. It's subjective, and depends on the particular case. Certainly having five calls in sequence makes it visually obvious that it's repeated-five-times, whereas a loop may need closer attention.
The maintainability thing is that when there's a pattern that can be exploited in a loop now, that pattern may sometimes be illusory - the requirements may change in a way that breaks the pattern, making the loop either hopeless or requiring conditionals that complicate it. You can loop over a data table to work around that, but writing out the sequence of table entries is in principle no different to writing out a sequence of explicit calls. You could even consider it a case of writing a special-case virtual machine interpreter (the loop) with a non-looped sequence of instructions (the data table entries).
One thing, though - if the loop (or sequence) is small enough that either option is viable, it's hard to imagine it being a big deal either way. In that sense, even though the issue is readability/maintainability rather than performance, this is still pointless micro-optimisation of readability/maintainability. I'm not quite in agreement with S.Lott because I don't believe in strict absolute rules, but I certainly wouldn't advocate spending time worrying about this.
Basically, a little time spent thinking about this issue "in principle" is a good thing, but when you're working, just go with your first instinct and - unless the result is clearly evil - leave it and move on to more important things.