Strategy Pattern Implementation

Question

I have to generate a code that will send through SMS or Email to implement the One Time Password (OTP) requirement of our client.

I just finished creating the design using strategy pattern, . .

This it the first time I've used strategy pattern in real life projects so I would like to ask if I'm doing it right.

Below is just a sample code that will be used by client.

 //OTPGeneratorStrategy generatorStrategy = new EmailGeneratorStrategy("test@gmail.com"); for sending to Email for example
OTPGeneratorStrategy generatorStrategy = new SmsGeneratorStrategy(993454454545);
OTPGeneratorContextgeneratorContext = new OTPGeneratorContext(generatorStrategy);
context.generate("1-12345-0");

OTPValidatorStrategy validatorStrategy = new SmsValidatorStrategy();
OTPValidatorContext validatorContext = new OTPValidatorContext (validatorStrategy );
try {
validatorContext.validate("1-12345-0","e7241f");
} catch (InvalidVerificationKeyException ivk) {
System.out.println("Verification key is invalid!");
} catch(VerificationKeyExpiredException vke) {
System.out.println("Verification key is already expired, Please regenerate");
}

Updated:

Thank you for the feedback, actually I was not able to update my latest UML design and already separated the generation and validation.

And the reason why I've used Exception rather than enums because the project was built in java 1.4.

The code snipet could use some indentation. – Tulains Córdova Jan 08 '16 at 14:51 — Tulains Córdova, Jan 08 '16 at 14:51

Euphoric · Answer 1 · 2016-01-08T07:49:45.033

1

First thing that strikes me as a problem is using exceptions for validation result. Don't use exceptions for application logic. Just return an enum value. I would assume there are not many ways validation can end up.

Next thing I would question is using of two strategies. What would happen if you used SMS generator with Email validator? To me, it seems there should be just one strategy with both generate and validate methods. As for "merging the two violates SRP", I would actually say having them separate violates SRP. SRP is about cohesion. And cohesion works both ways. It separates non-cohesive code and joins cohesive code that was split. Ask yourself "If I changed how Email one-time-password policy worked, would I change only generator or validator or would I have to change both?". I'm quite sure you would need to change both, meaning generation and validation are cohesive parts. If you are worried about putting email sending into the strategy implementation, then that should be strategy's concern. OTPContext doesn't need to be bothered by that. It doesn't need to know generation uses some special logic, that is not relevant to validation.

Last nitpick is that I would prefer if strategy was set to OTPContext in a constructor instead of getters/setters. What would happen if you changed the validator strategy after code was generated? You should keep things read-only unless there is explicit requirement for things to change.

edited Jan 08 '16 at 07:49

answered Jan 08 '16 at 07:35

Euphoric

36,735
6
78
110

