Recently I saw pieces of code that could be simplified. I see such code often, however, recently I came across code that seemed to follow the same pattern
The patern was as in the following example:
private boolean someFlag;
...
public boolean isSomeFlagSet() {
if (someFlag) {
return true;
}
else {
return false;
}
}
Why not simply write:
public boolean isSomeFlagSet() {
return someFlag;
}
It is so much simpler!
Similarly to boolean flags, there where occurencies of other object types being used and methods invoked on them in the following manner:
public String getCity() {
if (address == null) {
return null;
}
else {
return address.getCity();
}
}
This can be also simplified (applying "change if-else to ?:" refactoring) to:
public String getCity() {
return address == null ? null : address.getCity();
}
Please strive for simplicity in your code (K.I.S.S. principle). Verbosity clutters the code and can hide the meaning or intention of it.
17 comments:
Or even better, public boolean someFlag; and get rid of the getter.
You could do the same for address - make address public and then the caller is responsible for the null test. It seems that the caller might prefer to have an address to operate on instead anyway (IDEA code inspection; feature envy).
I see what you are saying, Ricky.
But wouldn't that be breaking of the encapsulation of your object? Not to mention that you would lose potential benefits of inheritance, where you would not be able to modify the behavior of the subclass (as those methods would not be present).
I still prefer to have control over what is happening with my (well encapsulated) object's state.
If you have 'simple' getters and setters, then there's no encapsulation to break.
I don't currently do any subclassing, because I find it harms designs, so I don't consider the effects on subclasses.
If you find that you need to, at some point, apply more interesting logic to the field then you can use your IDE to change the code. The only time this will cause problems is when the code is part of a public API.
In a better language than Java, one would be able to change a simple field into a less simple one without invalidating clients.
I agree with you.
My example was from classes that comprise the manager or service classes providing a public access to objects shielded by them - as such forming a public API to this application.
Cheers,
:Dushan
not sure conditional ternary statements really simplify things.
boolean amIEasyToRead() {
return true ? youLoveIt() : imBored() > phar;
}
I think one could comment that:
public String getCity() {
return address == null ? null : address.getCity();
}
is less simple than your if/then logic.
My reasoning behind this is that to newer developers the above syntax can be confusing (if you had never seen it before), and no matter what language you come from, or your level, you will understand the if/then logic.
As a comment to using getters/setters vs. just making the variables public, I always go with the getters/setters. This way if I have to add some additional logic at a later date, I can just go in and add it without having to track down everywhere that I might be using that value. I find that it simplifies life (especially since creating the methods is a mindless activity).
Another point, if you find yourself using flags for processing, take some time to see if the code can be refactored to not use flags.
Admittedly, there are cases for flags, just make sure they are really needed.
Usually they aren't, and after refactoring to remove, code is often much cleaner and easier to read.
Reply to Becky Reamy:
Good point! Conditional ternary statements can look more complicated to an untrained eye.
However, I prefer them to if-else statements if they do not extend 80 or 120 char line. If I can express something in one reasonably simple line, why say the same thing in 6-8 lines (depending on your formatting style)?
Moreover, I just realized that my example wasn't quite right.
The code I saw was totally redundant and it looked like this:
public Object getObject() {
if (object == null) {
return null;
}
else {
return object;
}
}
This is just stupid and could be simplified to:
public Object getObject() {
return object;
}
The ternary operator example shown by 'anonymous' is clearly bogus because the false part can never be reached.
return address==null ? null : address.getCity();
is quite easy to read. I don't accept that you shouldn't use the ternary operator just because novices might not have seen it before. Let them learn.
Really though, it would make more sense if 'if' was an expression rather than a statement, e.g.:
int x=if (address==null) null else address.getCity();
That's how it is in many many languages that haven't been brain-damaged by C.
E.g., Scheme, approximately:
(define x
. (if (eq? nil address)
. . . nil
. . . (get-city address))
(the dots are only for blogger to format it right)
Refactor your brain with this: In many languages, even loops are expressions, not statements.
If you have 'simple' getters and setters, then there's no encapsulation to break.
You have GOT to be joking?!
That's absolutely ridiculous.
OJ, I can see you're an open-minded fellow. I'll just ask you one question:
How is the first code snippet more encapsulated than the second here?
1.
class X
{
. . private int x;
. . public int getX()
. . {
. . . . return x;
. . }
. . public void setX(int x)
. . {
. . . . this.x=x;
. . }
}
2.
class X
{
. . public int x;
}
This isn't simplifying, this is the fast road making code less readable and less flexible. These are beginner mistakes. For god's sake don't take this advice.
Sure, there are many situations where coding standards should not be taken literally, and KISS is better. This isn't one of them. These kind of short sighted so called optimizations are one of the reasons coding standards exist.
Unless of course you are absolutely certain this code will never change. Guess what: that's the one thing you can never be certain of.
Shaving off a few lines of code isn't worth the trouble you will be causing yourself and others in the long run.
(Having said that, the original code leaves some things to be desired too. At least creating one exit point is an improvement. One thing that's even worse is organically grown functions with 'returns' hidden somewhere in the middle.)
Rick,
I am sorry if you misunderstood my intentions.
I am in no way advocating replacement of if-else conditions with ? : conditional operators. Quite the opposite, I like well formatted if-else statements in the code.
However, if you read my post again you will agree with me that if somebody writes "if something is true return true, else (something is false) return false" as in my first code example, it is simply verbose and can be simplified without sacrificing readability.
I also like having one exit point (return statement) in methods. I am even willing to write more code to do so. The benefit is that this will allow your IDE to perform refactorings if needed. For example several exit points from a method can prevent code extraction - e.g. extract method refactoring.
http://www.refactoring.com/catalog/extractMethod.html
Cheers,
Dushan
I advocate replacing if-else with ? :, because expressions are better than statements, as they are composable.
Coding specifically to get one return point seems to be a holdover from non-garbage-collected languages - you would need to free any temporary objects you created. There are still times you need to do that, in Java - external resources, but for those you can use a finally block.
I say exit as soon as you can, it keeps the code simple. IDEs not being able to refactor it is a minor point, and hopefully only a temporary glitch in the evolution of IDEs. It's fairly trivial to manually change code to use single-exit, for interaction with those tools.
An even simpler approach would to be to avoid null altogether introduce an instance of Address that represents the "empty" address.
Address.empty.getCity() could return null (or "" to further prevent null pointer exceptions)
There is a pattern for this, but I forget then name.
So far I like your blog. I'd be interested to hear your comments on my blog.
Daniel,
Your suggestion only serves to mask bugs. IDEA's @NotNull and @Nullable point them out to the developer instead. I suggest you use them.
Alternatively, copy Haskell's Maybe or Scala's Option to Java. They get rid of the problem, but the syntax for using them is generaly not pretty in Java.
@Ricky
It serves more than to mask bugs (although sometimes it may do that). It simplifies the case where EMPTY isn't really a special case. If you can describe the behavior of a not-yet-set instance, then you should consider using an EMPTY instance over null. If, on the other hand, you want to externalize the behavior of an object (the opposite of encapsulation), then feel free to return null.
@NotNull and @Null do help when you need to prevent runtime NPE, but they don't help to simplify code (which is what this post was about, wasn't it?)
the Nice programming language has built-in support for Null and NotNull, by prepending possibly-null reference types with ? (For exmaple, ?String is a possible null String)
Post a Comment