7

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, . Please see screenshot for the UML design.

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. See this

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 8 '16 at 14:51
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.

  • @DocBrown See edit. – Euphoric Jan 8 '16 at 7:49
  • @fuzzy28 What is reasoning behind separating generation and validation? – Euphoric Jan 8 '16 at 8: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 8 '16 at 8: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 8 '16 at 8:03
  • @Doc Brown, How can I achieve it using abstract factory? – fuzzy28 Jan 8 '16 at 8:07
0

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.