@DocBrown See edit. – Euphoric Jan 08 '16 at 07:49
@fuzzy28 What is reasoning behind separating generation and validation? – Euphoric Jan 08 '16 at 08:00
I've realized the setting of generation time and expiration time is no longer applied to validation that's why I've created a new separate context. – fuzzy28 Jan 08 '16 at 08:02
@Euphoric: sorry, but now I really disagree to what you wrote. That is IMHO a misconception of the SRP. I told you the better alternative is using abstract factory, don't know why you did not consider that as a solution which fulfills both requirements. – Doc Brown Jan 08 '16 at 08:03
@Doc Brown, How can I achieve it using abstract factory? – fuzzy28 Jan 08 '16 at 08:07
@fuzzy28: See https://en.wikipedia.org/wiki/Abstract_factory_pattern, replace `AbstractFactory` by `OTPContextPartsFactory` with subclasses `SMSContextPartsFactory`and `EmailContextPartsFactory`, and methods `CreateGenerator` and `CreateValidator`. The `AbstractProduct` hirearchies directly maps to your generator/validator hierarchies. Your OTPContext then gets a `OTPContextPartsFactory` passed through the constructor. – Doc Brown Jan 08 '16 at 08:11
@Doc Brown: May I know why and how did you come up with that solution? so I can better understand it. – fuzzy28 Jan 08 '16 at 08:13
@fuzzy28: the `why' is obvious: exactly the reason Euphoric gave: not to mix up "SMS generators" with "Email validators", but also not to mix up generator and validator logic in one class (that is the part Euphoric missed in his answer). How? From knowing the pattern and understanding that it is exactly made for providing a solution to the problem you presented, – Doc Brown Jan 08 '16 at 08:18
@DocBrown Its not misconception. Its how it is defined. See : https://drive.google.com/file/d/0ByOwmqah_nuGNHEtcU5OekdDMkk/view?pli=1 , Figure 8-3. Its by Uncle Bob, the guy who coined the SOLID principles. – Euphoric Jan 08 '16 at 08:18
@DocBrown Please see my updated UML, I've updated it a while ago which separates the concern of validation and generation. – fuzzy28 Jan 08 '16 at 08:22
@fuzzy28: tell that Euphoric, he seems to miss that validation and generation are separated concerns. If you need the abstract factory depends on if you want an `OTPValidatorContext` object initialized with an Email validator whilst at the same time an `OTPGenerationContext` contains an SMS generator object. If it is ok in your application to mix it up that way, you do not need the factory, and you should definitely not mix up validation and generation in one class. – Doc Brown Jan 08 '16 at 08:28
@DocBrown So you mean to say my updated UML seems okay? – fuzzy28 Jan 08 '16 at 08:31
@fuzzy28: first, you did not answer my implicit question. Do you want to combine "SMS generators" with "Email validators", or do you want to prevent that combination? Second, if your first or second design is "better" depends on the the requirements for those classes inside your application. Such a design is not an end in itself, it has to fulfill a purpose. We do not know the "outer context" and intended usage of those classes, that is something only you know. So you have to decide what fits best to that. – Doc Brown Jan 08 '16 at 08:40
@Doc Brown sorry, I actually realized that I have to separate it because some fields are not applicable to ValidatorContext. And I have to force the client to initialize the Context with strategy using Constructor Injection and I can't do that if it's combined. – fuzzy28 Jan 08 '16 at 08:46
@fuzzy28: you still did not answer my question. – Doc Brown Jan 08 '16 at 08:50
@Doc Brown What do you mean by "Combine SMS Generators"? Do you mean "Combine SMS Strategies"? – fuzzy28 Jan 08 '16 at 08:53
@fuzzy28: does it make sense in your application to have "SMSGeneratorStrategy" object in your `OTPGeneratorContext`, whilst at the same time there is an `EmailValidatorStrategy` object in you `OTPValidatorContext`? If that combination makes sense, you do not need a mechanism for making sure the types SMS/EMail are not intermixed. – Doc Brown Jan 08 '16 at 09:24
Sorry I did not understand please elaborate. – fuzzy28 Jan 08 '16 at 09:54

score 0 · Answer 2 · answered Jan 09 '16 at 07:32

As a former, but now somewhat reformed object-oriented programming enthusiast I have to say - be careful with design patterns. This feels like classic "kingdom of nouns" OOP overdesign (check out Steve Yegge's classic blog post on the subject: http://steve-yegge.blogspot.com/2006/03/execution-in-kingdom-of-nouns.html ).

I'd highly recommend trying a simple, procedural implementation of this, in addition to what you're considering - e.g. a simple static method that takes boolean or enum flag for SMS/email, in addition to the other inputs, and follows a simple linear flow of logic, using plain ol' if statements for steps that differ between SMS/email etc.

You may find that the strategy pattern leads to the simplest, most compact, easiest to maintain code in the end, but it definitely pays to try a few design approaches. The more code I've written and read over the years the more I've seen how OOP "best practices" can needlessly overcomplicate things.

Strategy Pattern Implementation

2 Answers2