5
\$\begingroup\$

In a Java/JPA project I usually define a base class (on which I put the id). The goal is to implement an generic toString() on this base class (named BaseEntity) from which all other entities inherit. We played around with apache commons ReflectionToStringBuilder, but we ran into problems because of cyclic references and the lack of different handling depending on the type of the fields.

My wish is that, if an Entity is printed/logged I get all field names and their values except for fields of type collection, in this case we just want the IDs of these objects to avoid recursion (PS: We can assume that collections on entities always contain other entities and they have an ID)

The desired output pattern is the following

classname[fieldname1=value1, fieldname2=value2,..., collectionname1=[id1,id2,..], collectionname2=[id1,id2,...],...]

a sample output for an entity "Project" which subclasses BaseEntity may look like this:

Project[nameLong=Testproject, nameShort=tpr, support=false, active=true, priority=0, blockers=[20], responsibilities=[13,14,15]]

We came up with the following implementation using reflection:

@Override
public String toString() {
    // init container for field names
    List<String> fieldToPrintout = new ArrayList<String>(0);

    // get all fields from this object (use declared fields to get private
    // and protected fields)
    Field[] fieldsOnTheObject = this.getClass().getDeclaredFields();

    // set the accessibility for field which are private
    AccessibleObject.setAccessible(fieldsOnTheObject, true);


    for (Field field : fieldsOnTheObject) {
        try {
            // ignore static fields
            if (!Modifier.isStatic(field.getModifiers())) {
                // get the object from the field
                Object fieldValue = field.get(this);
                // check if the field is a collection
                if (Collection.class.isAssignableFrom(field.getType())) { 
                    // check if the collection generic type is BaseEntity
                    if (field.getGenericType() instanceof ParameterizedType) {
                        Class<?> genericType = (Class<?>) ((ParameterizedType) field.getGenericType()).getActualTypeArguments()[0];
                        if (BaseEntity.class.isAssignableFrom(genericType)) {
                            // iterate over all BaseEntities and get all id's
                            List<String> ids = ((Collection<BaseEntity>) fieldValue).stream().map(p -> p.getId().toString()).collect(Collectors.toList());
                            fieldToPrintout.add(field.getName() + "=[" + String.join(",", ids) + "]");
                        }
                    }
                } else { // --> no collection
                    // note: Field.get(Object) creates wrapper objects for primitive
                    fieldToPrintout.add(field.getName() + "=" + fieldValue);
                }
            }
        } catch (IllegalAccessException ex) {
            logger.catching(ex);
        }
    }
    return MessageFormat.format("{0}[{1}]", this.getClass().getSimpleName(), String.join(", ", fieldToPrintout));
}

I'm currently not really happy with the code, I'm sure it can be improved/simplified. Any suggestions?

\$\endgroup\$
5
\$\begingroup\$

So, there are a couple of things that can be done with this. First, use methods! Short, well-named methods can make it much easier to read the code. Second, use short-circuiting logic. continue is your friend. Most of your comments don't really contribute much, or can be obviated with good method names. You can also set the size of the ArrayList at creation time, because you know how many fields you're going to be dealing with. In my opinion, something like this looks a lot cleaner:

@Override
public String toString() {

    final Field[] fields = this.getClass().getFields();
    final List<String> fieldsToPrintOut = new ArrayList<String>(fields.length);

    AccessibleObject.setAccessible(fields, true);

    for (final Field field : fields) {
        if (Modifier.isStatic(field.getModifiers())) {
            continue;
        }

        final Object fieldValue;
        try {
            fieldValue = field.get(this);
        } catch (final IllegalAccessException ex) {
            logger.catching(ex);
            continue;
        }

        if (!isCollection(field)) {
            fieldsToPrintOut.add(field.getName() + "=" + fieldValue);
            continue;
        }

        if (!isParameterized(field) || !isBaseEntity(field)) {
            continue;
        }

        final List<String> ids =
                ((Collection<BaseEntity>) fieldValue).stream().map(p -> p.getId().toString()).collect(Collectors.toList());
        fieldsToPrintOut.add(field.getName() + ids);

    }
    return MessageFormat.format(this.getClass().getSimpleName() + fieldsToPrintOut);
}

private static boolean isCollection(final Field field) {
    return Collection.class.isAssignableFrom(field.getType());
}

private static boolean isParameterized(final Field field) {
    return field.getGenericType() instanceof ParameterizedType;
}

