Is it bad to perform two different tasks in the same loop?

Question

I'm working on a highly-specialized search engine for my database. When the user submits a search request, the engine splits the search terms into an array and loops through. Inside the loop, each search term is examined against several possible scenarios to determine what it could mean. When a search term matches a scenario, a WHERE condition is added to the SQL query. Some terms can have multiple meanings, and in those cases the engine builds a list of suggestions to help the user to narrow the results.

Aside: In case anyone is interested to know, ambigous terms are refined by prefixing them with a keyword. For example, 1954 could be a year or a serial number. The engine will suggest both of these scenarios to the user and modify the search term to either year:1954 or serial:1954.

Building the SQL query and the refine suggestions in the same loop feels somehow wrong to me, but to separate them would add more overhead because I would have to loop through the same array twice and test all the same scenarios twice. What is the better course of action?

Answer 1

I don't think it's wrong to make two actions in one loop. I'd even suggest to make two methods that are called from inside the loop, like:

for (...) {
   refineSuggestions(..)
   buildQuery();
}

On the other hand, O(n) = O(2n)

So don't worry too much - it isn't such a performance sin.

Answer 2

I'd probably factor out the two actions into their own functions. Then you'd have

foreach (term in terms) {
    doThing1();
    doThing2();
}

which is nice and clean.

Answer 3

No. It's not bad. I would think looping twice would be more confusing.

Arguably some of the tasks might be put into functions if the tasks are decoupled enough from each other, however.

Answer 4

I don't think it makes sense to add multiple loops for the sake of theoretical purity, especially given that if you're going to add a loop against multiple scenarios you're going from an O(n) -> O(n*#scenarios). Another way to break this out without falling into the "God Method" trap would be to have a method that runs a single loop and returns an array of matches, and another that runs the search for each element in the match array.

Answer 5

Using the same loop seems as a valid optimization to me, try to keep the code of the two tasks independent so this optimization can be changed if necessary.

Answer 6

I'd call it a code smell, but not a very bad one. I would separate out the functionality inside the loop, putting one of the things first, and then after a blank line and/or comment the other one.

Answer 7

You could certainly run two loops.

If a lot of this is business logic, you could also create some kind of data structure in the first loop, and then use that to generate the SQL, something like

search_objects = []
loop through term in terms
   search_object = {}
   search_object.string = term
   // suggestion & rules code
   search_object.suggestion = suggestion
   search_object.rule = { 'contains', 'term' }
   search_objects.push(search_object)

loop through search_object in search_objects
   //generate SQL based on search_object.rule

This at least saves you from having to do if/then/elses in both loops, and I think it is a bit cleaner to move SQL code creation outside of the first loop.

Answer 8

I would look to it as if it were an instance of the observer pattern: each time you loop you raise an event, and as many observers as you want can subscribe to it. Of course it would be overkill to do it as the pattern but the similarities tell me that it is just fine to execute two or three or how many actions you want.

Answer 9

If the things you're doing in the loop are related, then fine. It probably makes sense to code "the stuff for each iteration" and then wrap it in a loop, since that;s probably how you think of it in your head.

Add a comment and if it gets too long, look at splitting it or using simple utility methods.

Answer 10

I think one could argue that this may not exactly be language-agnostic; it's also highly dependent on what you're trying to accomplish. If you're putting multiple tasks in a loop in such a way that they cannot be easily parallelized by the compiler for a parallel environment, then it is definitely a code smell.

Answer 11

Your scenario fits the builder pattern and if each operation is fairly complex then it would serve you well to break things up a bit. This is waaaaaay over engineering if all your logic fits in 50 lines of code, but if you have dependencies to manage and complex logic, then you should be using a proven design pattern to achieve separation of concerns. It might look like this:

var relatedTermsBuilder = new RelatedTermsBuilder();
var whereClauseBuilder = new WhereClauseBuilder();

var compositeBuilder = new CompositeBuilder()
    .Add(relatedTermsBuilder)
    .Add(whereClauseBuilder);

var director = new SearchTermDirector(compositeBuilder);
director.Construct("the search phrase");

string[] related = relatedTermsBuilder.Result;

string whereClause = whereClauseBuilder.Result;

The supporting objects would look like:

public interface ISearchTermBuilder {
    void Build(string term);
}

public class SearchTermParser {
    private readonly ISearchTermBuilder builder;

    public SearchTermParser(ISearchTermBuilder builder) {
        this.builder = builder;
    }

    public void Construct(string phrase) {
        foreach (var term in Parse(phrase)) {
            builder.Build(term);
        }
    }

    private static IEnumerable<string> Parse(string phrase) {
        throw new NotImplementedException();
    }
}