Strategy pattern or Chain of Responsibility pattern for determining right action

Question

I have the following UML diagram. CurlRequestHandler and KernelRequestHandler are both an implementation of the RequestHandlerInterface. The request handlers are responsible to process a certain Request object, all of them will return the same Response object.

   +------------------------+               +-------------------------+
   | CurlRequestHandler     |               | KernelRequestHandler    |
   |------------------------|               |-------------------------|
   |                        |               |                         |
   | - handleRequest(Request)               | - handleRequest(Request)|
   |                        |               |                         |
   |                        |               |                         |
   |                        |               |                         |
   +------------------------+               +-------------------------+
             +
             |                                             +
             |                                             |
             |        +---------------------------+        |
             |        | RequestHandlerInterface   |        |              +---------------+
             +---->   |---------------------------|  <-----+              |               |
                      |                           |                       |               |
                      | - handleRequest(Request)  |                       |    CLIENT     |
                      |                           |                       |               |
                      |                           |                       +---------------+
                      |                           |
                      |                           |
                      +---------------------------+

Now, to determine which handler I need to use, I have the following if statement in my client:

if ($mode == "kernel") {
    $handler = new KernelRequestHandler();
} else {
    $handler = new CurlRequestHandler();
}

$response = $handler->handleRequest($request);

Now, the problem is, when I need to add a new handler, I need to alter the if statement. I looked into the Chain of Responsibility design pattern and this seems to do a better job at this, but I'm not sure.

Which design pattern would be the best approach for this?

Steffen

Seems to me like you want to overuse design patterns. There's nothing worse, imo, than unnecessarily complicated code. Creating an instance dynammically using a string as class name would allow you to instanciate whatever that string contains without altering the i/else stuff, assuming that the class exists. `$className = 'implementation'; $instance = new $className();` — Virus721, Dec 03 '13 at 09:13
You just need simple [factory pattern](http://stackoverflow.com/questions/2079902/factory-abstract-factory-and-factory-method) — V G, Dec 03 '13 at 09:13
Oh come on, since when adding a new if became a problem? However, you can simply do a `if (class_exists($className = ucfirst($mode).'RequestHandler', false)) { $handler = new $className(); } else { $handler = new DefaultRequestHandler(); }` and it will avoid the multiple usages of ifs. — Twisted1919, Dec 03 '13 at 09:13
Because, I like to follow the open-close principle. When another developer is working on the application I created, it would be better for both if they could extend rather than modify. — Steffen Brem, Dec 03 '13 at 13:40

Pierre Arlaud · Accepted Answer · 2014-08-28T07:28:52.643

4

What you need is to implement a factory method design pattern to create the handlers.

class HandlerFactory {

    public function make($mode) {

        switch(strtolower($mode)) {

            case 'kernel': return new KernelRequestHandler();
            case 'curl': return new CurlRequestHandler();
        }

    }

}

And, yes, you need to add a case for every new handler you make.

PS: Why you shouldn't call your classes 'handler'

edited Aug 28 '14 at 07:28

answered Dec 03 '13 at 09:13

Pierre Arlaud

4,040
3
28
42

Altering a switch is the same problem as altering an if else – Virus721 Dec 03 '13 at 09:15
@Virus721: I never say it wasn't. I just thought it would be elegant. And I DID point out that there is no avoiding adding a new case for every new "handler". – Pierre Arlaud Dec 03 '13 at 09:18
2

@Virus721 Yes - The difference being that it's encapsulated within the factory – AlexP Dec 03 '13 at 09:18
Yes but your are altering one switch, and have no repeated code everywhere. – Aurimas Ličkus Dec 03 '13 at 09:18
@AlexP True, it's okay to alter the switch statement inside the factory, because that's what it is for :) – Steffen Brem Dec 03 '13 at 09:26
@ArlaudPierre I've read your recommendation about not calling classes with 'handler' or 'object'. It clearly makes sense, thanks! – Steffen Brem Dec 03 '13 at 09:32
1

