HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

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

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
executingwithbatterylongdeclaringconditionsandfunctionsflags

Problem

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?

Solution

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

Code Snippets

// 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;
    }
}
foreach (MyType variable in myVariables)
{
    if (variable == flag)
    {
        run_some_sql();
        set_some_flag = true;

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

Context

StackExchange Code Review Q#1720, answer score: 12

Revisions (0)

No revisions yet.