From eed2c319a62b73ee6e5e50281ec78cab0d54c712 Mon Sep 17 00:00:00 2001 From: Jean-Louis Rigau Date: Fri, 14 Jun 2013 02:46:57 +0200 Subject: [PATCH] Review on new feature compareToOnly --- .../de/danielbechler/diff/BeanDiffer.java | 37 +++++----- .../de/danielbechler/diff/Configuration.java | 53 +++++++------- .../java/de/danielbechler/diff/Instances.java | 9 +-- .../de/danielbechler/diff/NodeInspector.java | 4 +- .../java/de/danielbechler/util/Classes.java | 6 ++ .../de/danielbechler/util/Comparables.java | 38 ++++++++++ .../danielbechler/diff/BeanDifferShould.java | 40 ++++------- .../danielbechler/diff/ConfigurationTest.java | 66 ++++++++--------- .../diff/mock/ObjectImplementComparable.java | 9 --- .../diff/mock/ObjectWithCompareTo.java | 70 +++++++++++++++++++ .../mock/ObjectWithStringAndCompareTo.java | 49 +++++++++++++ 11 files changed, 255 insertions(+), 126 deletions(-) create mode 100644 src/main/java/de/danielbechler/util/Comparables.java delete mode 100644 src/test/java/de/danielbechler/diff/mock/ObjectImplementComparable.java create mode 100644 src/test/java/de/danielbechler/diff/mock/ObjectWithCompareTo.java create mode 100644 src/test/java/de/danielbechler/diff/mock/ObjectWithStringAndCompareTo.java diff --git a/src/main/java/de/danielbechler/diff/BeanDiffer.java b/src/main/java/de/danielbechler/diff/BeanDiffer.java index e506a07b..430e7289 100644 --- a/src/main/java/de/danielbechler/diff/BeanDiffer.java +++ b/src/main/java/de/danielbechler/diff/BeanDiffer.java @@ -72,18 +72,18 @@ else if (instances.hasBeenRemoved()) private void compareUsingAppropriateMethod(final Node beanNode, final Instances instances) { - if (nodeInspector.isIntrospectible(beanNode)) + if (nodeInspector.isCompareToOnly(beanNode)) { - compareUsingIntrospection(beanNode, instances); - } - else if (nodeInspector.isComparable(beanNode)) - { - compareUsingCompare(beanNode, instances); + compareUsingCompareTo(beanNode, instances); } else if (nodeInspector.isEqualsOnly(beanNode)) { compareUsingEquals(beanNode, instances); } + else if (nodeInspector.isIntrospectible(beanNode)) + { + compareUsingIntrospection(beanNode, instances); + } } private void compareUsingIntrospection(final Node beanNode, final Instances beanInstances) @@ -100,6 +100,19 @@ private void compareUsingIntrospection(final Node beanNode, final Instances bean } } + @SuppressWarnings({"MethodMayBeStatic"}) + private void compareUsingCompareTo(final Node beanNode, final Instances instances) + { + if (instances.areEqualByComparison()) + { + beanNode.setState(Node.State.UNTOUCHED); + } + else + { + beanNode.setState(Node.State.CHANGED); + } + } + @SuppressWarnings({"MethodMayBeStatic"}) private void compareUsingEquals(final Node beanNode, final Instances instances) { @@ -113,18 +126,6 @@ private void compareUsingEquals(final Node beanNode, final Instances instances) } } - private void compareUsingCompare(final Node beanNode, final Instances instances) - { - if (instances.areComparable()) - { - beanNode.setState(Node.State.UNTOUCHED); - } - else - { - beanNode.setState(Node.State.CHANGED); - } - } - @TestOnly void setIntrospector(final Introspector introspector) { diff --git a/src/main/java/de/danielbechler/diff/Configuration.java b/src/main/java/de/danielbechler/diff/Configuration.java index e3621cce..5396a2ea 100644 --- a/src/main/java/de/danielbechler/diff/Configuration.java +++ b/src/main/java/de/danielbechler/diff/Configuration.java @@ -77,9 +77,8 @@ public enum PrimitiveDefaultValueMode private final Collection includedProperties = new HashSet(10); private final Collection excludedProperties = new HashSet(10); private final Collection equalsOnlyProperties = new LinkedHashSet(10); + private final Collection> compareToOnlyTypes = new LinkedHashSet>(10); private final Collection> equalsOnlyTypes = new LinkedHashSet>(10); - private final Collection> comparableTypes = new LinkedHashSet>(10); - private final Map, Comparator> compareTypes = new HashMap, Comparator>(10); private boolean returnUnchangedNodes = false; private boolean returnIgnoredNodes = false; private boolean returnCircularNodes = true; @@ -126,24 +125,18 @@ public Configuration withoutProperty(final PropertyPath propertyPath) return this; } + public Configuration withCompareToOnlyType(final Class type) + { + this.compareToOnlyTypes.add(type); + return this; + } + public Configuration withEqualsOnlyType(final Class type) { this.equalsOnlyTypes.add(type); return this; } - public Configuration withComparableType(final Class type) - { - this.comparableTypes.add(type); - return this; - } - - public Configuration withCompareType(final Class type, Comparator comparator) - { - this.compareTypes.put(type, comparator); - return this; - } - public Configuration withEqualsOnlyProperty(final PropertyPath propertyPath) { this.equalsOnlyProperties.add(propertyPath); @@ -271,6 +264,23 @@ public boolean isExcluded(final Node node) return false; } + public boolean isCompareToOnly(final Node node) + { + final Class propertyType = node.getType(); + if (propertyType != null) + { + if (compareToOnlyTypes.contains(propertyType) && Comparable.class.isAssignableFrom(propertyType)) + { + return true; + } + if (Classes.isComparableType(propertyType)) + { + return true; + } + } + return false; + } + public boolean isEqualsOnly(final Node node) { final Class propertyType = node.getType(); @@ -300,7 +310,6 @@ public boolean isEqualsOnly(final Node node) return false; } - public boolean isReturnable(final Node node) { if (node.isIgnored()) @@ -326,21 +335,9 @@ else if (node.hasChildren()) return true; } - public boolean isComparable(Node node) { - return comparableTypes.contains(node.getType()) && Comparable.class.isAssignableFrom(node.getType()); - } - public boolean isIntrospectible(final Node node) { - if (isEqualsOnly(node)) - { - return false; - } - else if (isComparable(node)) - { - return false; - } - else if (node.isAdded()) + if (node.isAdded()) { return returnChildrenOfAddedNodes; } diff --git a/src/main/java/de/danielbechler/diff/Instances.java b/src/main/java/de/danielbechler/diff/Instances.java index 28ba0208..0c02765f 100644 --- a/src/main/java/de/danielbechler/diff/Instances.java +++ b/src/main/java/de/danielbechler/diff/Instances.java @@ -25,6 +25,7 @@ import java.util.*; import static de.danielbechler.util.Objects.*; +import static de.danielbechler.util.Comparables.*; /** @author Daniel Bechler */ @SuppressWarnings({"UnusedDeclaration"}) @@ -172,15 +173,11 @@ public boolean areEqual() return isEqual(base, working); } - public boolean areComparable() + public boolean areEqualByComparison() { - return areComparable((Comparable) base, (Comparable) working); + return isEqualByComparison((Comparable) base, (Comparable) working); } - private boolean areComparable(Comparable base, Comparable working) { - return base.compareTo(working) == 0; - } - public boolean areSame() { return working == base; diff --git a/src/main/java/de/danielbechler/diff/NodeInspector.java b/src/main/java/de/danielbechler/diff/NodeInspector.java index a9bcf664..9744eb4f 100644 --- a/src/main/java/de/danielbechler/diff/NodeInspector.java +++ b/src/main/java/de/danielbechler/diff/NodeInspector.java @@ -27,12 +27,12 @@ interface NodeInspector boolean isExcluded(Node node); + boolean isCompareToOnly(Node node); + boolean isEqualsOnly(Node node); boolean isReturnable(Node node); - boolean isComparable(Node node); - /** * @return Returns true if the object represented by the given node should be compared via * introspection. It must always return false if {@link diff --git a/src/main/java/de/danielbechler/util/Classes.java b/src/main/java/de/danielbechler/util/Classes.java index 72c36a2c..870af819 100644 --- a/src/main/java/de/danielbechler/util/Classes.java +++ b/src/main/java/de/danielbechler/util/Classes.java @@ -19,6 +19,7 @@ import org.slf4j.*; import java.lang.reflect.*; +import java.math.BigDecimal; import java.net.*; import java.util.*; @@ -98,6 +99,11 @@ public static boolean isSimpleType(final Class clazz) Class.class.equals(clazz); } + public static boolean isComparableType(final Class clazz) + { + return BigDecimal.class.equals(clazz); + } + public static T freshInstanceOf(final Class clazz) { if (clazz == null) diff --git a/src/main/java/de/danielbechler/util/Comparables.java b/src/main/java/de/danielbechler/util/Comparables.java new file mode 100644 index 00000000..2cc11976 --- /dev/null +++ b/src/main/java/de/danielbechler/util/Comparables.java @@ -0,0 +1,38 @@ +/* + * Copyright 2012 Daniel Bechler + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package de.danielbechler.util; + +/** @author Daniel Bechler */ +public class Comparables +{ + private Comparables() + { + } + + public static > boolean isEqualByComparison(final T a, final T b) + { + if (a != null) + { + return a.compareTo(b) == 0; + } + else if (b != null) + { + return b.compareTo(a) == 0; + } + return true; + } +} diff --git a/src/test/java/de/danielbechler/diff/BeanDifferShould.java b/src/test/java/de/danielbechler/diff/BeanDifferShould.java index 67da819e..9aaeacdf 100644 --- a/src/test/java/de/danielbechler/diff/BeanDifferShould.java +++ b/src/test/java/de/danielbechler/diff/BeanDifferShould.java @@ -24,8 +24,6 @@ import org.mockito.Mock; import org.testng.annotations.*; -import java.math.BigDecimal; - import static de.danielbechler.diff.node.NodeAssertions.assertThat; import static java.util.Arrays.*; import static org.hamcrest.MatcherAssert.assertThat; @@ -99,6 +97,18 @@ public void ignore_ignored_properties() assertThat(node).self().hasState(Node.State.IGNORED); } + @Test + public void compare_bean_via_compare_to() + { + final ObjectWithCompareTo working = new ObjectWithCompareTo("foo", "ignore"); + final ObjectWithCompareTo base = new ObjectWithCompareTo("foo", "ignore this too"); + configuration.withCompareToOnlyType(ObjectWithCompareTo.class); + + final Node node = differ.compare(Node.ROOT, Instances.of(working, base)); + + assertThat(node).self().isUntouched(); + } + @Test public void compare_bean_via_equals() { @@ -111,32 +121,6 @@ public void compare_bean_via_equals() assertThat(node).self().isUntouched(); } - @Test - public void compare_bean_via_comparable_when_same_value() - { - final BigDecimal working = new BigDecimal("1.0"); - final BigDecimal base = new BigDecimal("1.00"); - configuration.withComparableType(BigDecimal.class); - - final Node node = differ.compare(Node.ROOT, Instances.of(working, base)); - - assertThat(node).self().isUntouched(); - } - - @Test - public void compare_bean_via_comparable_when_different_value() - { - final BigDecimal working = new BigDecimal("1.0"); - final BigDecimal base = new BigDecimal("1.01"); - configuration.withComparableType(BigDecimal.class); - - final Node node = differ.compare(Node.ROOT, Instances.of(working, base)); - - assertThat(node).self().hasChanges(); - } - - - @Test public void compare_bean_via_introspection_and_delegate_comparison_of_properties() { diff --git a/src/test/java/de/danielbechler/diff/ConfigurationTest.java b/src/test/java/de/danielbechler/diff/ConfigurationTest.java index 92548578..ae88fa42 100644 --- a/src/test/java/de/danielbechler/diff/ConfigurationTest.java +++ b/src/test/java/de/danielbechler/diff/ConfigurationTest.java @@ -24,6 +24,8 @@ import org.mockito.stubbing.*; import org.testng.annotations.*; +import java.math.BigDecimal; + import static org.fest.assertions.api.Assertions.assertThat; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.core.Is.*; @@ -98,6 +100,35 @@ public void testIsIgnoredWithCategory() throws Exception assertThat(configuration.isIgnored(node), is(true)); } + @SuppressWarnings({"unchecked"}) + @Test + public void testIsCompareToOnlyWithConfiguredPropertyType() throws Exception + { + final Class aClass = ObjectWithStringAndCompareTo.class; + when(node.getType()).thenReturn(aClass); + configuration.withCompareToOnlyType(aClass); + assertThat(configuration.isCompareToOnly(node), is(true)); + } + + @SuppressWarnings({"unchecked"}) + @Test + public void testIsCompareToOnlyWithConfiguredPropertyTypeNotComparable() throws Exception + { + final Class aClass = ObjectWithString.class; + when(node.getType()).thenReturn(aClass); + configuration.withCompareToOnlyType(aClass); + assertThat(configuration.isCompareToOnly(node), is(false)); + } + + @SuppressWarnings({"unchecked"}) + @Test + public void testIsCompareToOnlyWithComparableType() throws Exception + { + final Class aClass = BigDecimal.class; + when(node.getType()).thenReturn(aClass); + assertThat(configuration.isCompareToOnly(node), is(true)); + } + @Test public void testIsEqualsOnlyWithConfiguredPropertyPath() throws Exception { @@ -142,13 +173,6 @@ public void testIsEqualsOnlyWithTypeThatShouldNotBeComparedUsingEquals() throws assertThat(configuration.isEqualsOnly(node), is(false)); } - @Test - public void testIsIntrospectibleWithEqualsOnlyNodeReturnsFalse() - { - when(node.getType()).then(returnClass(String.class)); - assertThat(configuration.isIntrospectible(node)).isFalse(); - } - @Test public void testIsIntrospectibleWithUntouchedNonEqualsOnlyNodeReturnsFalse() { @@ -189,34 +213,6 @@ public void testIsIntrospectibleReturnsFalseForRemovedNodeIfChildrenOfRemovedNod assertThat(configuration.isIntrospectible(node)).isFalse(); } - @Test - public void testIsComparableWithAComparableObjectAddedInConfiguration() throws Exception - { - final Class aClass = ObjectImplementComparable.class; - this.configuration.withComparableType(aClass); - when(node.getType()).thenReturn(aClass); - - assertThat(this.configuration.isComparable(node), is(true)); - } - - @Test - public void testIsComparableWithAComparableObjectNotAddedInConfiguration() throws Exception - { - final Class aClass = ObjectImplementComparable.class; - when(node.getType()).thenReturn(aClass); - - assertThat(this.configuration.isComparable(node), is(false)); - } - - @Test - public void testIsNotComparableWithANotComparableObject() throws Exception - { - final Class aClass = Object.class; - when(node.getType()).thenReturn(aClass); - - assertThat(this.configuration.isComparable(node), is(false)); - } - @SuppressWarnings({"TypeMayBeWeakened"}) private static Answer> returnClass(final Class aClass) { diff --git a/src/test/java/de/danielbechler/diff/mock/ObjectImplementComparable.java b/src/test/java/de/danielbechler/diff/mock/ObjectImplementComparable.java deleted file mode 100644 index 9c785dbb..00000000 --- a/src/test/java/de/danielbechler/diff/mock/ObjectImplementComparable.java +++ /dev/null @@ -1,9 +0,0 @@ -package de.danielbechler.diff.mock; - -public class ObjectImplementComparable implements Comparable { - - public int compareTo(ObjectImplementComparable o) { - return 0; - } - -} diff --git a/src/test/java/de/danielbechler/diff/mock/ObjectWithCompareTo.java b/src/test/java/de/danielbechler/diff/mock/ObjectWithCompareTo.java new file mode 100644 index 00000000..2e5baf92 --- /dev/null +++ b/src/test/java/de/danielbechler/diff/mock/ObjectWithCompareTo.java @@ -0,0 +1,70 @@ +/* + * Copyright 2012 Daniel Bechler + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package de.danielbechler.diff.mock; + +import de.danielbechler.util.Assert; + +/** @author Daniel Bechler */ +public class ObjectWithCompareTo implements Comparable +{ + private final String key; + + private String value; + private ObjectWithCompareTo item; + + public ObjectWithCompareTo(final String key) + { + Assert.hasText(key, "key"); + this.key = key; + } + + public ObjectWithCompareTo(final String key, final String value) + { + this(key); + this.value = value; + } + + public String getKey() + { + return key; + } + + public String getValue() + { + return value; + } + + public void setValue(final String value) + { + this.value = value; + } + + public ObjectWithCompareTo getItem() + { + return item; + } + + public ObjectWithCompareTo setItem(final ObjectWithCompareTo item) + { + this.item = item; + return this; + } + + public int compareTo(ObjectWithCompareTo objectWithCompareTo) { + return this.key.compareTo(objectWithCompareTo.key); + } +} diff --git a/src/test/java/de/danielbechler/diff/mock/ObjectWithStringAndCompareTo.java b/src/test/java/de/danielbechler/diff/mock/ObjectWithStringAndCompareTo.java new file mode 100644 index 00000000..f4e90c0d --- /dev/null +++ b/src/test/java/de/danielbechler/diff/mock/ObjectWithStringAndCompareTo.java @@ -0,0 +1,49 @@ +/* + * Copyright 2012 Daniel Bechler + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package de.danielbechler.diff.mock; + +/** @author Daniel Bechler */ +@SuppressWarnings({ + "UnusedDeclaration" +}) +public class ObjectWithStringAndCompareTo implements Comparable +{ + private String value; + + public ObjectWithStringAndCompareTo() + { + } + + public ObjectWithStringAndCompareTo(final String value) + { + this.value = value; + } + + public String getValue() + { + return value; + } + + public void setValue(final String value) + { + this.value = value; + } + + public int compareTo(ObjectWithStringAndCompareTo objectWithStringAndCompareTo) { + return this.value.compareTo(objectWithStringAndCompareTo.value); + } +}