views:

510

answers:

9

I need to compare dozens of fields in two objects (instances of the same class), and do some logging and updating in case there are differences. Meta code could look something like this:

if (a.getfield1 != b.getfield1)
  log(a.getfield1 is different than b.getfield1)
  b.field1 = a.field1

if (a.getfield2!= b.getfield2)
  log(a.getfield2 is different than b.getfield2)
  b.field2 = a.field2

...

if (a.getfieldn!= b.getfieldn)
  log(a.getfieldn is different than b.getfieldn)
  b.fieldn = a.fieldn

The code with all the comparisons is very terse, and I would like to somehow make it more compact. It would be nice if I could have a method which would take as a parameter method calls to setter and getter, and call this for all fields, but unfortunately this is not possible with java.

I have come up with three options, each which their own drawbacks.

1. Use reflection API to find out getters and setters
Ugly and could cause run time errors in case names of fields change

2. Change fields to public and manipulate them directly without using getters and setters
Ugly as well and would expose implementation of the class to external world

3. Have the containing class (entity) do the comparison, update changed fields and return log message
Entity should not take part in business logic

All fields are String type, and I can modify code of the class owning the fields if required.

EDIT: There are some fields in the class which must not be compared.

+1  A: 

This is probably not too nice either, but it's far less evil (IMHO) than either of the two alternatives you've proposed.

How about providing a single getter/setter pair that takes a numeric index field and then have getter/setter dereference the index field to the relevant member variable?

i.e.:

public class MyClass {
    public void setMember(int index, String value) {
        switch (index) {
           ...
        }
    }

    public String getMember(int index) {
        ...
    }

    static public String getMemberName(int index) {
        ...
    }
}

And then in your external class:

public void compareAndUpdate(MyClass a, MyClass b) {
    for (int i = 0; i < a.getMemberCount(); ++i) {
        String sa = a.getMember();
        String sb = b.getMember();
        if (!sa.equals(sb)) {
            Log.v("compare", a.getMemberName(i));
            b.setMember(i, sa);
        }
    }
}

This at least allows you to keep all of the important logic in the class that's being examined.

Alnitak
This would not work because there are some fields that I don't want to compare. Sorry for not pointing this out in the question, I will edit it. Thanks for the idea though.
tputkonen
that's fine - just don't expose those fields in the example code I gave.
Alnitak
A: 

While option 1 may be ugly, it will get the job done. Option 2 is even uglier, and opens your code to vulnerabilities you can't imagine. Even if you eventually rule out option 1, I pray you keep your existing code and not go for option 2.

Having said this, you can use reflection to get a list of the field names of the class, if you don't want to pass this as a static list to the method. Assuming you want to compare all fields, you can then dynamically create the comparisons, in a loop.

If this isn't the case, and the strings you compare are only some of the fields, you can examine the fields further and isolate only those that are of type String, and then proceed to compare.

Hope this helps,

Yuval =8-)

Yuval
+2  A: 

I would go for option 1, but I would use getClass().getDeclaredFields() to access the fields instead of using the names.

public void compareAndUpdate(MyClass other) throws IllegalAccessException {
    for (Field field : getClass().getDeclaredFields()) {
        if (field.getType() == String.class) {
            Object thisValue = field.get(this);
            Object otherValue = field.get(other);
            // if necessary check for null
            if (!thisValue.equals(otherValue)) {
                log(field.getName() + ": " + thisValue + " <> " + otherValue);
                field.set(other, thisValue);
            }
        }
    }
}

There are some restrictions here (if I'm right):

  • The compare method has to be implemented in the same class (in my opinion it should - regardless of its implementation) not in an external one.
  • Just the fields from this class are used, not the one's from a superclass.
  • Handling of IllegalAccessException necessary (I just throw it in the example above).
rudolfson
This would compare all the fields but there are some fields that I don't want to compare.
tputkonen
It compares all String fields, right. Didn't see this from your explanation.You could however 'mark' fields you want to compare by using an own subclass of String, just implementiny necessary constructors. This could be an inner private class of your class. Getters and setters for the fields should still use String.
rudolfson
Or better yet (sorry for so many comments :-)), use an Annotation on those fields.
rudolfson
As i mentioned above - this only compares Fields in the current class it doesnt visit fields in the super class.
mP
@mP I already mentioned that in my list of restrictions.
rudolfson
+9  A: 

Use Annotations.

