search

slimphp / Slim-Csrf

Slim Framework CSRF protection middleware
MIT License
338 stars 58 forks source link

Minor improvements in code complexity with early returns and early validation #116

Closed Ayesh closed 4 years ago

Ayesh commented 4 years ago

Hi there, I noticed a few minor improvements that we could make in the Guard class.

  1. Guard::__construct has a call that it initializes $this->keyPair = null. This can be moved to the property declaration without changing the functionality.
  2. There is one check for $strength < 16 check, 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.
  1. Guard::validateToken() and Guard::enforceStorageLimit() methods have nested if calls, 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.

l0gicgate commented 4 years ago

Are you planning to fix this PR?

Ayesh commented 4 years ago

@l0gicgate - I will update tests to make sure we improve the coverage. Previously, the entirety of the test-covered code was inside a big if statement, which made the coverage reach 100%. But now, the logic is inverted and there is an early-return, which there is no longer a test for. I will push another commit with tests. Thank you.

l0gicgate commented 4 years ago

@ayesh I’ll wait until coverage is back to 100% to merge! Thanks

Ayesh commented 4 years ago

@l0gicgate - I added a new test to cover the branch that fell behind in tests. Coverage back to 100% now. Thanks.