Method to lookup persistent entities by an example object

i'm currently writing a generic data access object for the persistent entities I have to handle in my JavaEE application. What this codeReview is about is a findByExample method that is used to find database entries on the basis of a given example object. This may already be implemented for openJPA but I'm bound to openJPA snapshot 1.2.x (old) and I couldn't find it anywhere in the manual. please tell me about the pitfalls, improvements and bad practices you see in my code. I'm willing to learn.

Here we go

Using it as following

MyEntity me = new MyEntity();
me.setName("john")
me.setAge(18);

entityAccess.findByExample(me);


will create the following JPQL query using only the parameters that were set

"SELECT x FROM MyEntity x WHERE x.name = :name AND x.age = :age"

Then it will set the parameters and exequte the query.

This is how I implemented it.

The two public methods (BasicEntity is a mappedSuperClass)

public  List findAllByExample(T entity) {
return getQueryFromExample(entity).getResultList();
}

public  T findByExample(T entity) throws NoResultException, NonUniqueResultException {
return (T) getQueryFromExample(entity).getSingleResult();
}

The method thats creates the Query and sets the parameters. (em is the jpa entityManager)

```
private Query getQueryFromExample(T entity) {

HashMap fieldNameValuePairs = getFieldNameValuePairs(entity.getClass(), entity);

/ build query string /
StringBuilder queryString = new StringBuilder();
queryString.append("SELECT x FROM ");
queryString.append(entity.getClass().getSimpleName());
queryString.append(" x WHERE ");
for (Iterator iterator = fieldNameValuePairs.keySet().iterator(); iterator.hasNext();) {
String fieldName = iterator.next();
queryString.append("x." + fieldName + " = :" + fieldName);
if (iterator.hasNext()) {
queryString.append(" AND ");
}
}

/ create query and set parameters /
Query q = em.createQuery(queryString.toString());
for (Entry entry : fieldNameVa

I found the code an interesting read and quite a pleasant one at that. Nothing really critical jumped out thought there are some precondition checking, logging, style and other violations in respect to what I might do.

1) I like to abstract trivial things away. For example, when you create the query string I would try to emphasize the structure of the query string instead of the noise caused by it's building. This might be done by creating method(s) that create the where condition of the query:

import static java.lang.String.format; 

... 

String queryString = format("SELECT x FROM %s WHERE %s",
                             entity.getClass().getSimpleName(),
                             join(asConditions(fieldNameValuePairs.keySet()), " AND "));

... 

private Collection asConditions(Collection fieldNames) {
   List conditions = new ArrayList();
   for(String fieldName : fieldNames) {
       conditions.add(format("x.%1$s = :%1$s", fieldName)); // 1$ reuses the 1st argument
   }
   return conditions;
}

private String join(Collection parts, String glue) {
   StringBuilder sb = new StringBuilder();
   boolean first = true;
   for(String part : parts) {
      if(!first) {
         sb.append(glue);
      }
      sb.append(part);
      first = false;
   }
   return sb.toString();
}

2) I find it that the assigments are not needed here as they create new lines of codes without really increasing the readability of the whole.

for (Entry entry : fieldNameValuePairs.entrySet()) {
   String fieldName = entry.getKey();
   Object fieldValue = entry.getValue();
   q.setParameter(fieldName, fieldValue);
}

=>

for (Entry entry : fieldNameValuePairs.entrySet()) {
   q.setParameter(entry.getKey(), entry.getValue());
}

3) In the second method, you have created a long if-block in cas type is null. I would rather have failed early so the preconditions of the method is shown in the beginning. Also I feel that if the precondition is not met then some logging is needed or better yet an exception would be thrown because I would think that the missing type is a programmer error of some sorts and should not be let slip by.

If the null check is because of the recursion, you might want to create a comment as the recursion step is a bit far removed from the branching statement. Better yet, put the recursion in one method and the logic for the recursion step in another.

4) After noticing that the second method is private I noticed that there is no precondition checking in the first method. If the preconditions would be checked there, you can be a bit more relaxed with the checking in private methods although private methods may end up being reused by another developer at some point.

5) Again when branching on isPersistentField() you create a large block instead of exiting quickly. If you rather use this style then I would suggest you put the content of the block in a separate method. This way you can emphasize the logic of selection and keep methods nice a small.

Edit: Current impl:

for (Field field : type.getDeclaredFields()) {
  if (isPersistentField(field)) {
    field.setAccessible(true);
    String fieldName = field.getName();
    Object fieldValue = null;
    try {
       fieldValue = field.get(instance);
    } catch (Exception e) {
      throw new RuntimeException("foo", e);
    }

    if (!(fieldValue == null || fieldValue.equals(""))) {
      fieldNameValuePairs.put(fieldName, fieldValue);
    }
  }
}

Alternative 1:

for (Field field : type.getDeclaredFields()) {
  if (!isPersistentField(field)) {
    continue;
  }
  field.setAccessible(true);
  String fieldName = field.getName();
  Object fieldValue = null;
  try {
    fieldValue = field.get(instance);
  } catch (Exception e) {
    throw new RuntimeException("foo", e);
  }

  if (!(fieldValue == null || fieldValue.equals(""))) {
    fieldNameValuePairs.put(fieldName, fieldValue);
  }
}

Alternative 2:

for (Field field : type.getDeclaredFields()) {
  if (isPersistentField(field)) {
     addNameValuePair(field, instance, fieldNameValuePairs);
  }
}

...

private void addNameValuePair(Field field, Object instance, Map fieldNameValuePairs) {
  Object fieldValue = valueOf(field, instance);
  if (!(fieldValue == null || fieldValue.equals(""))) {
    fieldNameValuePairs.put(field.getName(), fieldValue);
  }
}

private Object valueOf(Field field, Object instance) {
  boolean wasAccessible = field.isAccessible();
  try {
    field.setAccessible(true);
    return field.get(instance);
  } catch (Exception e) {
    throw new RuntimeException("foo", e);
  } finally { 
    field.setAccessible(wasAccessible);
  } 
}

6) Set up a logger instead of System.out -> That way you can keep the debug level logs for later use if they needed.

7) JPA does not require that each field is annotated. This might cause some trouble on the long run. You could check how OpenJPA deals with this. They have their own FieldMetaData implementation t

StackExchange Code Review Q#4314, answer score: 4