How can I avoid duplicate use of doSomethingCommon()
in the following block of code?
doSomething();
if (conditionA) {
doSomethingSpecificToConditionA();
doSomethingCommon();
}
else if (conditionB) {
doSomethingSpecificToConditionB();
doSomethingCommon();
}
doSomethingElse();
NB: Calculating conditionA and conditionB can be expensive.
6 Answers 6
Calling doSomethingCommon()
from 2 conditional blocks should be fine, and technically NOT code duplication, as only one of the conditional blocks will be executed at any given time.
However, if doSomethingCommon()
is not already a separate method, you should refactor it into a method and again calling the same is NOT code duplication.
-
3If your first assumption was true, then the problem would be trivial. OP would have figured it out himself. But he did not. The problem, as it is presented, implies it is not possible!Aphton– Aphton07/29/2018 07:32:52Commented Jul 29, 2018 at 7:32
-
1@Aphton Hence the second part of the answer. Also, as a general rule, assumptions, especially about the cost of boolean checks, should always be avoided.nikhil jhaveri– nikhil jhaveri07/29/2018 07:36:00Commented Jul 29, 2018 at 7:36
-
1Well, you put it into the else-branch too. So, that's different code.Deduplicator– Deduplicator07/30/2018 18:44:20Commented Jul 30, 2018 at 18:44
Introduce a special condition handler function:
doSomething();
handleSpecialConditions();
doSomethingElse();
...
void handleSpecialConditions()
{
if (conditionA)
doSomethingSpecificToConditionA();
else if (conditionB)
doSomethingSpecificToConditionB();
else
return;
doSomethingCommon();
}
-
1That's the best of the bunch yet.Deduplicator– Deduplicator07/30/2018 18:47:42Commented Jul 30, 2018 at 18:47
-
1That may be so but it's still not very good.Stack Exchange Broke The Law– Stack Exchange Broke The Law07/31/2018 04:50:27Commented Jul 31, 2018 at 4:50
Consider using inheritance to avoid multiple conditionals in a method
public class SomethingA : Something
{
public override void DoSomething()
{
base.DoSomething() //all the common things
this.SpecialForASomethings();
}
}
-
-
hmmm.. that seems super bad to me. but we do have limited data to go off, why dont you write a full answer?Ewan– Ewan07/29/2018 10:47:31Commented Jul 29, 2018 at 10:47
-
1The solution I proposed circumvents the problem of you accidentally forgetting to call the
base.DoSomething()
method, because it's called by the template. Because of that I like to put the necessary calls in the parent class and let the children only really define the specialisations. When it comes to the full answer, I don't feel like answering this question because the question is too broad and you'd need much more information to find the right solution. Perhaps the template pattern would be overkill in this situation, other time the if/else if is the wrong path. It depends.Andy– Andy07/29/2018 14:23:35Commented Jul 29, 2018 at 14:23 -
1This answer assumes that
doSomethingCommon()
can reasonably be abstracted into a base class where A/B can represent its subtypes. As there is no inheritance present in the example, not even a mention of objects altogether, suggesting inheritance seems like a very big leap to make. I'm not saying you're wrong, I'm just saying that your answer only applies to a small subset of possible scenarios that OP's example could be taking place in.Flater– Flater07/30/2018 11:08:00Commented Jul 30, 2018 at 11:08 -
@Flater it's such a generic example I don't think you can make assumptions either way. Q: 'how do I avoid conditionals?', A: 'use inheritance', Q: 'how do i avoid conditionals that have a special problem which means inheritance doesn't work?', A: '98% of the time, refactor and use inheritance'Ewan– Ewan07/30/2018 11:14:10Commented Jul 30, 2018 at 11:14
If you don't want to extract handleSpecialConditions()
like Eternal21 suggested (you probably should), consider memoizing:
doSomething();
bool flagA;
if (((flagA = conditionA)) || conditionB) {
if (flagA)
doSomethingSpecificToConditionA();
else
doSomethingSpecificToConditionB();
doSomethingCommon();
}
doSomethingElse();
-
2This is less readable than the original code.Stack Exchange Broke The Law– Stack Exchange Broke The Law07/31/2018 04:50:46Commented Jul 31, 2018 at 4:50
-
@immibis: If
doSomethingSpecific....
is just that one call of a well-named function, sure. It could be more complicated though, defying easy naming, and being very involved with the rest of the function. The simplified example-code gives no clues either way. Sure, that's unlikely, but it's better to know the option.Deduplicator– Deduplicator07/31/2018 12:18:47Commented Jul 31, 2018 at 12:18
You should also consider other best practices like Prefer Composition over inheritance. So my suggestion would look like this:
class CommonBehavior{
void doSomethingCommon(){
//...
}
}
interface SpecialBehavoior{
void doSomethingSpecial();
}
class SpecialBehavior1 implements SpecialBehavoior{
@Inject
private CommonBehavior commonBehavior;
@Override
public void doSomethingSpecial(){
// what ever
commonBehavior.doSomethingCommon();
// what ever more
}
}
class SpecialBehavior2 implements SpecialBehavoior{
@Inject
private CommonBehavior commonBehavior;
@Override
public void doSomethingSpecial(){
// what ever
commonBehavior.doSomethingCommon();
// what ever more
}
}
This unleashes its full power when combined with the Tell, don't ask! principle. That means that you convert your "conditions" as soon as possible to SpecialBehavior
objects (usually in a Factory class that still uses if
s and/or switch
es) and pass them around.
my question with this is that the calling code needs to know to call the special behaviour vs the common behaviour? – Ewa
This is the "smart" thing about inheritance and tell, don't ask!: the calling code does not need to know. The objects passed have this knowledge.
The calling code "splits up". You create the SpecialBehavior
objects as soon as the data enters your system and pass them around to places where methods of a common API are called and any special behavior is coded in the different implementations of that interface.
The code implies that some cases are neither special A or B, how are those dealt with? – Ewan
Doing "nothing" is also a "special behavior" so you need an implementation of the SpecialBehavior
interface for that:
class DoNothing implements SpecialBehavoior{
@Override
public void doSomethingSpecial(){
// do nothing
}
}
This looks like a lot of "glue code" but thanksfully Java has an eye candy named lambda so that you can equally write it like this:
SpecialBehavoior doNothing = ()->{;};
ok, but then shouldnt it be doSomethingCommon which calls doSomethingSpecial? – Ewan
Both ways are possible and it depends on your requirements.
As far as I understood your "special behavior" is mutual exclusive. In this case it is better to have a single instance of the CommonBehavior
class passed to the individual instances of SpecialBehavior
implementations. Especially the "DoNothing" requirement is easy to solve this way.
If you wold do it the other way around you would have two down sides:
You need a separate instance of
CommonBehavior
as "wrapper" for each and everySpecialBehavior
instance.There is no Tell, don't ask! way to prevent the "common behavior" from being executed.
This means you need something like
if(!specialBehavior instance of DoNothing){ // do the common behavior }
and that is exactly the kind of code we initially tried to avoid.
-
my question with this is that the calling code needs to know to call the special behaviour vs the common behaviour? The code implies that some cases are neither special A or B, how are those dealt with?Ewan– Ewan07/29/2018 14:15:11Commented Jul 29, 2018 at 14:15
-
ok, but then shouldnt it be doSomethingCommon which calls doSomethingSpecial?Ewan– Ewan07/29/2018 16:30:58Commented Jul 29, 2018 at 16:30
-
@Ewan please see my next updateTimothy Truckle– Timothy Truckle07/29/2018 17:00:37Commented Jul 29, 2018 at 17:00
-
maybe im just not following. Your saying that everything implements specialBehaviour, including a class which only runs the common behaviour? I dont see how this stops you forgetting to call commomBehaviour in one of these classes. Its simply a replacement of inheritance with composition and the interface should simply be IDoSomthing and make no reference to special behaviour?Ewan– Ewan07/29/2018 17:06:48Commented Jul 29, 2018 at 17:06
-
1This is clearly far more confusing and unreadable than the original code.17 of 26– 17 of 2607/30/2018 15:46:19Commented Jul 30, 2018 at 15:46
Boolean checks are not expensive:
doSomething()
if (conditionA) {
doSomethingSpecificToConditionA();
}
else if (conditionB) {
doSomethingSpecificToConditionB();
}
if (conditionA || conditionB) {
doSomethingCommon();
}
doSomethingElse()
-
1
conditionA
andconditionB
could be functions which return a boolean. In that case evaluating the condition could be expensive.combinatorics– combinatorics07/29/2018 08:43:22Commented Jul 29, 2018 at 8:43 -
1... and non-idempotent.Jörg W Mittag– Jörg W Mittag07/29/2018 09:22:22Commented Jul 29, 2018 at 9:22
-
8avoiding the expense of calculating conditionX can always be done by storing the result, no matter how many times you check itEwan– Ewan07/29/2018 09:28:50Commented Jul 29, 2018 at 9:28
-
1You code is less readable as the OPs example,Timothy Truckle– Timothy Truckle07/29/2018 11:29:21Commented Jul 29, 2018 at 11:29
-
4This removes the duplicate mention of doSomethingCommon(), but at the cost of duplicating the mentions of conditionA and conditionBbdsl– bdsl07/29/2018 11:34:29Commented Jul 29, 2018 at 11:34
Explore related questions
See similar questions with these tags.
if
statement is not code duplication. The whole point of an "extract method" refactoring is to eliminate code duplication, so multiple calls to a method don't count.