I've got a method that kind follows the following pattern:
void myMethod(String arg1) {
SomeObject foo = getSomeObject(arg1);
if(foo != null) {
SomeOtherObject bar = foo.getSomeOtherObject();
if(bar != null) {
bar.doSomething();
if(bar.isGood()) {
YetAnother baz = getAnotherByName(bar.getAnotherName());
if(baz != null) {
if(baz.exitEarly()) {
foo.recordEarlyExit(baz.getName());
return;
}
}
}
}
}
doNormalThing();
}
I'm wondering if there's a cleaner way to get the same behavior without so many levels.
If I didn't have to do doNormalThing();
at the end, I could do something like this:
void myMethod(String arg1) {
SomeObject foo = getSomeObject(arg1);
if(foo == null) { return; }
SomeOtherObject bar = foo.getSomeOtherObject();
if(bar == null) { return; }
bar.doSomething();
if(!bar.isGood()) { return; }
YetAnother baz = getAnotherByName(bar.getAnotherName());
if(baz == null) { return; }
if(baz.exitEarly()) {
foo.recordEarlyExit(baz.getName());
}
}
I'm basically doing the above, but add doNormalThing();
prior to all the return
s. But that's a lot of repetition and if I need to change something, I have to do it in all those blocks.
I could do something like wrap it in a try{ ... } catch (DoNormalThingException e) { doNormalThing(); }
and only throw DoNormalThingException
when I want to call that method prior to returning from the method, and not throw it when baz.exitEarly()
is true. But that doesn't seem clean either, and seems like it's abusing exceptions.
If it was just checking for null
in all the conditionals, then I could wrap in a try
and only run doNormalThing()
when a NullPointerException
is thrown. It's a a lot cleaner than the other exception based way: I wouldn't need any conditionals, and I'm catching a legit exception, not a made up one just being used for control flow. But not all the checks are for == null
and I wouldn't want to mask NPEs coming from deeper method calls.
If only java had goto
...
Edit: So I knew this type of refactoring had a name, it's: Replace Nested Conditional with Guard Clauses. This is what I attempted to do, but it doesn't work well if you have that extra method that needs to be called in most cases but not all. Still, I thought I'd mention it to help others find this question.
One option would be to wrap all but the doNormalThing
method call in another method that returns whether or not to execute doNormalThing
afterwards:
void myMethod(String arg1) {
if (myMethodInternal(arg1)) {
doNormalThing();
}
}
private boolean myMethodInternal(String arg1) {
SomeObject foo = getSomeObject(arg1);
if (foo == null) {
return true;
}
SomeOtherObject bar = goo.getSomeOtherObject();
if (bar == null) {
return true;
}
// etc
if (baz.exitEarly()) {
foo.recordEarlyExit(baz.getName());
return false;
}
return true;
}
It doesn't feel terribly clean, but it would at least work. You could also use try/finally
without the exception - using a boolean
field to determine whether or not to execute doNormalThing
.
See more on this question at Stackoverflow