Do lots of operations on multiple objects of the same type

Question

I have two graphical objects (say some kind of Tables) and I want to set their styles. The trivial code is as follows:

table1.BorderWidth = 2;
table1.BorderColor = Color.GloriousPink;

table2.BorderWidth = 2;
table2.BorderColor = Color.GloriousPink;

(The real code has more lines.)

A more clever way is using a method.

void Format Table(int tableIndex)
{
    Table table;
    if(tableIndex == 1)
        table = table1;
    if(tableIndex == 2)
        table = table2;
    table.BorderWidth = 2;
    table.BorderColor = Color.GloriousPink;
}

I was thinking of a way to make it more scalable (the if/switch part grows fast), and I came up with:

foreach(Table table in new List<Table> { table1, table2 })
{
    table.BorderWidth = 2;
    table.BorderColor = Color.GloriousPink;
}

This is shorter and any potential additional tables are added quite simply. Is there any downside to it?

That's about it. Duplicate code and multiple `if` statements are a common code smell. — vgru, Mar 27 '13 at 14:20
Your second method could be a little cleverer. Instead of accepting an `int tableIndex` as parameter, you could take a `Table table` instead. `void FormatTable(Table table)`. This way you won't need an `if` and your code gets shorter and more easy to read too. — Nolonar, Mar 27 '13 at 14:25
You might also consider having a list as a private field of the parent class, containing all your tables (to avoid having to recreate the list whenever you want to apply operations to all tables). — vgru, Mar 27 '13 at 14:34

score 7 · Accepted Answer · edited Mar 27 '13 at 14:21

7

Nothing jumps out as being wrong, but I'd go with your original idea and actually put that in a method, just instead pass in the actual table.

public void Format(Table table)
{
    table.BorderWidth = 2;
    table.BorderColor = Color.GloriousPink;
}

foreach(Table table in tables) 
{
   Format(table);
}

edited Mar 27 '13 at 14:21

Sergey Kalinichenko

714,442
84
1,110
1,523

answered Mar 27 '13 at 14:20

Ian

33,605
26
118
198

Ilya Ivanov · Answer 2 · 2013-03-27T14:25:41.203

2

I don't know your requirements, but how about some functional style:

Action<Table> formatTable = (table) => {
    table.BorderWidth = 2;
    table.BorderColor = Color.GloriousPink;
};

new List<Table> { table1, table2 }.ForEach(formatTable);

If you don't like all these Action thing:

void FormatTable(Table table)
{
    table.BorderWidth = 2;
    table.BorderColor = Color.GloriousPink;
}

new List<Table>{ table1, table2 }.ForEach(FormatTable);

edited Mar 27 '13 at 14:25

answered Mar 27 '13 at 14:19

Ilya Ivanov

23,148
4
64
90

This might be an overkill for such a simple operation, unless the applied function needs to be interchanged during runtime. – vgru Mar 27 '13 at 14:22
Idea is that he can have member of a class `ApplyTable` and pass it to `ForEach` – Ilya Ivanov Mar 27 '13 at 14:23
He does say that it's an overly simple example and likely to grow... – Immortal Blue Mar 27 '13 at 14:26
that's why we try to overuse code as max as possible – Ilya Ivanov Mar 27 '13 at 14:27

score 2 · Answer 3 · answered Mar 27 '13 at 14:27

Let the compiler create the array:

void FormatTable(params Table[] tables)
{
    foreach(var table in tables)
    {
        table.BorderWidth = 2;
        table.BorderColor = Color.GloriousPink;
    }
}

and call it like so:

FormatTables( table1, table2);

Do lots of operations on multiple objects of the same type

3 Answers3