patterncsharpModerate
Executing functions and declaring flags with a long battery of if conditions
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:
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
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?
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:
Note that if
If it is the case that you only need to execute
While refactoring, I'd also recommend choosing some better variable names than
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.