Refactor SLA problem of high level code inside if/else?

Question

I have some code where I believe the SLA (single level of abstraction) principle is broken in a way that the low-level code (if-statements) is outside and the high level code is nested inside the cases. A simplified version of the code is:

public void GenerateReport(int effort, bool delayed)
{
    if (effort < 5)
    {
        GenerateSimpleReport();
    }
    else if (effort < 20)
    {
        GenerateNormalReport();
    }
    else if (delayed)
    {
        GenerateDangerReport();
    }
    else
    {
        GenerateAwesomeReport();
    }
}

I would like to have the individual conditions for the different kind of reports at the abstraction level of those different report types, not at the abstraction level of the caller. I still don't want to move the condition tests into the Generate...Report() methods directly.

Which approach (maybe a design pattern) would be helpful here to refactor this code so that the SLA principle would be obeyed? If available, I'd like to use a well-known, documented pattern, but I am also open for other ideas.

Regarding the suggested duplicate: If I apply the Specification pattern, the code probably looks like

if (simpleSpecification.IsSatisfiedBy(effort, delayed))
{ 
    GenerateSimpleReport(); 
}
if (normalSpecification.IsSatisfiedBy(effort, delayed))
{ 
    GenerateNormalReport(); 
}
if (dangerSpecification.IsSatisfiedBy(effort, delayed))
{ 
    GenerateDangerReport(); 
}
if (awesomeSpecification.IsSatisfiedBy(effort, delayed))
{ 
    GenerateAwesomeReport(); 
}

and I don't see how this solves the problem, the conditions are still separated from the GenerateXxx() methods.

Answer 1

The first code with if-else looks fine to me. The logic is clearly expressed, which means the code is easy to understand and maintain.

If-statments are not "low level code". They operate at the abstraction level of the conditionals, in this case the abstraction level of "effort" and "delayed", which is the parameters which define which report is appropriate. So it seems to me everything is on the same abstraction level.

A SLA violation would be if the same code also (say) performed the actual generation of the reports into html or pdf. These are concerns at a different abstraction level than selecting the correct report.

Moving the condition checks into separate function or classes would just complicate the code and make it more bug prone. Because the conditions are not actually independent - they depend on whether a previous report condition matched or not. For example the condition for DangerReport is that delayed is true and that effort < 20. Should the DagerReport condition check for effort < 20 or should it rely on the prioritized order of checking the report conditions? In any case the logic determining which report to generate is now spread in multiple different places.

For example the final else provide the guarantee that a report is always generated. None of the more complex solution suggested actually provide this basic guarantee. But this is easy to miss because the increased complexity makes the logic harder to follow.

With the single if-else you have high cohesion - the logic for selecting the correct report in one single place, easy to read, understand and modify. By breaking it into separate functions give you low cohesion and high coupling, since the conditions are still logically interdependent. This makes the code hard to follow and easy to introduce bugs.

(If the conditions actually were independent, i.e. each report decided individually if it should be generated, regardless of whether other reports are generated, then it would make sense to separate the conditions. But this would mean you could get zero or multiple independent reports, not a single report as in the original code.)

It is easy to miss the forest for trees when reading about principles and patterns. In the end it comes down to if the code is easy to understand and easy to modify when things change. This seem to be the case for your code. Turning it into a "abstract factory" or whatever will in no way improve the readability or maintainability.

Answer 2

Reading your own answer, I think the code still violates another important principle: KISS.

The most simple and straightforward approach to move the conditions to a level of abstraction of their own would be to refactor the conditions into separate functions:

public void GenerateReport(int effort, bool delayed)
{
   bool done=false;
   done = done && GenerateSimpleReportIfApplicable(effort);
   done = done && GenerateNormalReportIfApplicable(effort);
   done = done && GenerateDangerReportIfApplicable(delayed);
   done = done && GenerateAwesomeReportIfApplicable();
}

bool GenerateSimpleReportIfApplicable(int effort)
{
    if (effort < 5)
    {
        GenerateSimpleReport();
        return true;
    }
    return false;
}

bool GenerateNormalReportIfApplicable(int effort)
{
    if (effort<20)
    {
        GenerateNormalReport();
        return true;
    }
    return false;
}
// ...

Now, this gives you several opportunities for further refactorings:

• making the IfApplicable functions all conform to the same signature, so you can put them all into an array of functions, where GenerateReportcan loop over them. This may be worth the effort if there are several more of them, not just four.

• remove the boolean return value from the IfApplicable functions, rework the conditions, allowing to generate a full report from several sub-reports, if you can modularize the reports that way (as scetched in your own answer).

• move the IfApplicable functions into different or separate classes, may into a different library, so someone else can maintain them in parallel, without touching the main GenerateReport function any more. This makes sense if you want to make it easier for several people to work at different reports in parallel.

• making those different classes all conform to the same interface, and maybe end up with your idea of a Chain-Of-Responsibility. This makes most sense if you want to create an extensible report generation framework.

But beware: this additional complexity always comes for a cost which must be justified by a reason. The SRP, or the SLA (at least how you interpreted it), and other principles and patterns are not an end in themselves. The guideline when to apply which should be YAGNI: only use the above refactorings and flexibilizations when you notice the current code forces you to maintain things which belong together in different places, violates DRY too much, is not testable, or does not fulfill other non-functional requirements. Otherwise, do yourself a favor and keep the code as simple as it was originally.

Answer 3

I will consider the comments saying that the code is fine.

However, in my particular case, I think I found a combination of the Chain of Responsibility pattern along with a Template Method makes the code conforming better to my requirement.

The affected method becomes nicely SLA without any low-level ifs:

public void GenerateReport(int effort, bool delayed)
{
    var report = new DangerReport()
        .SetNext(new AwesomeReport())
        .SetNext(new NormalReport())
        .SetNext(new SimpleReport());
    report.GenerateReport(effort, delayed);
}

and I have a base class allowing to chain the different reports (Append()) and do the work (HandleRequest()). Each condition goes into the IsApplicable() method and it's impossible to bypass the checks due to the template method pattern inside HandleRequest().

abstract class ChainableReport
{
    private ChainableReport successor;

    public ChainableReport SetNext(ChainableReport nextReport)
    {
        if (successor == null)
            successor = nextReport;
        else
            successor.SetNext(nextReport);
        return this;
    }

    public void GenerateReport(int effort, bool delayed)
    {
        if (IsApplicable(effort, delayed))
            GenerateReport();
        else if (successor != null)
            successor.GenerateReport(effort, delayed);
    }

    protected abstract bool IsApplicable(int effort, bool delayed);
    protected abstract void GenerateReport();
}

Yes, I certainly have introduced a higher coupling between the report and its condition. On the other hand side I am now able to change the order of the reports freely and let the most important one decide first whether or not it wants to handle the case or not. Note that I was able to put new DangerReport() in first position where it was in third position in the if-else cascade.