How to avoid code duplication in if else condition?
https://softwareengineering.stackexchange.com/questions/376057
-
07-02-2021 - |
Domanda
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.
Soluzione
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.
Altri suggerimenti
Introduce a special condition handler function:
doSomething();
handleSpecialConditions();
doSomethingElse();
...
void handleSpecialConditions()
{
if (conditionA)
doSomethingSpecificToConditionA();
else if (conditionB)
doSomethingSpecificToConditionB();
else
return;
doSomethingCommon();
}
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();
}
}
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();
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.
Boolean checks are not expensive:
doSomething()
if (conditionA) {
doSomethingSpecificToConditionA();
}
else if (conditionB) {
doSomethingSpecificToConditionB();
}
if (conditionA || conditionB) {
doSomethingCommon();
}
doSomethingElse()