search

vt-middleware / passay

Password policy enforcement for Java.
http://www.passay.org
Other
282 stars 64 forks source link

Misc improvements #90

Closed amichair closed 5 years ago

amichair commented 5 years ago

Here are a bunch of little improvements. You can pick whichever ones you like for merging, I'd be happy to rebase, reorder etc. as necessary. I'll also probably add to and update this branch, but I'm putting it here as a starting point for review and discussion on what might be accepted. I can also separate any of the commits/topics into a separate branch/PR, but it's a bunch of work rebasing them every time, so I'd rather do that only if necessary and if I know something will be accepted for merging... in short, feedback and/or merging is welcome. Thanks!

amichair commented 5 years ago

Added some more improvements.

btw, I'm not sure how you deal with removal of methods and inner classes... the build fails if they are deleted, so I marked them as deprecated for now and I hope you'll remove them when possible.

dfish3r commented 5 years ago

Added some more improvements.

btw, I'm not sure how you deal with removal of methods and inner classes... the build fails if they are deleted, so I marked them as deprecated for now and I hope you'll remove them when possible.

Versioning policy is that revision releases must be API compatible. I wasn't planning another 1.4.x release. Unless someone requests one the next release will likely be 2.0.0 on Java 11. In which case, the deprecated methods will be removed. Marking them deprecated is fine for now.

dfish3r commented 5 years ago

Overall looks good. Made a few comments inline.

amichair commented 5 years ago

btw I also wanted to change the multiple identical implementations of ArraySorter.sort(String[]) into a default method in the interface, but that versioning api checker fails the build on it (even though it should be backwards compatible, I think). How would one go about making such a change? in a separate PR, that you merge only when ready to release a new major version?

btw thanks for the quick reviews and merges, I realize I'm creating some unexpected work for you :-)

amichair commented 5 years ago

Rebased against master to resolve conflicts after the previous PR was merged.

amichair commented 5 years ago

I added another commit cleaning up AbstractFileWordList, and along with a separate PR with the ArraySorter change, I think I've gone over the whole project code and offered whatever I thought might improve it.

The only thing that I'm still a bit uncomfortable with is the AbstractFileWordList Cache mechanism (I think it's actually an index, rather than a cache), which is a bit unclear, sometimes allocates significantly more memory than is requested, can be slightly simplified, etc. However assuming the user can't provide the number of words in the file up-front, and we don't want to make an extra pass over the file to count them, I'm not sure what would be the best way to build such an index optimally and within the memory constraints. Gotta think about that someday.

I'm also not a fan of the various createRuleResultDetailParameters methods, but that's arguably a matter of style, so I'll leave it for another day as well.

Is there anything else you'd like me to change/fix/add in this PR?

amichair commented 5 years ago

@dfish3r just wanted to thank you here for the quick reviews, good feedback and openness to merging many changes, here and in the other recent PRs - it is very much appreciated!

What the project lacks in friendly checkstyle preferences it more than makes up for in your support ;-)