private static boolean isBaseEntity(final Field field) {
    final Class<?> genericType =
            (Class<?>) ((ParameterizedType) field.getGenericType()).getActualTypeArguments()[0];

    return BaseEntity.class.isAssignableFrom(genericType);
}
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Eric, your implementation is definitely more readable than the original, but you have 1) two times <code>if(condition) { continue; }</code> that may be extracted in <code>if (!shouldOutputField(field)) { continue; }</code>. 2) fieldValue initialization would better be extracted into separate method with something returned on exception, otherwise the entire field is ignored in the output. 3) String.join(",", list) is not necessary on Lists: the items are already concatenated with ", " in case of default list.toString() call. 4) there are two typos in deCLaredFields and fieldSToPrintout \$\endgroup\$ – Antot Oct 22 '15 at 17:31
  • \$\begingroup\$ @Antot 1) I disagree. This style shows why the field should not be output without having to dig into another method. 2) The original method ignores fields with an exception. Adding a method to change the try {} catch(){} into an if (fieldValue == null) is of dubious value, but the OP can certainly do so. 3) Fixed. Good catch. 4) Also fixed. Note that the original method variable was fieldToPrintout, but fieldsToPrintOut is definitely better. \$\endgroup\$ – Eric Stein Oct 22 '15 at 17:55
  • \$\begingroup\$ Eric, 1) ok; 2) I thought that generally it would be better to see in the output something like "fieldName=[ERROR:not_accessible]" than not to have it at all. \$\endgroup\$ – Antot Oct 22 '15 at 18:01
  • \$\begingroup\$ @Antot That's up to the OP. I can't tell him how he should handle an error, so I duplicated the functionality. \$\endgroup\$ – Eric Stein Oct 22 '15 at 18:17
  • \$\begingroup\$ thanks for the comments, the extracting of methods made it far more readable. I did not cared too much about the excpetion handling so far because I don't see why exceptions should happen at all. But I guess I would be nice to incorporate such things in the returned string \$\endgroup\$ – Raphael Roth Oct 23 '15 at 5:04
5
\$\begingroup\$

After some time using the solution given by Eric, I discovered certain problems (they are already present in my original solution, therefore I don't want to blame Eric).

  • The ID is not given in the output as getDeclaredFields() does NOT include fields defined on the superclass.
  • Accessing the private fields directly is not a good idea. It would be better to access the public accessors.
  • I'm not sure whether is really a good idea to override toString() in the first place, as toString() may get automatically invoked by frameworks, therefore having a slow toString() is undesireable. I therefore decided to use a separate method for the inspection of the JPA entity.

Here my new solution using org.apache.commons.beanutil:

import java.io.Serializable;
import java.lang.reflect.InvocationTargetException;
import java.text.MessageFormat;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;

import org.apache.commons.beanutils.PropertyUtils;

public abstract class BaseEntity implements Serializable {

    private static final long serialVersionUID = 1326510770497247826L;

    private Long id;

    public Long getId() {
        return id;
    }

    public void setId(Long id) {
        this.id = id;
    }

    public String inspect() {
        return inspect(this);
    }

    private String inspect(Object o) {
        if (o == null) {
            return "null";
        }

        // List to store key/value strings
        List<String> fieldToPrintout = new ArrayList<String>(0);

        // check if object is a collection, if yes, invoke inspect each element
        if (Collection.class.isAssignableFrom(o.getClass())) {
            Collection<Object> col = ((Collection<Object>) o);
            for (Object obj : col) {
                // if element extends BaseEntity, fetch only id
                if (BaseEntity.class.isAssignableFrom(obj.getClass())) {
                    fieldToPrintout.add(((BaseEntity) obj).getId() == null ? "null" : ((BaseEntity) obj).getId().toString());
                } else {
                    fieldToPrintout.add(obj.toString());
                }
            }
           return MessageFormat.format("{0}[{1}]", o.getClass().getSimpleName(), String.join", ", fieldToPrintout));

            // if the argument is no collection and does not extending BaseEntity
            // --> invoke standard toString()
        } else if (!(BaseEntity.class.isAssignableFrom(o.getClass()))) {
            return o.toString();
        }

        // at this point we know the argument extends BaseEntity, i.e. a is a Bean.

        // map for storing key/values of beans
        Map<String, Object> properties = new HashMap<>();

        // inspect properties:
        try {
            properties = PropertyUtils.describe(o);
        } catch (IllegalAccessException | InvocationTargetException | NoSuchMethodException e) {
            e.printStackTrace();
        }

        // loop over all properties
        for (Entry<String, Object> entry : properties.entrySet()) {
            if (exclude(entry)) {
                continue;
            }
            fieldToPrintout.add(entry.getKey() + "=" + inspect(entry.getValue()));
        }
        return MessageFormat.format("{0}[{1}]", o.getClass().getSimpleName(), String.join(", ", fieldToPrintout));
    }


    /**
     * helpermethod to exclude unwanted key/value pairs from PropertyUtils.describe(o)
     * 
     * @param entry
     * @return
     */
    private boolean exclude(Entry<String, Object> entry) {
        String key = entry.getKey();
        Object value = entry.getValue();

        if (key.equals("class") && value.getClass().equals(Class.class)) {
            return true;
        }

        return false;
    }
}

A small demo (Company and Employee are standard Beans extending BaseEntity):

Company company = new Company();
company.setId(1L);
company.setName("Super Company");
company.getDomains().add("Engineering");
company.getDomains().add("Consulting");
company.getDomains().add("Training");
company.setSize(100);

Employee employee1 = new Employee();
employee1.setId(1L);
employee1.setActive(true);
employee1.setName("Employee 1");
employee1.setCompany(company);

Employee employee2 = new Employee();
employee2.setActive(false);
employee2.setName("Employee 2");
employee2.setCompany(company);

company.getEmployees().add(employee1);
company.getEmployees().add(employee2);

System.out.println(company.inspect());

Gives:

Company[size=100, name=Super Company, domains=ArrayList[Engineering, Consulting, Training], id=1, employees=HashSet[1, null]]
|improve this answer|||||
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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