current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

First version:

...
query.setMaxResults(1);         
List<Employee> list = query.getResultList();    
if (list != null) {
    for (Employee employee : list) {
        anotherList.add(employee);
    }
}

Second version:

...
query.setMaxResults(1);         
List<Employee> list = query.getResultList();    
if (list != null && list.size() !=0) {
    anotherList.add(list.get(0));
}
share

migration rejected from programmers.stackexchange.com Feb 21 '14 at 18:30

This question came from our site for professional programmers interested in conceptual questions about software development. Votes, comments, and answers are locked due to the question being closed here, but it may be eligible for editing and reopening on the site where it originated.

closed as primarily opinion-based by Jamal♦ Feb 21 '14 at 18:30

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise. If this question can be reworded to fit the rules in the help center, please edit the question.
1  
I'd prefer to get a single result and not a list when all you need is one single object. –  Falcon Aug 29 '11 at 13:30
6  
The first version is misleading, as it's easy to overlook the setMaxResults. AFAIK the null check is not needed (at least for JDBC). –  Landei Aug 29 '11 at 14:24
1  
This looks like JPA. It might be better to use anotherList.add(query.getSingleResult()); in a try/catch. –  Gary Buyn Nov 20 '11 at 20:31

3 Answers 3

up vote 4 down vote accepted

I agree that the null check is not need.

I also agree that the first version is misleading.

I suggest something like :

CurrentDao.java
  public MyBean find(...) {
    ...
    query.setMaxResults(1);
    List<MyBean> results = query.getResultList();
    return ListHelper.firstOrNull(results);
  }

ListHelper.java
  public <E> E firstOrNull(Collection<E> items) {
    if (items == null || items.isEmpty()) {
      return null;
    }
    return items.iterator().next();
  }

As an advantage for the extraction of the technical code for finding the first, you can see that:

  • the functional code is shorter, clearer (a static import could even go further)
  • the technical code will not not repeated, it is reusable.
  • the technical code handles all cases (if you forget one, you could add it later without impact on the calling code), and works on any type of Collection (only the iterator is usable).
share
up vote 4 down vote

I think @Landei is correct -- the first snippet makes it much too easy to miss the fact that you've used setMaxResults to limit the number of records to one, so you're only adding one record. Most of the code is written exactly as if you expected there to be many records, not just one.

Although the condition in the second snippet is marginally more complex looking, it does at least make the intent clear: that you'll add at most one record to the destination.

share
up vote 2 down vote

Skimming the first snippet, I expect that some kind of looping is going to occur, but it doesn't. It's just there to provide an invisible null check. This is clever, but surprising.

I'd prefer the second snippet because it is explicit about what it is doing.

share

Not the answer you're looking for? Browse other questions tagged or ask your own question.