Instead of trying to create a filter that includes everything, try just omitting the filter


The question was sent to a peer-to-peer discussion group for an internal program, let's call it Program Q. I'll add additional context so you can follow along.

Hi, I'm trying to build a query that finds all issues owned by a particular user, regardless of which product the issue belongs to. I know that I can query for specific products by saying

q select -owner bob -product LitWare
q select -owner bob -product Contoso

Is there a better way to do this than just running the query for every product in the database? It would be great to find all the issues at one shot instead of having to issue dozens of commands and combine the results.

The person who submitted this question got so distracted by the -product filter, that they forgot that they could just omit the filter.

q select -owner bob

If you don't filter by product, then it finds everything regardless of the product.

Enumerating all the products, then repeating the query for each product is some sort of anti-pattern. I don't know if it has a name, so I'll make one up. The long division anti-pattern performs an operation on a collection by arbitrarily breaking the collection into groups, then performing the operation on each member of each group, all this even though the grouping is unrelated to the operation.

In C#, it would be phrased something like this:

public void MakeUnavailable(string productId)
{
    var stores = Inventory.Select(p => p.Store).Distinct();

    foreach (var store in stores) {
        foreach (var product in
                 from p in Inventory
                 where p.Store == store &&
                       p.ProductId == productId) {
            product.Available = false;
        }
    }
}

In words, we first dig through the inventory and collect all the unique stores. For each store, we go through the inventory again, looking for products from that store, and if the product is the one we want to make unavailable, set the Available property to false. (To avoid playing favorites, I used both fluent and query expression syntax.)

Assuming the order in which the product is made unavailable is not important (and it doesn't appear to be, since we didn't sort the stores), the grouping by store is superfluous. You can just iterate across the entire inventory without regard for store:

public void MakeUnavailable(string productId)
{
    foreach (var product in
             from p in Inventory
             where p.ProductId == productId
             select p) {
        product.Available = false;
    }
}
Comments (12)
  1. Trevel says:

    I have to admit that there are occasions where I've done something like that — but only because the algorithm that I was told to run on the data was so poorly optimized (O(n^3) or so) that breaking it into related chunks was much faster than trying to do it all in one go. (Thank goodness I'm no longer working there.)

    I suspect the person in the first problem was attempting Copy and Paste Coding while not having a clue what the code actually did. I'm sure it's the first time you've ever seen anyone try that before.

  2. George says:

    Not to be pedantic, but the two solutions aren't the same — the second one will mark products available in which "Store" is null, the first won't.

    Just sayin'

  3. George says:

    Okay, to be pedantic on myself, I meant "mark products unavailable". Whoops :)

  4. JM says:

    The number one legitimate reason to do this is, as Trevel indicated, to have a natural splitting function for operations that take too much continuous time if performed in one go. You wouldn't expect to see this if no I/O is involved unless the algorithm is poor, but if I/O is involved (and especially if transaction logging a la SQL is involved) working in batches can be a better approach than doing everything at once.

    Even then this is obviously a poor man's batching since you don't know how big the batches will be (what if there are 100,000 LitWare products and only 10 Contoso products?) but it can be simpler than implementing a generic mechanism that splits things in constant-sized batches.

  5. Samir Talwar says:

    This seems like a hand-rolled implementation of the "n + 1" problem. Which is impressive. Usually those just show up when using an ORM or similar.

  6. John Gardner says:

    There's a similar version of this I've seen many times, where given a Map / Dictionary, the person iterates over every entry, checking for equality to find a value given a key, instead of just using dictionary[key] or TryGetValue(key), or .get(x) or whatever that language has.

    something like:

    object value = null;

    foreach( var entry in dictionary.Entries )

    {

       if (entry.Key == thing)

       {

          value = entry.value;

          break;

       }

    }

    [Ah, the for/if anti-pattern. -Raymond]
  7. Anonymous Coward says:

    It's like the For/Case anti-pattern where all the cases do the same thing.

  8. Joshua says:

    [Ah, the for/if anti-pattern. -Raymond]

    Way too easy to do in Linq. The obvious way of writing an if (.TryGetValue()) in Linq compiles to something like this.

  9. Luke says:

    As someone else said, this is the "n + 1" problem. Can be caused by people ignorant of how ORM's work, frequently caused by the repository pattern as well.

  10. Neil says:

    Not totally unrelated, but I once had a SQL statement that ran very slowly when the filter used a GROUP BY.

    So I ended up querying the filter to give me a static list of containers to use in the original query.

  11. Danny says:

    To play the Devil's advocate I'll say that usually a query on itself is pretty damn useless, most of the times is followed by a report. And if the report wants also the store as one of the columns you'll have to do it that way (or "group by" the store).

  12. Dave B says:

    Long division by 1: 57216 ÷ 1: 5 divided by 1 is 5, 5 – 5 leaves zero, bring down the 7, 7 divided by 1 is 7, remainder zero, bring down the 2… (much later) … 6 – 6 = 0, 57216 remainder zero!

     57216 r0

     _____

    1)57216

    -5

      7

     -7

     —

       2

      -2

      —

        1

       -1

       —

         6

        -6

        ==

    Actually caught my daughter doing just that to divide a polynomial by x.

Comments are closed.

Skip to main content