Return to Answer

• 1

• 30.3k
• 9
• 82
• 157

1. It ignores values in superclasses. You might want to check that too.

Anyway, don't reinvent the weel, there is a library for that! I guess EqualsBuilder.reflectionEquals from Apache Commons Lang does exactly what you want. It also has solutions to corner cases, like

• transient fields (since they are likely derived fields, and not part of the value of the Object) in a configurable way
• generated fields with $ (for inner classes).

You can also exclude field names from the comparison.

It's open-source, you can check the source for further details.

See also: Effective Java, 2nd edition, Item 47: Know and use the libraries (The author mentions only the JDK's built-in libraries but I think the reasoning could be true for other libraries too.)

1. The else keyword is unnecessary here:

    if(currentObj==null || otherObj==null){
    return false;
    }
    else if(otherObj.getClass()!=null&&currentObj.getClass()!=null&&!currentObj.getClass().isInstance(otherObj)){
    return false;
    }
   

The following is the same:

 if (currentObj == null || otherObj == null){
 return false;
 }
 if (otherObj.getClass() != null && currentObj.getClass() != null && 
 !currentObj.getClass().isInstance(otherObj)) {
 return false;
 }
 

It's called guard clause. I've also used a little bit more spacing to make it readable (easier separaton of values, comparisons, method calls).

1. Guard clauses would make this easier to follow too:

 if(value instanceof Boolean){
 result=areEqual((Boolean)value, (Boolean)attriButeNameValueMap.get(field.getName()));
 }
 else if(value instanceof Character){
 result=result&&areEqual((Character)value, (Character)attriButeNameValueMap.get(field.getName()));
 }
 else if(value instanceof Long){
 result=result&&areEqual((Long)value, (Long)attriButeNameValueMap.get(field.getName()));
 }
 ...
 if(!result){
 return result;
 }

I've used a few exaplanatory variable to remove some duplication:

 Object value = field.get(currentObj);
 String fieldName = field.getName();
 if(attriButeNameValueMap.containsKey(fieldName)){
 Object otherFieldValue = attriButeNameValueMap.get(fieldName);
 if (value instanceof Boolean) {
 if (!areEqual((Boolean) value, (Boolean) otherFieldValue)) {
 return false;
 }
 } 
 if (value instanceof Character) {
 if (!areEqual((Character) value, (Character) otherFieldValue)) {
 return false;
 }
 }

Note that it makes superfluous the result variable. The last line of the method could be simply the following, since its never modified anymore:

return true;

1. Formatting is not consistent. Indentation sometimes is two spaces, sometimes four.

 Map<String, Object> attriButeNameValueMap=null; 
 /*This map to store *property name and its value of any of object in my calse the otherObj */
 try {
 attriButeNameValueMap=new HashMap<>();

B should be lowercase in the variable name (I guess it's just a typo) and you could declare it inside the try block:

I'd also rename it to otherObjectAttributeValues to make the comment unnecessary:

 try {
 Map<String, Object> otherObjectAttributeValues = new HashMap<>();

(Effective Java, Second Edition, Item 45: Minimize the scope of local variables)

1. Consistent formatting would be great here too:

 } catch (IllegalArgumentException | IllegalAccessException e) {
 // TODO Auto-generated catch block
 e.printStackTrace();
 throw e;
 }

Furthermore, TODO comments does not suggest professional work. Fix that and remove the comment.

I'd throw out the IllegalAccessException or wrap it into a RuntimeException. The IllegalArgumentException already a RuntimeException, so you don't have to put it into the method signature.