9
\$\begingroup\$

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?

\$\endgroup\$
  • 3
    \$\begingroup\$ Well, at least you are sure you will do better than the previous employees. ;p \$\endgroup\$ – Steven Jeuris Apr 7 '11 at 22:42
  • \$\begingroup\$ wtf!? oh my god... o.O \$\endgroup\$ – greatwolf Apr 8 '11 at 3:29
  • 1
    \$\begingroup\$ Reminds me of the SO question get rid of ugly if statements \$\endgroup\$ – Sean Patrick Floyd Apr 8 '11 at 9:20
  • 2
    \$\begingroup\$ you're not crazy... although, i wonder if SLOC stats are management's measure for employees' productivity/contribution within the project ;) \$\endgroup\$ – justin Apr 9 '11 at 7:19
  • \$\begingroup\$ You don't know thedailywtf.com , right? \$\endgroup\$ – Landei Sep 15 '11 at 8:41
12
\$\begingroup\$

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. :)

\$\endgroup\$
  • \$\begingroup\$ Well, what I'm dealing with is a group of variables that are clearly all related, but not stored as an array. varOne, varTwo is an accurate generic representation of this. \$\endgroup\$ – Incognito Apr 8 '11 at 2:57
  • \$\begingroup\$ @user2905 Is there a logical place where you can put them into an array or List, such as during initialization? If they are all related, I imagine this won't be the only function you'll find that has copy-paste code like this. \$\endgroup\$ – Scott Wegner Apr 8 '11 at 15:50
9
\$\begingroup\$

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?

\$\endgroup\$
5
\$\begingroup\$

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.

\$\endgroup\$

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.