Commons Lang
  1. Commons Lang
  2. LANG-637

There should be a DifferenceBuilder with a ReflectionDifferenceBuilder implementation

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.3
    • Component/s: lang.builder.*
    • Labels:
      None

      Description

      The ToStringBuilder and ReflectionToStringBuilder are great tools for everyday development.
      We use them to show all the properties in an object, which comes handy especially for testing.

      However, JUnit with its assertEquals() just outputs the toString() of the two compared objects. For complex objects, this becomes unreadable.

      So, it would be great to have a DifferenceBuilder with a ReflectionDifferenceBuilder implementation to be able to get only the differing properties of two objects. The question is whether the two objects would have to be of the same type or not.

      1. commons-lang3-LANG-637-complete.patch
        90 kB
        Duncan Jones
      2. commons-lang3-LANG-637.patch
        66 kB
        Duncan Jones
      3. DiffList.java
        5 kB
        Duncan Jones
      4. DiffBuilder.java
        23 kB
        Duncan Jones
      5. Diffable.java
        2 kB
        Duncan Jones
      6. Diff.java
        3 kB
        Duncan Jones
      7. DiffTest.java
        2 kB
        Duncan Jones
      8. DiffListTest.java
        5 kB
        Duncan Jones
      9. DiffBuilderTest.java
        17 kB
        Duncan Jones

        Activity

        Hide
        Henri Yandell added a comment -

        Putting into 3.1 as there's no backwards compat issues. That said - patches welcome

        Show
        Henri Yandell added a comment - Putting into 3.1 as there's no backwards compat issues. That said - patches welcome
        Hide
        Duncan Jones added a comment -

        I've begun work on an implementation of this feature, attached are some partial files. I would love some thoughts on whether the approach is sensible. Note that a reflection based builder would be required, but is not yet sketched out.

        I have defined a new interface, Differentiable<T>, that is similar to Comparable<T>. The DifferenceBuilder will test user-chosen fields for equality (much like EqualsBuilder) and will append any differing fields an internal ToStringBuilder (one for each object). The build() method will either return an empty string if the tested fields were equal, or a string of the form:

        [LHS Object] differs from [RHS Object]

        (where the objects are printed using the ToStringBuilder and a user-chosen style).

        Expected usage is as follows (taken from JavaDoc):

        public class Person implements Differentiable<Person> {
          String name;
          int age;
          boolean smoker;
          
          ...
          
          public String differentiateFrom(Person obj) {
            // No need for null check, as NullPointerException correct if obj is null
            return new DifferenceBuilder(this, obj, ToStringStyle.SHORT_PREFIX_STYLE)
              .append("name", this.name, obj.name)
              .append("age", this.age, obj.age)
              .append("smoker", this.smoker, obj.smoker)
              .build();
          }
        }
        

        If the design appeals, I'll continue with the leg-work and add all the necessary method overloads and additional constructors etc. I can then post an update for a more detailed review.

        Show
        Duncan Jones added a comment - I've begun work on an implementation of this feature, attached are some partial files. I would love some thoughts on whether the approach is sensible. Note that a reflection based builder would be required, but is not yet sketched out. I have defined a new interface, Differentiable<T> , that is similar to Comparable<T> . The DifferenceBuilder will test user-chosen fields for equality (much like EqualsBuilder ) and will append any differing fields an internal ToStringBuilder (one for each object). The build() method will either return an empty string if the tested fields were equal, or a string of the form: [LHS Object] differs from [RHS Object] (where the objects are printed using the ToStringBuilder and a user-chosen style). Expected usage is as follows (taken from JavaDoc): public class Person implements Differentiable<Person> { String name; int age; boolean smoker; ... public String differentiateFrom(Person obj) { // No need for null check, as NullPointerException correct if obj is null return new DifferenceBuilder( this , obj, ToStringStyle.SHORT_PREFIX_STYLE) .append( "name" , this .name, obj.name) .append( "age" , this .age, obj.age) .append( "smoker" , this .smoker, obj.smoker) .build(); } } If the design appeals, I'll continue with the leg-work and add all the necessary method overloads and additional constructors etc. I can then post an update for a more detailed review.
        Hide
        Matt Benson added a comment -

        The direction looks promising. I think it would be nice to have an actual datatype returned from the DifferenceBuilder offloading the string-building stuff to its #toString() implementation. This way a client could use the e.g. Difference object in ways other than simply rendering as text.

        Show
        Matt Benson added a comment - The direction looks promising. I think it would be nice to have an actual datatype returned from the DifferenceBuilder offloading the string-building stuff to its #toString() implementation. This way a client could use the e.g. Difference object in ways other than simply rendering as text.
        Hide
        Duncan Jones added a comment -

        I think it would be nice to have an actual datatype returned from the DifferenceBuilder offloading the string-building stuff to its #toString() implementation.

        Yes, that's a good idea. I'll also incorporate the suggestions from James Carman on the mailing list regarding a simpler nomenclature (he offers Diffable, DiffBuilder etc.).

        Show
        Duncan Jones added a comment - I think it would be nice to have an actual datatype returned from the DifferenceBuilder offloading the string-building stuff to its #toString() implementation. Yes, that's a good idea. I'll also incorporate the suggestions from James Carman on the mailing list regarding a simpler nomenclature (he offers Diffable , DiffBuilder etc.).
        Hide
        Duncan Jones added a comment -

        After some consideration, I'm struggling to imagine what exactly could be useful about a separate Diff object. In principle it seems like a fine idea and the first thing that came to mind would be methods to retrieve the differences via suitable getter methods. However, I can't think of a way to implement an interface that is not quite ugly.

        Building the differences is elegant enough, via methods such as:

        addBytes(String fieldName, byte lhs, byte rhs);
        

        but storing and retrieving them seems challenging, other than as Object s.

        Can anyone suggest how a Diff class could be structured and used? If not, then I may continue along the lines of my previous suggestions and focus on a DiffBuilder class that produces a String, much like ToStringBuilder.

        Show
        Duncan Jones added a comment - After some consideration, I'm struggling to imagine what exactly could be useful about a separate Diff object. In principle it seems like a fine idea and the first thing that came to mind would be methods to retrieve the differences via suitable getter methods. However, I can't think of a way to implement an interface that is not quite ugly. Building the differences is elegant enough, via methods such as: addBytes( String fieldName, byte lhs, byte rhs); but storing and retrieving them seems challenging, other than as Object s. Can anyone suggest how a Diff class could be structured and used? If not, then I may continue along the lines of my previous suggestions and focus on a DiffBuilder class that produces a String , much like ToStringBuilder .
        Hide
        Matt Benson added a comment - - edited

        Again, from the usage standpoint I just envisioned that if I were getting diffs I might for some reason want to know at a code level precisely in what manner two objects differed. You are of course correct that ultimately the retrieval mechanism could really only be object-based, though we could do some generics tricks in that regard. What about something like:

        public abstract class Diff<T> extends oacl.tuple.Pair<T, T> {
          private final Type type;
          private final String fieldName;
        
          protected Diff(String fieldName) {
            this.type = ObjectUtils.defaultIfNull(TypeUtils.getTypeArguments(getClass(), Diff.class).get(Diff.class.getTypeParameters()[0]), Object.class);
          }
        
          public final Type getType() {
            return type;
          }
        
          public final String getFieldName() {
            return fieldName;
          }
        }
        
        public class DiffBuilder implements Builder<List<Diff>> {
          private final List<Diff> diffs = new ArrayList<Diff>();
        
          public List<Diff> build() {
            return Collections.unmodifiableList(new ArrayList<Diff>(diffs));
          }
        
          public DiffBuilder append(final String fieldName, final boolean lhs, final boolean rhs) {
            if ( /* different */ ) {
              diffs.add(new Diff<Boolean>(fieldName) {
                public Boolean getLeft() {
                  return Boolean.valueOf(lhs);
                }
                public Boolean getRight() {
                  return Boolean.valueOf(rhs);
                }
              });
            }
            return this;
          }
        }
        

        The above example would avoid storing your primitives as Objects, for whatever small additional storage costs that would have incurred.

        $0.02,
        Matt

        Show
        Matt Benson added a comment - - edited Again, from the usage standpoint I just envisioned that if I were getting diffs I might for some reason want to know at a code level precisely in what manner two objects differed. You are of course correct that ultimately the retrieval mechanism could really only be object-based, though we could do some generics tricks in that regard. What about something like: public abstract class Diff<T> extends oacl.tuple.Pair<T, T> { private final Type type; private final String fieldName; protected Diff( String fieldName) { this .type = ObjectUtils.defaultIfNull(TypeUtils.getTypeArguments(getClass(), Diff.class).get(Diff.class.getTypeParameters()[0]), Object .class); } public final Type getType() { return type; } public final String getFieldName() { return fieldName; } } public class DiffBuilder implements Builder<List<Diff>> { private final List<Diff> diffs = new ArrayList<Diff>(); public List<Diff> build() { return Collections.unmodifiableList( new ArrayList<Diff>(diffs)); } public DiffBuilder append( final String fieldName, final boolean lhs, final boolean rhs) { if ( /* different */ ) { diffs.add( new Diff< Boolean >(fieldName) { public Boolean getLeft() { return Boolean .valueOf(lhs); } public Boolean getRight() { return Boolean .valueOf(rhs); } }); } return this ; } } The above example would avoid storing your primitives as Objects, for whatever small additional storage costs that would have incurred. $0.02, Matt
        Hide
        Duncan Jones added a comment -

        I've attached my latest work on this issue (both as individual files and a patch). I believe the DiffBuilder is now complete for the non-reflection use case.

        I will now work on adding reflection support. I welcome any comments on the attached.

        Show
        Duncan Jones added a comment - I've attached my latest work on this issue (both as individual files and a patch). I believe the DiffBuilder is now complete for the non-reflection use case. I will now work on adding reflection support. I welcome any comments on the attached.
        Hide
        Duncan Jones added a comment -

        Note: a few Javadoc comments need tidying. Also, I'm working to remove the requirement that the objects to compare must be non-null. It seemed like a good idea initially, but I now feel it will just restrict the usage of the classes, particularly in the reflection case.

        Show
        Duncan Jones added a comment - Note: a few Javadoc comments need tidying. Also, I'm working to remove the requirement that the objects to compare must be non-null. It seemed like a good idea initially, but I now feel it will just restrict the usage of the classes, particularly in the reflection case.
        Hide
        Duncan Jones added a comment - - edited

        Now attached is commons-lang3-LANG-637-complete.patch, which contains the complete implementation of the `DiffBuilder` with reflection-based methods, including necessary unit tests and full Javadoc comments.

        Show
        Duncan Jones added a comment - - edited Now attached is commons-lang3- LANG-637 -complete.patch, which contains the complete implementation of the `DiffBuilder` with reflection-based methods, including necessary unit tests and full Javadoc comments.
        Hide
        Matt Benson added a comment -

        Hi, Duncan. After more than a year, I have now reviewed this. I personally would like to see DiffList implement List<Diff<?>>, and then I would personally be in favor of committing this code. As long as it has been, I can understand if you've moved on and don't have time to make this change, in which case I can do it.

        Show
        Matt Benson added a comment - Hi, Duncan. After more than a year, I have now reviewed this. I personally would like to see DiffList implement List<Diff<?>>, and then I would personally be in favor of committing this code. As long as it has been, I can understand if you've moved on and don't have time to make this change, in which case I can do it.
        Hide
        Duncan Jones added a comment -

        Hi Matt, if you are willing to make the change that would be easier than for me to submit a patch etc. If you don't have the time I'm happy to submit one, however. Thanks for taking the time to review this!

        Show
        Duncan Jones added a comment - Hi Matt, if you are willing to make the change that would be easier than for me to submit a patch etc. If you don't have the time I'm happy to submit one, however. Thanks for taking the time to review this!
        Hide
        Benedikt Ritter added a comment - - edited

        I've set this to 3.3 so that we finally can include this. I plan to roll out 3.3 in early feburary.

        I'm not sure wether DiffList should implement the List interface. Random access doesn't seem to be a requirement here as well as the possibility to mutate the result. Maybe we're better off with DiffResult implements Iterable<Diff>. WDYT?

        Show
        Benedikt Ritter added a comment - - edited I've set this to 3.3 so that we finally can include this. I plan to roll out 3.3 in early feburary. I'm not sure wether DiffList should implement the List interface. Random access doesn't seem to be a requirement here as well as the possibility to mutate the result. Maybe we're better off with DiffResult implements Iterable<Diff> . WDYT?
        Hide
        ASF IRC Bot added a comment -

        Reply from mbenson on IRC:
        I would be okay with that as well (I had suggested List because the type was called DiffList).

        Show
        ASF IRC Bot added a comment - Reply from mbenson on IRC: I would be okay with that as well (I had suggested List because the type was called DiffList ).
        Hide
        Duncan Jones added a comment -

        I think Iterable is a good choice.

        Show
        Duncan Jones added a comment - I think Iterable is a good choice.
        Hide
        Duncan Jones added a comment -

        URL: http://svn.apache.org/r1561215
        Log:
        LANG-637: There should be a DifferenceBuilder with a ReflectionDifferenceBuilder implementation

        Added:
        commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/builder/Diff.java (with props)
        commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/builder/DiffBuilder.java (with props)
        commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/builder/DiffList.java (with props)
        commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/builder/Diffable.java (with props)
        commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/builder/DiffBuilderTest.java (with props)
        commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/builder/DiffListTest.java (with props)
        commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/builder/DiffTest.java (with props)
        Modified:
        commons/proper/lang/trunk/src/changes/changes.xml

        Show
        Duncan Jones added a comment - URL: http://svn.apache.org/r1561215 Log: LANG-637 : There should be a DifferenceBuilder with a ReflectionDifferenceBuilder implementation Added: commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/builder/Diff.java (with props) commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/builder/DiffBuilder.java (with props) commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/builder/DiffList.java (with props) commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/builder/Diffable.java (with props) commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/builder/DiffBuilderTest.java (with props) commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/builder/DiffListTest.java (with props) commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/builder/DiffTest.java (with props) Modified: commons/proper/lang/trunk/src/changes/changes.xml
        Hide
        Duncan Jones added a comment -

        URL: http://svn.apache.org/r1561225
        Log:
        Renamed DiffList to DiffResult as per discussion in LANG-637.

        Show
        Duncan Jones added a comment - URL: http://svn.apache.org/r1561225 Log: Renamed DiffList to DiffResult as per discussion in LANG-637 .

          People

          • Assignee:
            Duncan Jones
            Reporter:
            Eric Lewis
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development