Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I wrote a utility method to override equals() using reflection. This works fine, but I wonder if this code will pass all the tests.

public static boolean checkObjectsEqualityFromInstance(Object currentObj,Object otherObj) throws Exception{
      if(currentObj==null || otherObj==null){
          return false;
      }
      else if(otherObj.getClass()!=null&&currentObj.getClass()!=null&&!currentObj.getClass().isInstance(otherObj)){
          return false;
      }
      boolean result =true;
        Field[] fields = otherObj.getClass().getDeclaredFields();
        Field[] currentObjFields = currentObj.getClass().getDeclaredFields();

        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<>();
            for (Field field : fields) {
                field.setAccessible(true); 

                Object value = field.get(otherObj);
                attriButeNameValueMap.put(field.getName(), value);
            }
            for (Field field : currentObjFields) {
                field.setAccessible(true); 

                Object value = field.get(currentObj);
                if(attriButeNameValueMap.containsKey(field.getName())){
                    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()));
                    }
                    else if(value instanceof Float){
                        result=result&&areEqual((Float)value, (Float)attriButeNameValueMap.get(field.getName()));
                    }
                    else if(value instanceof Double){
                        result=result&&areEqual((Double)value, (Double)attriButeNameValueMap.get(field.getName()));
                    }
                    else if(value instanceof Object[]){
                        result=result&&  Arrays.equals((Object[])value, (Object[])attriButeNameValueMap.get(field.getName()));
                    }
                    else{
                        result=result&&areEqual(value, attriButeNameValueMap.get(field.getName()));

                    }
                    if(!result){
                        return result;
                    }
                }
                else{
                    return false;
                }
            }

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

The areEqual() methods are overloaded for different types.

share|improve this question
1  
This code doesn't deal properly with fields that are arrays of primitives. –  bowmore Mar 28 at 9:57
1  
There is no need for a special case for box classes like Boolean, Character, Double... they can simply be treated the same as Object. –  bowmore Mar 28 at 10:33
    
I agree thanks.... i am waiting for more comments so that i can remember points for future, when i code again :) –  khushnood Mar 28 at 12:35

2 Answers 2

up vote 8 down vote accepted
  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

    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.)

  2. 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).

  3. 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;
    
  4. Formatting is not consistent. Indentation sometimes is two spaces, sometimes four.

  5.   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)

  6. 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.

share|improve this answer
1  
I really liked the way you reviewed my code and commented on each mistake.This was nice experience for me on this site ... :) thanks bro. –  khushnood Mar 29 at 17:46
    
@khushnood: You're welcome! I'm happy that you found it helpful :) –  palacsint Mar 29 at 20:26
1  
@khushnood: One more thing: If you like the site help graduating. I'm quite sure that you can find other useful questions and answers on the site to vote on. –  palacsint Mar 29 at 22:14
  1. You should check if the references are equal first to save a whole lot of comparing when there is no need to.

  2. If both objects are null your method will return false which is probably unexpected.

  3. You call getClass() several times on these objects - It should only be called once for each object and then stored in a local variable.

  4. There is a typo in attriButeNameValueMap - should be attributeNameValueMap

  5. You call field.getName() several times - again just call once per loop and store in a local variable.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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