Q:

How to refactor this legacy code snippet to make it extensible?

I've come across a method resembling the below snippet.
public void process(Data row) {

    Value value1 = row.getValue1();
    Value value2 = row.getValue2();
    boolean saved = false;

    if (value1 != null) {

        if (relevantValues.contains(value1)) {
            addRow(value1, row, only1collection);

            if (relevantValues.contains(value2)) {
                addRow(value2, row, values12collection);
            }

            repository.save(row);
            stats.increment();
            saved = true;
        }

    }

    if (relevantValues.contains(value2)) {
        addRow(value2, row, only2collection);
        if (!saved) {
            repository.save(row);
            stats.increment();
        }
    }

}

private void addRow(Value date, Data row, Map<Value, List<Data>> mapping) {
    mapping.computeIfAbsent(date, (p) -> new ArrayList());
    mapping.computeIfPresent(date, (d, rows) -> {
        rows.add(row);
        return rows;
    });
}

In the above code, the method process will be called for each row of some data file and based on the value in a couple of fields, the row should be added to any or all of the 3 collections.
The code,
        repository.save(row);
        stats.increment();

seems like I'm coupling two independent functionalities/aspects into kind of "lifecycle"/business logic of row processing. I can not straight up use decorator pattern here as these two steps need to be performed conditionally and only once per row. Moreover, if I want to extend the functionality, for example, send an email after incrementing stats, the code will look like,
        repository.save(row);
        stats.increment();
        sendEmail(row);

This clearly violates the Open-Closed principle. I'm thinking whether it'd be a good idea to take a Set<RowProcessor> and execute them when the row is interesting. What would be a good way or a pattern to organize this?
The nested if conditions are not looking good. Any general principles, patterns to make it more functional?
How can I eliminate the if(!saved) check?
Any other suggestions, improvements are more than welcome. If you know any book that gives practical guidance for refactoring/cleaning code in scenarios like this, please suggest.

A:

I personally don't consider the code to be that bad.
First thing I consider is that it is quite feasible to write automated tests for it. You should definitely do it before you start any refactoring. Having tests would allow you to experiment around getting rid of the saved flag. Or you can realize it is not as bad as it seems. I would consider following issues to be more pressing than the flag.
Next thing that was quite confusing at first notice is the addRow method. You should at least make the mapping parameter to be first one. If this was C#, it would make sense to make it into extension method. In case of Java creating your own structure, that encapsulates the Map and exposes the addRow method would be best option.
And your problem with extensibility around repository can be solved simply. Just make repository an interface. Then, you can use Composite pattern to implement multiple different ways how rows are "stored" and Decorator pattern for statistics. It will also make it easier to test, as it can be mocked/faked.
Last note about OCP. OCP should be applied strategically. Making code open against changes makes it more complex. You should be careful about adding complexity where it is not needed. In your case, you have identified one area where making your code more open will make things easier, while not adding that much complexity. But you should be careful about creating new abstractions you only think might be needed.

A:

Before worrying about extensibility, what I'd do is put this thing under unit test to be sure I'm not making any unintended changes and rework it until this arrow anti pattern goes away.
public void process(Data row) {

    Value value1 = row.getValue1();
    Value value2 = row.getValue2();  

    Boolean relevant1 = ( (value1 != null) && relevantValues.contains(value1) );
    Boolean relevant2 = ( relevantValues.contains(value2) );

    if (relevant1) {
        addRow(value1, row, only1collection);
    }

    if (relevant1 && relevant2) {
        addRow(value2, row, values12collection); 
    }   

    if (relevant2) {
        addRow(value2, row, only2collection); 
    }

    if (relevant1 || relevant2) {
        store.update(row);
    }
}

That's a tad more readable. Since the original code, thankfully, had no temporal coupling between its lines they could each be wrapped in whatever conditionals pertained to them and rearranged at will. Here's the history of simple changes that got me here: 
1. Put each addRow() in its own structure
2. Collapse to multiline boolean logic and group similar sections together
3. Turn "saved" bool into an "else if"
4. Turn "else if" into an OR
5. Simplify conditionals with boolean vars 
Now you may be looking at this and wondering "what is store?" It's whatever you'd like it to be. 
public interface Store {
    public void update(Row row);
}

public StoreDefault implements Store {
    public void update(Row row);
        repository.save(row);
        stats.increment();
    }
}

public StoreReport implements Store {
    public void update(Row row);
        repository.save(row);
        stats.increment();
        email.send(row);
    }
}

I've used The Strategy Pattern but there are many other patterns (and non-patterns) that could work as well. Since changes in how process() concludes can be made by creating new store types, this respects OCP just fine.  The important thing is that process() doesn't have to know what was used. That not-knowing is what lets the polymorphism work. And polymorphism is the heart and soul of Object Oriented Programming.

