views:

200

answers:

4

I've written the following if-statement in Java:

if(methodName.equals("set" + this.name) ||
    isBoolean() ? methodName.equals("is" + this.name) :
                  methodName.equals("get" + this.name)) {
    ...
}

Is this a good practice to write such expressions in if, to separate state from condition? And can this expression be simplified?

+8  A: 

I would change it to

if (methodName.equals("set" + this.name)
 || methodName.equals( (isBoolean() ? "is" : "get") + this.name)) {
    ...
}
SLaks
And I would drop the redundant `this.`'s
Software Monkey
@Software Monkey: that's my code-style, I'm sorry if it bothers you.
Pindatjuh
@Pindatjuh: Fair enough - I said "I would", not "you should". To each their own. It doesn't bother me, it's just extra typing.
Software Monkey
+2  A: 

Is it good practice? It's good if it makes it easier to read. It makes it easier to read if (1) it does and (2) the sort of person who'd be confused by it won't be reading it. Who's going to read it?

Vuntic
+2  A: 

Wouldn't something like the following work?

if (methodName.equals("set" + this.name)
    || methodName.equals("get" + this.name)
    || (isBoolean() && methodName.equals("is" + this.name))) {
    ...
}

It's more readable than the way in which you used the ternary operator and certainly easier to understand. It also has the advantage that it can avoid an unnecessary method call to the isBoolean method (it has either 1, 2 or 4 method calls whereas yours always has either 1 or 3; the performance gain/loss is probably too minute to notice).

Also there's a similar question here titled "Is this a reasonable use of the ternary operator?" One user had the following to say:

The ternary operator is meant to return a value.

IMO, it should not mutate state, and the return value should be used.

In the other case, use if statements. If statements are meant to execute code blocs.

Do note that I included parentheses around the expression containing '&&' for readability. They aren't necessary because x && y is evaluated before m || n.

Whether you choose to use it is up to you, but I tend to avoid it in favour of readability.

Dustin
Your rewrite is very practical, as the method "getSomeBooleanProperty" will also pass. Though, it's not as readable as SLaks's (subjective).
Pindatjuh
+2  A: 

I would be inclined to change it to

if (methodName.equals(setterForThis())
   || methodName.equals(getterForThis())) {
    ...
}

with some functions extracted:

private String setterForThis() {
   return "set" + this.name;
}

private String getterForThis() {
   return (isBoolean() ? "is" : "get") + this.name;
}

It's longer of course, but I'm not really into golf anyway.

Don Roby