Executing functions and declaring flags with a long battery of if conditions

Question

I'm only asking because almost the entire code base I've inherited here, in C# and PHP, looks like this:

if (varOne == flag) {
    run_some_sql();
    set_some_flag = true;
}

if (varTwo == flag) {
    run_some_sql();
    set_some_flag = true;
}

if (varThree == flag) {
    run_some_sql();
    set_some_flag = true;
}

if (varFour == flag) {
    run_some_sql();
    set_some_flag = true;
}

if (varFive == flag) {
    run_some_sql();
    set_some_flag = true;
}

if (varSix == flag) {
    run_some_sql();
    set_some_flag = true;
}

if (varSeven == flag) {
    run_some_sql();
    set_some_flag = true;
}

if (varEight == flag) {
    run_some_sql();
    set_some_flag = true;
}

For about ten thousand lines.

I haven't lost my mind, right? Is this bad? There's only about five loops in the entire code base of about 50,000 lines.

I would normally go about this sort of thing by storing the data into an associated array, and loop over that, rather than adding a new if statement for every variable.

Have I totally lost my mind? Eight people worked on this before me, and I'm seriously worried here. Is there ever a good reason to do this sort of thing?

Answer 1

This is bad. Very bad.

It appears the original implementer doesn't quite understand arrays. You should definitely refactor this. Michael K's answer has the right approach. In C# syntax:

// Hopefully these variables are already in an array or list already, otherwise:
MyType[] myVariables = new MyType[] { varOne, varTwo, /* ... */ varEight };

foreach (MyType variable in myVariables)
{
    if (variable == flag)
    {
        run_some_sql();
        set_some_flag = true;
    }
}

Note that if run_some_sql() only needs to be executed once, you can break out early and save some computation time:

foreach (MyType variable in myVariables)
{
    if (variable == flag)
    {
        run_some_sql();
        set_some_flag = true;

        break;
    }
}

If it is the case that you only need to execute run_some_sql() once, you can make the code even simpler using new LINQ syntax:

if (myVariables.Any(v => v == flag))
{
    run_some_sql();
    set_some_flag = true;
}

While refactoring, I'd also recommend choosing some better variable names than varOne, varTwo, flag, etc. Though I assume these are just your examples. :)

Answer 2

No, you haven't lost your mind. It should more like...

// declared and set somewhere
flags = new int[x];

//...

for (int value: flags) {
   if (value == flag) {
      run_some_sql();
      set_some_flag = true;
   }
}

...assuming that the flags are the same type and they really do do the exact same thing. Are you sure that there are no god objects somewhere that the other methods are basing their responses of off?

Answer 3

Another possible approach is to store the SQL statements in a dictionary and to use the flag as key

Dictionary<TypeOfFlag, string> dict = new Dictionary<TypeOfFlag, string>();
dict.Add(valueOne, "SELECT x FROM y");
dict.Add(valueTwo, "SELECT z FROM t");
...

string sql;
if (dict.TryGetValue(flag, out sql)) {
    RunSQL(sql);
    set_some_flag();
}

If you need more information in order to run a query, you could create a class containing this information, the SQL statement, and possibly even an Execute method and store that one in the dictionary. See: Command pattern (Wikipedia).

Dictionaries have a constant access time O(1) and require no loops in order to access an item and of course require no endless if statements!

---

Endless if-chains and switch-statements (Case-statements in other languages) are often a strong hint that a poor procedural approach has been used. Applying the DRY principle can even improve such procedural approaches. Polymorphism (Subtyping) is an object-oriented approach that can often eliminate these unpleasant structures as well.