What is the best approach to organize utility/helper methods in controller?

Question

I have several controllers. In each controller class I defined the same set of helper methods:

private void info(String msg)
{
    ApexPages.addMessage(new ApexPages.Message(ApexPages.Severity.INFO, msg));
}

private void error(String msg)
{
    ApexPages.addMessage(new ApexPages.Message(ApexPages.Severity.ERROR, msg));
}

Credit: ConnectorController.cls

I try to follow coding best practices, thus I need to avoid code duplication. Now I am thinking how to do it right way. I am considering two options:

1. Create an abstract class and then inherit all controller classes from it. But I don't feel that's a correct approach since info/error methods bear utility nature. That said, I still wanted to know whether my thinking is correct.

2. Create a utility class and make those methods static. Then call those methods from the controller.

This question may be considered as too broad or opinion-based, but as long as best practices are concerned I don't believe many different views will pop up.

Answer 1

Personally, I wouldn't make any effort to avoid duplicating those single lines. If you write code with an IDE, then auto-complete and/or templates would get you those lines pretty quickly.

But, if you do want to wrap them up:

1. Yes, you're right, inheritance wouldn't be a great idea. Generally, it's best to favour composition over inheritance. There are lots of references for that, here's the first reasonable one I found on google: https://en.wikipedia.org/wiki/Composition_over_inheritance
2. If you're going to make a utility class, then it's probably best not to go with static methods. Static methods can't be overridden, and don't support interfaces

If I were to go full-on with OO stuff (probably too much, see Enterprise Hello World), then I would do it like this:

interface ApexPagesMessenger {
    void error(String msg);
    void info(String msg);
}

class DefaultApexPagesMessenger implements ApexPagesMessenger {
    public void info(String msg) {
        ApexPages.addMessage(new ApexPages.Message(ApexPages.Severity.INFO, msg));
    }

    public void error(String msg) {
        ApexPages.addMessage(new ApexPages.Message(ApexPages.Severity.ERROR, msg));
    }
}


class ApexPagesMessengerFactory {
    public static ApexPagesMessenger newInstance() {
        return new DefaultApexPagesMessenger();
    }
}

class MyController {
    private ApexPagesMessenger messenger = ApexPagesMessengerFactory.newInstance();
}

What does this buy you? The possibility of having a family of different implementations of ApexPagesMessenger, which could potentially be swapped in/out in different ways.

Edit

Of course, you could take a middle-ground approach to this and skip the factory i.e.

class MyController {
    private ApexPagesMessenger messenger = new DefaultApexPagesMessenger();
}

And then refactor later if it makes sense to.

Answer 2

Static Utility class, hands down. These are nice, contained methods, which helps reduce some of the verbosity of the ApexPages.AddMessage(new ApexPages.Severity.FATAL, '') into Messages.AddError('').

If you already had a parent class for your page extensions/controllers, I can see an argument being made to add it there (especially since it opens up the door for unique customizations for messages without changing, say, the save method).

You don't however, and this code is very simple - it just needs a home to live in between your different classes. Even if later you decided to use inheritance to create a structure of controllers, you can use the static methods as part of your controller calls, say as the default behavior.