2
votes
\$\begingroup\$

I'm looking at some code that goes a bit like this:

class Foo { public int Frob; public int Frib; }
class Bar { public Foo Left; public Foo Right; }

public void ProcessBars(Bar[] bars)
{
    this.LeftFrobs = new int[bars.Count];
    this.RightFrobs = new int[bars.Count];
    this.LeftFribs = new int[bars.Count];
    this.RightFribs = new int[bars.Count];
    //... you get the idea

    int counter = 0;
    foreach(Bar bar in bars)
    {
        LeftFrobs[counter] = bar.Left.Frob;
        LeftFribs[counter] = bar.Left.Frib;
        //...27 more lines of similar
        if(bar.Left.Frob == MAGICNUM_A) LeftFribs[counter] = MAGICNUM_B;

        RightFrobs[counter] = bar.Right.Frob;
        RightFribs[counter] = bar.Right.Frib;
        //...27 more lines of similar
        if(bar.Right.Frob == MAGICNUM_C) LeftFribs[counter] = MAGICNUM_D;

        counter++;
    }
}

Aesthetically, this bothers me, but so far, I haven't managed to come up with anything better.

\$\endgroup\$

locked by Jamal Aug 9 '14 at 23:42

This question exists because it has historical significance, but it is not considered a good, on-topic question for this site so please do not use it as evidence that you can ask similar questions here. This question and its answers are frozen and cannot be changed. See the help center for guidance on writing a good question.

Read more about locked posts here.

  • 2
    \$\begingroup\$ I don't understand, why do you have 27 more lines like that? Do you mean Foo and Bar have more properties? How exactly do you use the arrays after you create them? I mean, why do you need an array for each combination? \$\endgroup\$ – svick Sep 19 '12 at 12:40
  • \$\begingroup\$ @svick, yes, more properties. You're right about usage. I think it's to dump them as columns into Excel (via a wrapper which likes columns), but I haven't got that far yet. I'm just trying to refactor this bit into something manageable. Currently looking at reducing to two properties Left and Right of Data { Frobs, Fribs }... \$\endgroup\$ – Benjol Sep 19 '12 at 12:49
  • 1
    \$\begingroup\$ I don't think doing it that way would work well. If you have 60 fields, then the best way to set them is probably to have a line of code for each fields. On the other hand, having 60 fields is probably not the way to do this. But to know how to fix that, you will need to know how are they used. \$\endgroup\$ – svick Sep 19 '12 at 13:01
3
votes
\$\begingroup\$

That method is immense, and is surely impossible to easily read and maintain. Once you break it into smaller methods you will be able to work with it ("just like those acorns, one at a time").
This will also help avoid repetitions of code.

I propose creating a class to hold all that data, and merely instantiate a Left and a Right objects of that class. Then move the loading of data into the class as well.

Then the foreach is limited to 3 lines plus all the ifs:

public class WildGooseChaser
{
    class Foo { public int Frob; public int Frib; }
    class Bar { public Foo Left; public Foo Right; }

    private InternalData Left;
    private InternalData Right;

    class InternalData
    {
        public int[] Frobs;
        public int[] Fribs;
        // ... 27 more lines of similar.

        public InternalData(int dataLength) {
            Frobs = new int[dataLength];
            Fribs = new int[dataLength];
            //... you get the idea
        }

        public void LoadBar(int index, Bar bar) {
            Frobs[index] = bar.Left.Frob;
            Fribs[index] = bar.Left.Frib;
            //...27 more lines of similar
        }
    }

    public void ProcessBars(Bar[] bars) {
        Left = new InternalData(bars.Length);
        Right = new InternalData(bars.Length);

        int counter = 0;
        foreach (Bar bar in bars) {
            Left.LoadBar(counter, bar);
            Right.LoadBar(counter, bar);

            if (bar.Left.Frob == MAGICNUM_A) {
                Left.Fribs[counter] = MAGICNUM_B;
            }

            if (bar.Right.Frob == MAGICNUM_C) {
                Left.Fribs[counter] = MAGICNUM_D;
            }

            counter++;
        }
    }
}

If you receive an array, you can use for instead of foreach.
So you can either get rid of the counter variable, or receive an IEnumerable<Bar> instead.

\$\endgroup\$
  • \$\begingroup\$ Very nice. I was already part of the way there, but hadn't got as far as pulling the common logic into the data class. \$\endgroup\$ – Benjol Sep 19 '12 at 13:10
  • \$\begingroup\$ It's actually a list, but I'm always in two minds about for when I also need to access the objects: for i with added bar = bars[i], or foreach with index++, or even Select((bar, index) =>... \$\endgroup\$ – Benjol Sep 19 '12 at 13:15
  • \$\begingroup\$ Sorry, that's three minds :) \$\endgroup\$ – Benjol Sep 19 '12 at 13:23

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