Reputation: 2174
There are two code code 1:
if(isApplicable() || isGood()) {
//something
}
private boolean isApplicable() {
}
private boolean isGood() {
}
Code 2:
boolean applicable = isApplicable();
boolean good = isGood();
if(applicable || good) {
//something
}
private boolean isApplicable() {
}
private boolean isGood() {
}
Which of the the approach is good java practice ? To me code1 seams more clean and code 2 seams to have extra code. code2 can make remote debugging easy.
Upvotes: 0
Views: 247
Reputation: 41203
To generalise your question, you're asking about the two forms:
// local variable form
Foo foo = methodReturningFoo();
Bar bar = methodTakingFoo(foo);
// inlined form
Bar bar = methodTakingFoo(methodReturningFoo());
Most modern IDEs have a shortcut to refactor between these at a keystroke: "inline" and "extract local variable". The fact that both refactorings exist is an indicator that both are appropriate, in different circumstances.
Inlining to a single statement makes the code more compact and sometimes more readable. You can see everything that's happening without having to read up to find out where a variable was set.
Here's a good candidate for inlining:
String name = customer.getName();
String greeting = createGreeting(name);
// ... becomes ...
String greeting = createGreeting(customer.getName());
Extracting a local variable turns what may be a long statement into two (or more) shorter statements. It may also allow you to re-use a value rather than calculate it twice.
Here's an example where we just break a statement into smaller chunks.
String greeting = createGreeting(greetingFactory.get(customer.getTitle()), customer.getName());
// ... becomes ...
Title title = customer.getTitle();
String name = customer.getName();
String greeting = createGreeting(greetingFactory.get(title), name));
... here's an example where we reuse a calculated value.
// doing the work twice
CustomerCategory category = findCategory(totalOrderValues(
customer.getOrders(currentMonth)));
List<Promotion> eligiblePromotions = findEligiblePromotions(totalOrderValues(
customer.getOrders(currentMonth)));
// ... becomes ...
BigInteger totalOrderValues = totalOrderValues(
customer.getOrders(currentMonth))
CustomerCategory category = findCategory(totalOrderValues);
List<Promotion> eligiblePromotions = findEligiblePromotions(totalOrderValues);
Generally, prefer the inlined version, until you see that the line is too long and complicated. Then extract a local variable (or extract a method) to make it neater. If it makes sense to store a value to avoid repeating an expensive calculation, then do so.
Upvotes: 1