@SteffenBrem: I'm glad you liked the discussion, naming may sometimes be even more important for code evolution than using the right Design Pattern. – Pierre Arlaud Dec 03 '13 at 09:33

score 1 · Answer 2 · answered Dec 03 '13 at 09:15

1

If your $mode equals to the beginning of your RequestHandler class, then you can just append it.

$mode = 'kernel';
$class = ucfirst($mode).'RequestHandler';
$handler = new $class;

Will produce new KernelRequestHandler

answered Dec 03 '13 at 09:15

Royal Bg

6,988
1
18
24

and a few bugs in the long run. Okay maybe not. The short run will do. – Pierre Arlaud Dec 03 '13 at 09:27
This is not a very good approach in my opinion. But thanks for sharing your ideas on it! – Steffen Brem Dec 03 '13 at 09:28
@ArlaudPierre what bugs ? :) – Royal Bg Dec 03 '13 at 09:29
1

@RoyalBg For instance I advised in my answer to change the name of the classes because "Handler" is about as generic a suffix as "Class". So you change it, and poof, it no longer works. This approach is dangerous, and besides the answer of V G following the RequestFactory (between mine and this answer) encapsulates the creation of the entities in one class, preventing the duplication of code and all the problems that it implies. – Pierre Arlaud Dec 03 '13 at 09:32
@ArlaudPierre my answer is how to create it dynamically. Where to put it, to not replicate the code depends on the project structure. According to me it's the same if he uses `switch` or `if`. He never said, he uses the `many if's` replicated on every page. The OP's problem was that everytime he adds new class, he needs to go back to these conditions and add a new one. Tbh, `V.G`'s answer seems to be the full - with the pattern and the dynamic build. Why do you consider dynamic build of classname a dangerous one? – Royal Bg Dec 03 '13 at 09:36
@RoyalBg: I agree than in a way your answer is closer to what was really asked. But the implied question was "Is it okay if I have to add an 'if' block every time I add a class", to which the answer is "yes it is". Gaining a few lines by doing the build of classname is not worth it and makes the code harder to read. What if you arrive to the project at this point and have no idea what all the different handlers are? Or where they were declared? By listing them in a switch, you get at least a small hint. – Pierre Arlaud Dec 03 '13 at 09:40
@ArlaudPierre Altering code for further use does not worth self-explaining. You change whole functionality this way. Just because someone will not understand where handlers came from, you add cases? Wouldn't it be better to add a small comment before the Method declaration to make reference where $handler is, and what types they could be? Tbh, I'm not arguing, just asking. Because of my last experience, I really started to hate when everything needs to be altered in order to work with new things – Royal Bg Dec 03 '13 at 09:48
@RoyalBg Code is going to change. No matter how proud you are about it. And the whole point of design patterns is to prevent this evolution from being a huge pain. No one tells you that all the handlers may be defined in the same file or even in the same folder. I would advise against using class names as string because there is no way to know it doesn't work until the code is called and renaming is a very usual thing (to make the code more readable and thus evolutive). – Pierre Arlaud Dec 03 '13 at 09:55
@ArlaudPierre there should be conventions. Yes, it might be bad to name your classes `Handler` but if there are already 100 handlers named that way, you should not name your new handler different way, not just because you will break the Dynamic instantiation, but just as mature programmer, to not leave your steps in the code. It should seem the way it was left, unless refactoring come. If you do not refactor existent code, just don't break the conventions from your old co-workers. Isn't that an unwritten rule? As conventions I mean not only naming, but where to put handlers aswell. – Royal Bg Dec 03 '13 at 10:02
@RoyalBg What if I create a KernelRequestHandler2? How will your code work? I'd agree it's a stupid example but one would but wonder. You may call it KernelAlternateRequestHandler, and then you have to use $mode='kernelAlternate' because $mode='kernelalternate' will not work… There are just so many things that can go wrong with this. – Pierre Arlaud Dec 03 '13 at 10:08
@ArlaudPierre You can also name it `Kernel2RequestHandler`. The same way you can create MVC where convention to name your Models and Controllers is that the classes should be name respectively `UserModel` and `UserController`, so, yes, you will break the application if you name your class `UserModel2`, but won't break it if you name it `User2Model`. I always look at the existent application as a framework with solid rules. You should just obey the rules in the application. Giving all the liberty to a co-worker will result in having everyone's habits as scars in the code, wouldn't it? – Royal Bg Dec 03 '13 at 10:12
@RoyalBg it would make the name unclear. This is a second requester for the kernel, not a requester for the second kernel. And they should be called Requester and not RequestHandler - there are tools today to change names in a clean way, so why not do it (after sending a mail to your team asking them what they think of the new clearer name)? I'd say the rule is simple: either your trust your co-worker with doing what's best for the code, or you don't. I'd personally avoid working with someone who writes unmaintainable (may it only be unclear/unreadable) code. – Pierre Arlaud Dec 03 '13 at 10:16
@ArlaudPierre so you can choose who you would work with, in a current assignment? The most of the situations I've seen, if you are not the one who recruit the newcommmers, you result with co-workers that you cannot tell your manager - I don't want to work with them. And you need to put them the frames, which they cannot break. – Royal Bg Dec 03 '13 at 10:25
@RoyalBg Putting the frames should be with an order if you're a manager, not by technical means. Enough of these offices with IE6 because the company's proxy doesn't work on other browsers. You should force them to use the same name conventions, yes, but not by putting technical constraints like creating constrained-names dynamically. – Pierre Arlaud Dec 03 '13 at 10:32
1

@ArlaudPierre this seems like the perfect way, I hope I will be in a project someday where with an order everyone uses the same conventions :) Thanks for the discussion – Royal Bg Dec 03 '13 at 10:48
@RoyalBg Just for the record, and to end this discussion, the hungarian notations were originally created and used in the MS Office Word project team. Read the excellent article from Joel about this : http://www.joelonsoftware.com/articles/Wrong.html – Pierre Arlaud Dec 03 '13 at 10:51

V G · Answer 3 · 2013-12-03T09:54:51.263

0

class RequestFactory {
    public static function getHandler($mode) {
        $className = ucfirst($mode).'RequestHandler';
        return new $className();
    }
}

You can use it like this:

$handler = RequestFactory::getHandler('kernel');
$handler->handleRequest($request);

edited Dec 03 '13 at 09:54

answered Dec 03 '13 at 09:18

V G

1,225
9
13

Why the static method? To create a static contest only to avoid factory creation? – Pierre Arlaud Dec 03 '13 at 09:38
Imho, doing so is breaking OOP. You can't extend your factory properly. It's not a factory because it doesn't build anything (it's not an object). What I mean by that is that it's equivalent as having a getHandler() global method. Then why bother putting it inside a class? Programming in an imperative style is not a bad thing, but I tend to believe that mixing it with OOP is error-prone. In fact I think this is the biggest complain many people had about C++. – Pierre Arlaud Dec 03 '13 at 09:58
@ArlaudPierre, Not agree with you. This code is can be extensible with Singleton for example. – V G Dec 04 '13 at 15:02
Yes, exactly, your solution is as bad as singletons https://sites.google.com/site/steveyegge2/singleton-considered-stupid – Pierre Arlaud Dec 04 '13 at 15:03
@ArlaudPierre, can you please provide some code example, for better explanation why my method returning new instances is so bad? What I can't do with him? – V G Dec 04 '13 at 15:44
It's not "bad", it just doesn't follow object-oriented logic. Singleton and static methods are an anti-pattern, you will find plenty resources about that topic on your favourite search engine. The question was about design-patterns and best OOP practice; therefore providing an answer with an anti-pattern seems a bit off. – Pierre Arlaud Dec 04 '13 at 16:09

Strategy pattern or Chain of Responsibility pattern for determining right action

3 Answers3