Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

refactor(equalsFunction): divide internal implemenation #13958

Closed
wants to merge 1 commit into from

Conversation

m-amr
Copy link
Contributor

@m-amr m-amr commented Feb 6, 2016

extract complex logic to named methods
fixes #13957

@m-amr m-amr force-pushed the refactor_equals_function branch 7 times, most recently from e8c54bb to 7f8c3a2 Compare February 6, 2016 12:05
@m-amr m-amr changed the title refactor(equalsFunction): divid internal implemenation refactor(equalsFunction): divide internal implemenation Feb 6, 2016
@m-amr m-amr force-pushed the refactor_equals_function branch 2 times, most recently from 427ef56 to 2105286 Compare February 6, 2016 12:33
extract complex logic to named methods
fixes angular#13957
@gkalpak gkalpak added this to the Backlog milestone Feb 6, 2016
@gkalpak
Copy link
Member

gkalpak commented Feb 6, 2016

I don't feel refactoring everything out into functions makes it much more clear (if at all).
Also, note this may have perf implications, considering equals is used in some critical paths.

@lgalfaso
Copy link
Contributor

lgalfaso commented Feb 6, 2016

Hi, thanks for the PR! Unfortunately I fully agree with @gkalpak and I find that this change does not make angular.equals any simpler to understand.

@lgalfaso lgalfaso closed this Feb 6, 2016
@m-amr
Copy link
Contributor Author

m-amr commented Feb 6, 2016

@gkalpak @lgalfaso I respect your decision
but the equals was a very complex function, with a lot of cases but now after
refactoring it just.

  function equals(o1, o2) {
      if (areBothEqualValue(o1, o2)) return true;
      if (isAnyOneHasValueEqualNull(o1, o2)) return false;
      if (areBothValuesEqualNaN(o1, o2)) return true;
      if (areBothObjectsEqual(o1, o2)) return true;
      return false;
  }

I didn't change any logic or and all cases are checked in the same order as before.
I refactored it by extracting functions with a meaningful names. to make it tell the story to reader
now it clear that we are doing the following steps

1 - check if their values are equal using areBothEqualValue(o1, o2) function
2 - check if one of them is null using AnyOneHasValueEqualNull(o1, o2) function
3 - check if their values equal NAN using areBothValuesEqualNaN(o1, o2) function
4 - check if two objects are using equal areBothObjectsEqual(o1, o2) function

I think using a meaningful function names make it easier to understand and better than adding
inline code comments that have the risk to be not updated with the code.
based on http://www.amazon.com/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882/ref=sr_1_1?ie=UTF8&qid=1454769207&sr=8-1&keywords=clean+code
chapter 4 (Comments )

I think it now easier to understand and easier to change too. and each function have just one responsibility is compare a single case. i hope you check it again.
it is a very big function and you need to follow all the paths to understand what is happening and has a lot of nested conditions which make it harder to change.

function equals(o1, o2) {
  if (o1 === o2) return true;
  if (o1 === null || o2 === null) return false;
  if (o1 !== o1 && o2 !== o2) return true; // NaN === NaN
  var t1 = typeof o1, t2 = typeof o2, length, key, keySet;
  if (t1 == t2 && t1 == 'object') {
    if (isArray(o1)) {
      if (!isArray(o2)) return false;
      if ((length = o1.length) == o2.length) {
        for (key = 0; key < length; key++) {
          if (!equals(o1[key], o2[key])) return false;
        }
        return true;
      }
    } else if (isDate(o1)) {
      if (!isDate(o2)) return false;
      return equals(o1.getTime(), o2.getTime());
    } else if (isRegExp(o1)) {
      if (!isRegExp(o2)) return false;
      return o1.toString() == o2.toString();
    } else {
      if (isScope(o1) || isScope(o2) || isWindow(o1) || isWindow(o2) ||
        isArray(o2) || isDate(o2) || isRegExp(o2)) return false;
      keySet = createMap();
      for (key in o1) {
        if (key.charAt(0) === '$' || isFunction(o1[key])) continue;
        if (!equals(o1[key], o2[key])) return false;
        keySet[key] = true;
      }
      for (key in o2) {
        if (!(key in keySet) &&
            key.charAt(0) !== '$' &&
            isDefined(o2[key]) &&
            !isFunction(o2[key])) return false;
      }
      return true;
    }
  }
  return false;
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor complex angular equals function
4 participants