Skip to content

Fix the Array Comparison bug in outputs.js #6662

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Fix the Array Comparison bug in outputs.js #6662

wants to merge 1 commit into from

Conversation

nikhilkalburgi
Copy link

Signed-off-by: nikhilkalburgi [email protected]

Resolves: #6660

PR Checklist

  • npm run lint passes
  • [Inline documentation] is included / updated
  • [Unit tests] are included / updated

Signed-off-by: nikhilkalburgi <[email protected]>

Resolves: #6660
Copy link

@ayushanand308 ayushanand308 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me but this might fail if order of properties is different in two arrays even if though both might be equal.

@nikhilkalburgi
Copy link
Author

Hi @ayushanand308,
What if we sort them and check?

JSON.stringify(this.ingredients.shapes[f].slice().sort()) !== JSON.stringify([include].slice().sort())

This will create a copy , sort then stringify to compare

@limzykenneth
Copy link
Member

@nikhilkalburgi That will be very inefficient and considering this will run on every frame, may significantly impact performance if the drawings get complex enough.

@nikhilkalburgi
Copy link
Author

Hi @limzykenneth,
Do you think that the initial condition is necessary as it always returns true? How can it get resolved?

@limzykenneth
Copy link
Member

In the subsequent for loop there is already a check for existence of the element in the list so the check before that is either not needed (ie. be an else instead of else if or be something that can be completed very quickly. I think for this case not having that earlier check should be fine and is how is has worked until now effectively. The loop can be optimized a bit by breaking early as well.

@HusainModiwala
Copy link

maybe here we can use isEqual function of lodash library to compare them.

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

Successfully merging this pull request may close these issues.

this.ingredients.shapes[f] !== [include] always returns true
4 participants