Minor improvements in code complexity with early returns and early validation #116
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…moving $keyPair initialization to the declaration
…y with early returns
|
Are you planning to fix this PR? |
|
@l0gicgate - I will update tests to make sure we improve the coverage. Previously, the entirety of the test-covered code was inside a big |
|
@Ayesh I’ll wait until coverage is back to 100% to merge! Thanks |
During a refacotiring of this method, the early-return `if` block was not tested properly. There is a new test \Slim\Tests\Csrf\GuardTest::testNotEnforceStorageLimitWithArrayWhenLimitIsZero that makes sure the storage array provided remains the same after a token is processed when the storage limit is set to zero, which should trigger the early-return `if` block, this adding more coverage.
|
@l0gicgate - I added a new test to cover the branch that fell behind in tests. Coverage back to 100% now. Thanks. |
l0gicgate
approved these changes
May 6, 2020
adriansuter
approved these changes
May 6, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi there,
I noticed a few minor improvements that we could make in the
Guardclass.Guard::__constructhas a call that it initializes$this->keyPair = null. This can be moved to the property declaration without changing the functionality.$strength < 16check, which can be moved to the top as well, so the later calls are not executed if we have to throw an exception. The early calls do not mutate state.Guard::validateToken()andGuard::enforceStorageLimit()methods have nestedifcalls, which can be removed in favor of early returns.Fingers crossed this can be merged, and I'm open to make any further changes if necessary. Thank you for your time.