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}));

10 Responses to “Bad and Good (code snippets)”

  1. John Kostaras Says:

    Πολύ ωραία τα παραπάνω και πολύ χρήσιμα που τα μάζεψες εδώ. Επίσης χρήσιμο είναι και όταν ξέρεις ότι μια μεταβλητή η παράμετρος δεν αλλάζει να την ορίζεις final. Καλή συνέχεια…

  2. John Kostaras Says:

    And don´t use try-catch to make decisions; use if:

    http://www.sdnshare.com/view.jsp?id=781

  3. papo Says:

    ωραίο post …παρόλα ομολογώψ ότι συνειδητά το

    πρώτο πρώτο και το άλλο με το return new()-

    βασικά αποφεύγω ιδιαίτερα σε pre-mature implementation την χρήση inline calls σε return statements! από δικη μου εμπειρία καθώς ο κώδικας εξελίσσεται τέτοια inline statements με γ@μ@νε!

  4. John Kostaras Says:

    http://findbugs.sourceforge.net/

  5. dik_ Says:

    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 ;)

  6. Ioannis Mavroukakis Says:

    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.

  7. cherouvim Says:

    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.

  8. shaun Says:

    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

  9. cherouvim Says:

    @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.

  10. Rob Church Says:

    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.)