If you mark the fields that you need to compare (no matter if they are private, you still don't lose the encapsulation, and then get those fields and compare them. It could be as follows:

In the Class that need to be compared:

@ComparableField 
private String field1;

@ComparableField
private String field2;

private String field_nocomparable;

And in the external class:

public <T> void compare(T t, T t2) throws IllegalArgumentException,
                                          IllegalAccessException {
    Field[] fields = t.getClass().getDeclaredFields();
    if (fields != null) {
        for (Field field : fields) {
            if (field.isAnnotationPresent(ComparableField.class)) {
                field.setAccessible(true);
                if ( (field.get(t)).equals(field.get(t2)) )
                    System.out.println("equals");
                field.setAccessible(false);
            }
        }
    }
}

The code is not tested, but let me know if helps.

David Santamaria
Still somewhat ugly as it uses reflection, but if you want to use reflection, probably the best option.
sleske
you're comparing t to t at the moment...
Alnitak
Thanks Alnitak, changed
David Santamaria
The above code is incomplete it doesnt visit Fields belonging to Super classes - for that you will need some recursion.
mP
Notice for someone using this: remember to define @Retention(RetentionPolicy.RUNTIME) for your annotation interface.
tputkonen
Nice Answer +1! But one little note: The (fields != null) check is unnecessary, because getDeclaredFields never returns null.
Tim Büthe
A: 

I would also propose a similar solution to the one by Alnitak.

If the fields need to be iterated when comparing, why not dispense with the separate fields, and put the data into an array, a HashMap or something similar that is appropriate.

Then you can access them programmatically, compare them etc. If different fields need to be treated & compared in different ways, you could create approriate helper classes for the values, which implement an interface.

Then you could just do

valueMap.get("myobject").compareAndChange(valueMap.get("myotherobject")

or something along those lines...

sleske
A: 

A broad thought:

Create a new class whose object takes the following parameters: the first class to compare, the second class to compare, and a lists of getter & setter method names for the objects, where only methods of interest are included.

You can query with reflection the object's class, and from that its available methods. Assuming each getter method in the parameter list is included in the available methods for the class, you should be able to call the method to get the value for comparison.

Roughly sketched out something like (apologies if it isn't super-perfect... not my primary language):

public class MyComparator
{
    //NOTE: Class a is the one that will get the value if different
    //NOTE: getters and setters arrays must correspond exactly in this example
    public static void CompareMyStuff(Object a, Object b, String[] getters, String[] setters)
    {
        Class a_class = a.getClass();
        Class b_class = b.getClass();

        //the GetNamesFrom... static methods are defined elsewhere in this class
        String[] a_method_names = GetNamesFromMethods(a_class.getMethods());
        String[] b_method_names = GetNamesFromMethods(b_class.getMethods());
        String[] a_field_names = GetNamesFromFields(a_class.getFields());

        //for relative brevity...
        Class[] empty_class_arr = new Class[] {};
        Object[] empty_obj_arr = new Object[] {};

        for (int i = 0; i < getters.length; i++)
        {
            String getter_name = getter[i];
            String setter_name = setter[i];

            //NOTE: the ArrayContainsString static method defined elsewhere...
            //ensure all matches up well...
            if (ArrayContainsString(a_method_names, getter_name) &&
                ArrayContainsString(b_method_names, getter_name) &&
                ArrayContainsString(a_field_names, setter_name)
            {
                //get the values from the getter methods
                String val_a = a_class.getMethod(getter_name, empty_class_arr).invoke(a, empty_obj_arr);
                String val_b = b_class.getMethod(getter_name, empty_class_arr).invoke(b, empty_obj_arr);
                if (val_a != val_b)
                {
                    //LOG HERE
                    //set the value
                    a_class.getField(setter_name).set(a, val_b);
                }
            } 
            else
            {
                //do something here - bad names for getters and/or setters
            }
        }
    }
}
Demi
+3  A: 

The JavaBeans API is intended to help with introspection. It has been around in one form or another since Java version 1.2 and has been pretty usable since version 1.4.

Demo code that compares a list of properties in two beans:

  public static void compareBeans(PrintStream log,
      Object bean1, Object bean2, String... propertyNames)
      throws IntrospectionException,
      IllegalAccessException, InvocationTargetException {
    Set<String> names = new HashSet<String>(Arrays
        .asList(propertyNames));
    BeanInfo beanInfo = Introspector.getBeanInfo(bean1
        .getClass());
    for (PropertyDescriptor prop : beanInfo
        .getPropertyDescriptors()) {
      if (names.remove(prop.getName())) {
        Method getter = prop.getReadMethod();
        Object value1 = getter.invoke(bean1);
        Object value2 = getter.invoke(bean2);
        if (value1 == value2
            || (value1 != null && value1.equals(value2))) {
          continue;
        }
        log.format("%s: %s is different than %s%n", prop
            .getName(), "" + value1, "" + value2);
        Method setter = prop.getWriteMethod();
        setter.invoke(bean2, value2);
      }
    }
    if (names.size() > 0) {
      throw new IllegalArgumentException("" + names);
    }
  }

Sample invocation:

compareBeans(System.out, bean1, bean2, "foo", "bar");

If you go the annotations route, consider dumping reflection and generating the comparison code with a compile-time annotation processor or some other code generator.

McDowell
Thanks, this seems perfect!
tputkonen
Where is this solution different from option 1? It's nicer due to the use of the Beans API instead of using the Relections API directly. But you still have to name all your fields for comparison, still leaving the problem when you rename some of them. Or did I overlook something?
rudolfson
Yes, that issue remains, but I don't see a way to get around it (without annotations). However reflection is much more "low level" compared to BeanInfo.
tputkonen
If (as stated) the logic for this comparison/update must be kept out of the bean, then annotating the fields is not an ideal design decision (though putting all the logic in one place is nice). The approach I outlined is not ideal either - by its nature, reflection cannot catch issues at compile-time (though missing fields will be caught at runtime). Generated direct calls (annotation-based or property-list-based) would be better than either and avoids writing boiler-plate, but only the OP knows if it is worth the effort.
McDowell
As a matter of fact I changed my mind and decided to go Annotations way, because of type safety and in a way it's natural to specify the fields to compare in the entity. Thanks for all who answered, I've learned a lot!
tputkonen
A: 

since

All fields are String type, and I can modify code of the class owning the fields if required.

you could try this class:

public class BigEntity {

    private final Map<String, String> data;

    public LongEntity() {
        data = new HashMap<String, String>();
    }

    public String getFIELD1() {
        return data.get(FIELD1);
    }

    public String getFIELD2() {
        return data.get(FIELD2);
    }

    /* blah blah */
    public void cloneAndLogDiffs(BigEntity other) {
        for (String field : fields) {
            String a = this.get(field);
            String b = other.get(field);

            if (!a.equals(b)) {
                System.out.println("diff " + field);
                other.set(field, this.get(field));
            }
        }
    }

    private String get(String field) {
        String value = data.get(field);

        if (value == null) {
            value = "";
        }

        return value;
    }

    private void set(String field, String value) {
        data.put(field, value);
    }

    @Override
    public String toString() {
        return data.toString();
    }

magic code:

    private static final String FIELD1 = "field1";
    private static final String FIELD2 = "field2";
    private static final String FIELD3 = "field3";
    private static final String FIELD4 = "field4";
    private static final String FIELDN = "fieldN";
    private static final List<String> fields;

    static {
        fields = new LinkedList<String>();

        for (Field field : LongEntity.class.getDeclaredFields()) {
            if (field.getType() != String.class) {
                continue;
            }

            if (!Modifier.isStatic(field.getModifiers())) {
                continue;
            }

            fields.add(field.getName().toLowerCase());
        }
    }

this class has several advantages:

  • reflects once, at class loading
  • it is very simply adding new fields, just add new static field (a better solution here is using Annotations: in the case you care using reflection works also java 1.4)
  • you could refactor this class in an abstract class, all derived class just get both
    data and cloneAndLogDiffs()
  • the external interface is typesafe (you could also easily impose immutability)
  • no setAccessible calls: this method is problematic sometimes
dfa
A: 

You say you presently have getters and setters for all these fields? Okay, then change the underlying data from a bunch of individual fields to an array. Change all the getters and setters to access the array. I'd create constant tags for the indexes rather than using numbers for long-term maintainability. Also create a parallel array of flags indicating which fields should be processed. Then create a generic getter/setter pair that use an index, as well as a getter for the compare flag. Something like this:

public class SomeClass
{
  final static int NUM_VALUES=3;
  final static int FOO=0, BAR=1, PLUGH=2;
  String[] values=new String[NUM_VALUES];
  static boolean[] wantCompared={true, false, true};

  public String getFoo()
  {
    return values[FOO];
  }
  public void setFoo(String foo)
  {
    values[FOO]=foo;
  }
  ... etc ...
  public int getValueCount()
  {
    return NUM_VALUES;
  }
  public String getValue(int x)
  {
    return values[x];
  }
  public void setValue(int x, String value)
  {
    values[x]=value;
  }
  public boolean getWantCompared(int x)
  {
    return wantCompared[x];
  }
}
public class CompareClass
{
  public void compare(SomeClass sc1, SomeClass sc2)
  {
    int z=sc1.getValueCount();
    for (int x=0;x<z;++x)
    {
      if (!sc1.getWantCompared[x])
        continue;
      String sc1Value=sc1.getValue(x);
      String sc2Value=sc2.getValue(x);
      if (!sc1Value.equals(sc2Value)
      {
        writeLog(x, sc1Value, sc2Value);
        sc2.setValue(x, sc1Value);
      }
    }
  }
}

I just wrote this off the top of my head, I haven't tested it, so their may be bugs in the code, but I think the concept should work.

As you already have getters and setters, any other code using this class should continue to work unchanged. If there is no other code using this class, then throw away the existing getters and setters and just do everything with the array.

Our Java bean is also an JPA entity so this approach would not work in our case.
tputkonen