CODEWITHSUNDEEP
calgorithmloops
Deqing
Deqing

Reputation: 14652

Redundant code in a loop

I have a list:

id num1 ...
-----------
 1  123 ...
 1  456 ...
 2  789 ...
 2  666 ...

And would like to create an object array based on it:

{ 1, [123, 456], [...] },
{ 2, [789, 666], [...] }

Here is my pseudo code:

int previous_id = -1;
array a1 = null; // B
array a2 = null; // B
array a3 = null; // B
array a4 = null; // B
while (++c) { // c is a pointer pointing to the list
  if (c.id != previous_id && previous_id != -1) {
    j[i++].id = previous_id;   // A
    j[i++].data1 = a1;         // A
    j[i++].data2 = a2;         // A
    j[i++].data3 = a3;         // A
    j[i++].data4 = a4;         // A
    a1 = null;   // B
    a2 = null;   // B
    a3 = null;   // B
    a4 = null;   // B
  }
  a1.add(c.num1);
  a2.add(c.num2);
  a3.add(c.num3);
  a4.add(c.num4);
  previous_id = c.id;
}
j[i++].id = previous_id;   // A
j[i++].data1 = a1;         // A
j[i++].data2 = a2;         // A
j[i++].data3 = a3;         // A
j[i++].data4 = a4;         // A

It is working but with some redundant code, i.e. A and B

Is it possible to merge them to make it more concise and clear?

Upvotes: 0

Views: 182

Answers (3)

Cyan
Cyan

Reputation: 13968

There are a couple of things you could do to improve clarity of your code, if that's the objective.

Rather than using different names, use indexes.

"array" is not defined, so I guess it's a table of something. You could have something like :

  #define NB_ARRAYS 4
  array myArrays[NB_ARRAYS];

This way, you will be able to loop through your arrays, and eventually change the number of arrays anytime in the future.

The good think is that you no longer need to list separately your arrays, a single loop is enough. Hence :

    a1 = null;   // B
    a2 = null;   // B
    a3 = null;   // B
    a4 = null;   // B

becomes

    { int i; for (i=0; i<NB_ARRAYS; i++) myArrays[i] = NULL; }

You may complain that this is no better than the first version, but you can actually hide this complexity behind a macro :

    #define INIT_ARRAYS(a) { int i; for (i=0; i<NB_ARRAYS; i++) a[i] = NULL; }

or even better, behind an inline function :

    static inline void initArrays(array* a) { int i; for (i=0; i<NB_ARRAYS; i++) a[i] = NULL; }

So it becomes :

    initArrays(myArrays);

which is much clearer.

Reusing the same principles, it would lead your code to something like this :

    int previous_id = -1;
    initArrays(myArrays);

    while (++c) 
    { // c is a pointer pointing to the list
       addToArrays(myArrays, c);
       if (c.id != previous_id && previous_id != -1 || c.islast() ) 
       {
           j[i++].id = previous_id;   // A
           setArrays(j, i, myArrays); i+=NB_ARRAYS;
           if (!c.islast()) { initArrays(myArrays); }
       }
    }
    previous_id = c.id;

which should be easier to read, and therefore easier to maintain.

Also : try to use easier to read variables. In this example, I don't know what c, i or j stand for (nor where they are defined). It doesn't cost much to use 5-6 characters to properly name them, and really help code maintainance.

Upvotes: 1

Deqing
Deqing

Reputation: 14652

Just found a way to merge A and avoid necessarily resetting B:

int previous_id = -1;
array a1 = null; // B
array a2 = null; // B
array a3 = null; // B
array a4 = null; // B
while (++c) { // c is a pointer pointing to the list
  a1.add(c.num1);
  a2.add(c.num2);
  a3.add(c.num3);
  a4.add(c.num4);
  if (c.id != previous_id && previous_id != -1
      || c.islast() ) {
    j[i++].id = previous_id;   // A
    j[i++].data1 = a1;         // A
    j[i++].data2 = a2;         // A
    j[i++].data3 = a3;         // A
    j[i++].data4 = a4;         // A
    if (!c.islast()) {
        a1 = null;   // B
        a2 = null;   // B
        a3 = null;   // B
        a4 = null;   // B
    }
  }
  previous_id = c.id;
}

Upvotes: 0

Bernhard Barker
Bernhard Barker

Reputation: 55629

Your code is a bit too pseudo-code-y for my liking - suggestions are very dependent on what your actual implementation looks like. However, I'll give some ideas.

For the A part:

struct's in C will allow you to do something like:

someStruct temp = {previous_id, a1, a2, a3, a4};
j[i++] = temp;

For the B part:

I assume null actually refers to an empty array, rather than a null array, and you possibly have a fixed-length array with a separate length indicator for the populated length.

You can always have an array of lengths, and a 2D main array instead, and memset the entire length array to 0 rather than setting them one by one.

Upvotes: 0

Related Questions