Skip to content
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

Reduce method complexity of setParameters() #1634

Merged
merged 3 commits into from
Jul 4, 2019

Conversation

gverni
Copy link
Contributor

@gverni gverni commented Jun 25, 2019

Created two functions:

  • setDefaultInputValidators(): to assign default input validators
  • validateCustomTargetElement(): to check if the target element is a valid one

Also:

  • Moved warning message to a constant outside of function
  • Code clean-up

Fix #1490

Created two functions:
* setDefaultInputValidators(): to assign default input  validators
* validateCustomTargetElement(): to check if the target element
is a valid one

Also:
* Moved warning message to a constant outside of function
* Code clean-up
@gverni
Copy link
Contributor Author

gverni commented Jun 25, 2019

Codeclimate integration reports only 1 issue fixed ("Function setParameters has 34 lines of code (exceeds 25 allowed). Consider refactoring."). I'm checking about method complexity.

@coveralls
Copy link

coveralls commented Jun 25, 2019

Pull Request Test Coverage Report for Build 5068

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 13 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.02%) to 90.656%

Files with Coverage Reduction New Missed Lines %
dist/sweetalert2.js 13 90.66%
Totals Coverage Status
Change from base Build 5061: 0.02%
Covered Lines: 1256
Relevant Lines: 1331

💛 - Coveralls

The previous logic was redundant because was returning always true
@gverni
Copy link
Contributor Author

gverni commented Jun 26, 2019

In 8e989d2 I removed the if statement at the end of the setParameters() function. The reason is that the condition:

!oldPopup ||
// If the model target has changed, refresh the popup
(oldPopup && targetElement && oldPopup.parentNode !== targetElement.parentNode)

always returned true (or I didn't understand the logic). In fact oldPopup.parentNode is the Swal2 container (i.e. swal2-container). The only condition when this condition is true is if the targetElement is actually the popup itself.... which is a use case I cannot think of.

Replacing the right side of the OR with a more correct:

oldPopup.parentNode.parentNode !== targetElement

makes 11 test fail:

✗ toast aria attributes
✗ grow fullscreen
✗ Modal positions
✗ Toast positions
✗ Toast positions with target
✗ should throw console error about invalid type
✗ container should have .swal2-rtl in case of
✗ container should have .swal2-rtl in case of
✗ should show the popup with OK button in case of empty object passed as an argument
✗ JS element as html param
✗ onBeforeOpen

I can analyze why the above fails (likely cause is that the dom is not updated unless the target element is different), but before doing so, @limonte @zenflow do you agree with my analysis that that condition is always true? Or is there some obscure use case that I cannot think of that needs that check?

@gverni gverni marked this pull request as ready for review June 27, 2019 11:04
@gverni gverni requested a review from limonte July 4, 2019 14:20
@limonte limonte force-pushed the fix/method-complexity-setParameters branch from cf788a8 to ba73581 Compare July 4, 2019 15:22
Copy link
Member

@limonte limonte left a comment

Choose a reason for hiding this comment

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

I can analyze why the above fails (likely cause is that the dom is not updated unless the target element is different), but before doing so, do you agree with my analysis that that condition is always true? Or is there some obscure use case that I cannot think of that needs that check?

This is the leftover from the previous versions when we were reusing the same DOM node for all popups. Now we're creating the new DOM node (.swal2-container) for each popup.

@limonte
Copy link
Member

limonte commented Jul 4, 2019

Thank you @gverni!

I reverted the loaderOnConfirmMessage variable, other than that all is good 🚀

@limonte limonte merged commit 603e9fa into master Jul 4, 2019
@limonte limonte deleted the fix/method-complexity-setParameters branch July 4, 2019 15:30
@limonte
Copy link
Member

limonte commented Jul 8, 2019

🎉 This PR is included in version 8.13.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

Fix "method_complexity" issue in src/utils/setParameters.js
3 participants