Reputation: 47
I know that you can remove if blocks by using polymorphism with Strategy or Command patterns. However, let's say my code looks something like this:
ArgumentObject arg = getArg();
if (arg.getId() == "id2_1" || arg.getId() == "id3_1" || arg.getId() == "id4_1") {
//do something
}
if (arg.getId() == "id2_1") {
//do something
}
if (arg.getId() = "id2_2" || arg.getId() = "id3_1") {
//do something
}
if (arg.getId().startsWith("id1") || arg.getId().startsWith("id4")) {
//do something
}
Basically, the code performs some fields population actions based on product's id. It checks whether product comes from some group of products with startsWith(), then performs population, checks if a product has a particular id, then populates fields appropriately, etc.
I am not sure how can I use Strategy pattern here? If I create one Strategy to handle first if statement, then how can I proceed handling the second statement? The problem is that the ifs aren't mutually exclusive. Should I just move the relevant ifs to Strategy classes? Any pattern that would help me refactor this so its easier to test?
Upvotes: 0
Views: 272
Reputation: 41223
You may not be able to get rid of your if
based code entirely, but you may be able to tidy it up into a more appropriate class.
The if
s are domain logic, and if it makes sense within the domain then you could write an object solely responsible for this classification:
interface ArgumentClassifier {
Classification classify(Argument arg);
}
Classification
would be an enum
.
The implementation of this will contain your if
s, encapsulating that domain logic.
ArgumentClassifier
in isolation. ArgumentClassifier
turns out to be large and complicated, you might well be able to break it up using the Composite Pattern.ArgumentClassifier
.Now you can use the Strategy pattern:
Map<Classification, Handler> strategies = ...;
ArgumentClassifier classifier = new SimpleArgumentClassifier();
private Foo handleArg(Argument arg) {
return strategies
.get(classifier.classify(arg))
.handle(arg);
}
Whether all this is worthwhile or not, depends on your specific needs.
If:
... then it may be wiser to leave the if
logic where it is.
Another approach to classification is to build a binary number or a string from predicates about the object being classified. For example:
int classification(Person p) {
return 0
| ( p.getGender() == Gender.MALE ? 1 : 0 )
| ( p.isEmployed() ? 2 : 0 )
| ( p.age >= 18 ? 4 : 0 );
}
This gives you a classification of 0b001
for an unemployed 17-year-old male, 0b010
for an employed 17-year-old female, and so on.
You can use these directly as keys to a map of strategies (it could be many-to-one). Or you can do further bitwise manipulation to reduce them to a smaller set.
There's a lot of scope here, but take care not to lose sight of simplicity and clarity. Sometimes a chain of if/else
gets the job done, is clear and effective.
Upvotes: 0
Reputation: 325
Your question lacks the business logic for these ifs. But I can make some assumptions from :
Basically, the code performs some fields population actions based on product's id. It checks whether product comes from some group of products with startsWith(), then performs population, checks if a product has a particular id, then populates fields appropriately, etc.
I am assuming all you if blocks are specific business logic and are independent of each other. You can try Decorator Pattern for this. As the wiki says:
Decorator Pattern is a design pattern that allows behavior to be added to an individual object, either statically or dynamically, without affecting the behavior of other objects from the same class
Your case can be viewed as :
public Product{
private String id;
//other fields
}
ProductDecoratorInterface{
Product populateItem(Product product);
}
ProductXYZDecorator{
@Override
public Product populateItem(Product product){
// check the id for 1,2,3
//do the field population
}
}
ProductABCDecorator{
@Override
public Product populateItem(Product product){
// check the id for 1,3
//do the field population
}
}
Your main class would look like:
public Mainclass{
List<ProductDecoratorInterface> decorators;
public void someFn(Product product){
decorators.stream.forEach(i -> i.populateItem(product));
}
}
}
...
Any pattern that would help me refactor this so its easier to test?
Here you would need to test just the individual Decorators.
You would need to maintain the list of all decorator and pass your object through all of them. This is not the proper use of the pattern but helps solve your use case(testing complexity).
Upvotes: 0
Reputation: 15622
The problem your code reveals is somewhat more basic: It violates the Tell! Don't ask! principle.
Instead of having a method getId()
your arg
objects should have a common interface method which is called at this point. The work to be done could be injected to the arg
objects when they are instantiated, or they could be passed as Parameters to the method and the concrete Inmplementation of the arg
object decides which parameter to call...
Upvotes: 1