Bad and Good (code snippets)
What follows is usual modifications I apply to java code, when I code review. Most of the following changes have a stylistic nature, but some of them can impact performance. Bad code is in red and good code is in green. Feel free to comment.
Unnecessary if statement
if (foo>bar) { return true; } else { return false; }
return foo>bar;
Unnecessary nesting
if (foo) { if (bar) { // do something } }
if (foo && bar) { // do something }
A bit of redundancy here
if (foo!=null && foo.equals("bar")) { // do something }
if ("bar".equals(foo)) { // do something }
Looks ugly
String comment = request.getParameter("post"); if (comment==null) { comment = "n/a"; }
String comment = request.getParameter("post")!=null? request.getParameter("post"): "n/a";
This only holds if request.getParameter("post")
is cheap.
Unnecessary variable allocation
Something something = new Something(foo); return something;
return new Something(foo);
Too much variable scope
Something something = new Something(); if (foo) { something.prepare(); request.setAttribute("something", something); }
if (foo) { Something something = new Something(); something.prepare(); request.setAttribute("something", something); }
Method exposes implementation
public ArrayList getSomething() { ... }
public List getSomething() { ... }
A bit slow
String foo = "something"; if (!foo.equals("")) { // do something }
String foo = "something"; if (foo.length()>0) { // do something }
Too many lines
List immutableList = new ArrayList(); immutableList.add(foo); immutableList.add(bar); immutableList.add(example); Something something = new Something(immutableList);
Something something = new Something( Arrays.asList(new Object[] {foo, bar, example}));
August 21st, 2007 at 10:14
Πολύ ωραία τα παραπάνω και πολύ χρήσιμα που τα μάζεψες εδώ. Επίσης χρήσιμο είναι και όταν ξέρεις ότι μια μεταβλητή η παράμετρος δεν αλλάζει να την ορίζεις final. Καλή συνέχεια…
August 22nd, 2007 at 11:01
And don´t use try-catch to make decisions; use if:
http://www.sdnshare.com/view.jsp?id=781
August 27th, 2007 at 12:53
ωραίο post …παρόλα ομολογώψ ότι συνειδητά το
πρώτο πρώτο και το άλλο με το return new()-
βασικά αποφεύγω ιδιαίτερα σε pre-mature implementation την χρήση inline calls σε return statements! από δικη μου εμπειρία καθώς ο κώδικας εξελίσσεται τέτοια inline statements με γ@μ@νε!
December 19th, 2007 at 17:20
http://findbugs.sourceforge.net/
February 21st, 2008 at 17:41
Nice snippets :)
Regarding the “Too much variable scope” one, may I add that a new Something instance will be created even if foo is false, taking up unnecessarily memory. So there’s another reason to move the instantiation in the if block ;)
July 8th, 2008 at 16:22
Ah the age old chestnut of readability versus one liners..for me
String comment = request.getParameter(“post”);
if (comment==null) {
comment = “n/a”;
}
Is much more readable and I am willing to loose the ill perceived coolness of ternary expressions.
July 8th, 2008 at 16:30
Ioanni thanks for your comment.
If it helps you then good for you.
I don’t think that using the ternary operator is a matter of coolness though.
February 10th, 2009 at 20:02
A noob programmer here so forgive me if I’m asking a stupid question. Regarding “Method exposes implementation”, shouldn’t it be better to return an ArrayList instead of a List? Therefore the caller (expecting an ArrayList) does not have to cast the returned value. No?
Great site btw
February 10th, 2009 at 20:36
@shaun: Good question.
The interface List contains everything that a user of a List implementation would like to do, such as add(), remove(), indexOf() etc. The point is that it shouldn’t matter what the underlying implementation of List is (ArrayList? LinkedList? Vector? etc). So the client shouldn’t need to downcast anyway.
December 31st, 2010 at 2:03
Syntactically correct:
public ArrayList getSomething() {}
Better:
public List getSomething() {}
Even better:
public List<T> getSomething() {}
(Assuming that getSomething() is able to return a type-safe generic list, and isn’t in fact returning a collection of arbitrary types, in which case you’ve got other problems anyway. Also assumes you can use generics, and thus implies JDK 1.5, and if you can’t, my sympathies.)