1

I need to refactor a class that became too messy. The class has a method that reads some raw data from one of our repositories, and another that "filters" it:

The data is in the form of level and on/off, for each X*Y of the units we have. The filtering process lets some of the data pass in the raw form, changes a small amount from on to off, or reduces the level, and some is just blocked in this stage.

And this stage there a already quite a lot of rules, but none of them is general enough to apply to all units, so we have a lot of if/else statements that are hard to follow and can cause bugs.

I'd be happy to redesign it in a more readable and easier-to-maintain form.

I can't provide a code snippet, but a pseudo code will look like this:

for x in X:
    for y in Y:
        if data[x] == ...
            if data[x][y] == ...
                copy as is
            else if
                manipulate data and copy
            else
               disable this data
        else if data[x] == ...

Is there a known design pattern for this scenario? I was looking at the filter pattern but I don't know if my criteria are unified enough

1
  • 2
    You have a structure for each item{ ruleA(item); ruleB(item) }. Sometimes it can be helpful to flip this around, with each rule or filter handling all items and potentially forwarding or disabling items for the next rule: items = ruleA(items); items = ruleB(items). This could make it easier to test rules in isolation and could make it possible to write individual rules more efficiently, though different rules might then have to duplicate some work. In particular, as CandiedOrange points out in an answer, some rules might not have to iterate over the entire X × Y cartesian product.
    – amon
    Oct 15, 2023 at 18:56

3 Answers 3

4

Here's a pattern for you:

If you don't need y yet then stay out of the y loop.

for x in X:
    if data[x] == ...
        for y in Y:
            if data[x][y] == ...
                copy as is
            else if
                manipulate data and copy
            else
               disable this data
    else if data[x] == ...

Note the 2nd and 3rd line of your pseudo code are transposed.

With this change the code looks much more like it could be decomposed into methods that take the x or x & y that you need. With that, you just have to come up with good names for them.

4

My immediate reaction would be to start by separating the iteration from the filtering:

for x in X:
    if data[x] == ...
        for y in Y:
            data[x][y] = filter1(data[x][y]);
    else if data[x] == ...


T filter1(T input):
    if input == ...
        return input;
    else if ...
         return manipulate(input)
    else 
         return disable(input)
3

The definition of filtering in the question is not pure "filtering", as the question states, it also mutates the data. It is indeed a sequence of "filter" and "map".

You can box each distinct unit of such business logic in a Rule:

interface Rule[T] {
  applicable(item T) boolean
  filter(item T) boolean
  map(item T) T
}

Now you can apply any number of such rules:

input = // input sequence of items
output = // output sequence of items
rules = // a set of rules to apply

for item in  input {
  included = true

  for rule in rules {
    if !rule.applicable(item) {
      continue
    }
    
    included = included && rule.filter(item)
    if included {
      item = rule.map(item)
    }
  }

  if included {
    output.add(item)
  }
}

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.