4

For example, in my project, I often found some head of for-loop appears many times, eg:

for(let i=0;i<SharedData.students.length;i++){
    SharedData.students[i].something=.....
}

if(isReset){
    for(let i=0;i<SharedData.students.length;i++){
        SharedData.students[i].reset();
    }
}
.
.
.

which the task that inside and outside for-loop are totally different, but it commonly needs

for(let i=0;i<SharedData.students.length;i++)

. So my question is, is copying and pasting

for(let i=0;i<SharedData.students.length;i++)

violating DRY principle?

  • 1
    No, it isn't. But you could have added the reset within the body of the original for loop, assuming there isn't any logical reason not to. – Robert Harvey Dec 14 '18 at 3:02
  • 2
    Yes it is, but since you can hardly avoid iterating over collections, restructuring your code is not the correct way to avoid this. Instead, the language should offer an easier way of achieving the same thing, and over time they often do (e.g. Java's enhanced for loop or map for streams). – Kilian Foth Dec 14 '18 at 7:17
  • Are You expecting an answer for a specific language? – Robert Andrzejuk Dec 14 '18 at 10:36
  • In C++ algorithms are prefered over raw loops. In C++20 to even improve this more "range algorithms" are been added. – Robert Andrzejuk Dec 14 '18 at 10:52
  • I know this isn’t exactly on-topic but I’d be FAR more worried about the need to reset a global state object in this way. – Sebastiaan van den Broek Dec 17 '18 at 4:10
27

The Don't Repeat Yourself (DRY) principle is easy to mindlessly over apply.

Keep in mind that the real sin isn't using copy and paste. It's spreading a design decision around in a way that makes it difficult to change that decision. If what you really have is two decisions that just happen to look the same at the moment then everything is fine. You'd be doing damage if you forced the two decisions to be expressed in the same place.

By leaving them as separate, as you have now, you're allowing the two loops to vary independently. If you rewrote them as Robert Harvey suggests:

for(let i=0;i<SharedData.students.length;i++){
    SharedData.students[i].something=.....
    if(isReset){
        SharedData.students[i].reset();
    }
}

then you'd lose the ability to easily make them vary independently (say by having one skip the first element, for whatever reason).

This idea can be hard to grasp so let me say it another way:

int x = 100;
int y = 100;    

Here is a "violation" of DRY that most people wouldn't think twice about. Why? Because we know that even though y is a redundant copy of x it might not always be. It has it's own meaning. We don't want to lose that meaning just because it happens to have the same value as x right now.

So please when you think about DRY think less about copy and paste and more about what you're making easy to change.

  • Arguably, if one loop needed to skip the first element, you're no longer in a situation where the two loops if they were written separately would be a repetition. But I agree with your general sentiment that the source of the problem is from making future maintenance more difficult and that this is the true concern. – Neil Dec 14 '18 at 9:07
  • « It's spreading a decision around in a way that makes it difficult to change that decision. » this also shows that DRY is just another way to say SRP. It’s all about not repeating yourself in terms of how you solve a problem or how concepts are applied in your application, not so much about code itself. – Steve Chamaillard Dec 14 '18 at 12:01
  • Of course, 100 in your last example is probably one of those dreaded "Magic Numbers". Or is it two of them? Anyway, good one. – Deduplicator Dec 14 '18 at 12:05
  • Amusingly, loop structures exist specifically so you don't have to repeat yourself. :-) – Blrfl Dec 14 '18 at 14:24
  • @SteveChamaillard DRY and SRP are not the same. They are in a sense "dual". DRY means don't have two code blocks for one unit of functionality. SRP means don't have two units of functionality for one code block. Although as you observe they have similar purposes. – Solomonoff's Secret Dec 14 '18 at 18:10
8

Is this violating the DRY principle?

To some degree, yes - and I think it is a little bit astonishing that some commenters here seem to overlook or deny it. Indeed, the consequences are often acceptable in lots of real-world cases, but I think it is worth to take a closer look at the example.

So let us assume for a moment this statement

for(let i=0;i<SharedData.students.length;i++){
    SharedData.students[i].something=.....

appears 100 times in a program. Then there are some design decisions which already became harder to change:

  1. the decision to have SharedData an attribute named students

  2. the decision that students is an indexable array

  3. the decision that students has a mutable field named something

(Of course, you did not write to repeat the inner part of the loop, but let me put this into this example for the purpose of demonstration)

So how can you mitigate these issues? The first one can be mitigated by avoiding to repeat the explicit expression SharedData.students more often than necessary. Often, a simple additional local variable can help:

 let studentArray = SharedData.students;
 for(let i=0;i<studentArray.length;i++){
        studentArray.something=.....

Note that this simple change alone divides the number of repetitions of SharedData.students by two. On a larger scale, you may consider to have several functions implemented in terms of a parameter studentArray instead of a parameter SharedData.

Issue #2 can be mitigated, for example, by using a foreach statement, if your programming language has such a thing:

  foreach(student in studentArray){
       student.something = ...

Now, it is only necessary to have students an iterable container, which is a weaker assumption than being an indexable array.

Issue #3 can be attacked by encapsulating the inner part of the for loop inside a function:

foreach(student in studentArray)
      DoSomething(student);

Now, the logic of manipulating or using student in a specific manner is in one place, not 100 any more.

It maybe also worth to have a look why such a for-head repeats so often inside a program. It may be a sign that the overall code section containing the for loop can be generalized, maybe by introducing the operation as a parameter itself (I prefer C# syntax, I guess you get the idea):

 void DoSomethingForAllStudents(Action<Student> DoSomething)
 {
     foreach(student in studentArray)
         DoSomething(student);
 }

But beware, this can already be overengineered, and if the cost of making things less DRY is overengineering you should often better leave those things as they are.

As I wrote at the beginning, in lots of real-world cases the named issues are design decisions which you are not going to change later during the whole lifetime of your program, or where the real number of repetitions is not that high. So even if this literally violating DRY, don't overthink this.

1

Yes, this is violating the DRY principle. The DRY principle isn't absolute however and should be used with a personal standard. We don't live in a perfect world.

The frequency of use is a big tell however. If you are copy and pasting the exact same code frequently, I'd say that you should DRY it out.

0

First an other, more recognizable, example of repeated for-loops:

for (let i = 0; i < columnCount; ++i) {
    write getColumnTitle(i);
}
writeln();
for (let rowI = 0; rowI < rowCount; ++rowI) {
    row = rows.get(i);
    for (let i = 0; i < columnCount; ++i) {
        write row.getColumnData(i);
    }
    writeln();
}

It is easy to agree, that the loop is a bit verbose (i.e. the i), an iterator, or better a stream (passing a lambda function expression being repeatedly called) might be better style.

But in this case the DRY principle does not hold.

However if you see the following pattern:

for i A
B
for i C
D
for i E
F

Then the problem is not DRY but rather that the code is working generally working on entire i-tuples, records, but the loop handles a second dimension.

rec.a()
B
rec.c()
D
rec.e()
F

This code looks like a sequential processing, and is almost a nested loop for j for i X Y.

The most likely error here is a violated Separation of Concerns.

For instance importing from an Excel file into a database also has this two dimensional approach: reading Excel, writing database records.

The best practise is:

  • make an Excel table reader, which can be passed:
    • a table definition
    • a row importing function
  • make a row database writer
  • make the integrating importer

Synopsis

  1. The rec.a() approach of eliminating the for loops, is a step forward, but might cause too much complexity of many very specific, highly tied methods.

  2. Separation-of-concerns might simplify things: also the above mentioned Excel reader might be developed in test-driven fashion in a unit test.

0

It depends.

The purpose of they DRY principal isn't so much to eliminate "identical looking" lines of code as it is to "facilitate consistency" and "reduce the burden of change."

Taking your example,

for(let i=0;i<SharedData.students.length;i++)

Under what circumstances would this line change? Are those circumstances "likely?" And, in the event of that change, would the identical or similar lines also change identically??

Use your judgement. Calibrate your own sensitivities over time.

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.