search

DanElbert / vt-middleware

Automatically exported from code.google.com/p/vt-middleware
0 stars 0 forks source link

vt-password: review SequenceRule refactoring #103

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago 
Things to consider:
  * Are class names indicative of their function?
  * Too many constructors? useful for testing, but may be confusing for developers
  * Is AbstractSequenceRule useful or confusing? There is actually very little overlap between *all* the impls
  * Can sequence generation be improved from a performance standpoint?
  * Can password validation be improved from a performance standpoint?

Original issue reported on code.google.com by dfis...@gmail.com on 18 Feb 2011 at 3:50

GoogleCodeExporter commented 8 years ago 
Link to the revision to review:
http://code.google.com/p/vt-middleware/source/detail?r=1832

Original comment by dfis...@gmail.com on 18 Feb 2011 at 4:16

GoogleCodeExporter commented 8 years ago 
Attached patch with alternative implementation that makes heavy use of 
iterators to do the minimal amount of character comparison and make the most of 
code reuse in abstract base class.

Original comment by marvin.addison@gmail.com on 22 Feb 2011 at 4:38

Attachments:

GoogleCodeExporter commented 8 years ago 
I wrote a unit test to confirm performance.
Validating 1000 passwords with all sequence rules in the rule list:
Current impl: ~40ms
Patch impl: ~110ms

Not particularly surprising since the current impl definitely trades memory for 
performance.
Those numbers aside, I think the patch has something to offer.
I'll continue my review...

Original comment by dfis...@gmail.com on 22 Feb 2011 at 8:45

GoogleCodeExporter commented 8 years ago 
Attached patch that is along same lines of previous but with some 
considerations for performance and clarity.  Using the PerformanceTest in the 
test source, this implementation is a mere 10% slower than the original with 
precomputed static strings (with all flags set to true).  This seems like a 
clear winner in the speed/space tradeoff.

Original comment by marvin.addison@gmail.com on 23 Feb 2011 at 7:34

Attachments:

GoogleCodeExporter commented 8 years ago

Original comment by dfis...@gmail.com on 25 Feb 2011 at 8:35