Added a password strength calculator class and unit tests. Added label displaying master password...

Added a password strength calculator class and unit tests. Added label displaying master password strength the first time it is entered.

[stevet, 2011-06-28 22:58:59]

Awesome. This works like you said!

Over to you for review.

[Roger Tawa (Google), 2011-06-29 02:56:14]

http://codereview.appspot.com/4662062/diff/1/res/values/strings.xml
File res/values/strings.xml (right):

http://codereview.appspot.com/4662062/diff/1/res/values/strings.xml#newcode142
res/values/strings.xml:142: <string name="password_strength_caption">Password
Strength:</string>
Make this a format string, like:

  Password strength: {0}

then use MessageFormat to build the string.  This makes the message i18n ready.

http://codereview.appspot.com/4662062/diff/1/src/net/tawacentral/roger/secret...
File src/net/tawacentral/roger/secrets/LoginActivity.java (right):

http://codereview.appspot.com/4662062/diff/1/src/net/tawacentral/roger/secret...
src/net/tawacentral/roger/secrets/LoginActivity.java:153:
strength.setVisibility(TextView.INVISIBLE);
should this INVISIBLE or GONE?

http://codereview.appspot.com/4662062/diff/1/src/net/tawacentral/roger/secret...
src/net/tawacentral/roger/secrets/LoginActivity.java:300:
strength.setVisibility(TextView.INVISIBLE);
GONE ?

http://codereview.appspot.com/4662062/diff/1/src/net/tawacentral/roger/secret...
src/net/tawacentral/roger/secrets/LoginActivity.java:390: String text =
getString(R.string.password_strength_caption) + " ";
use MessageFormat here.

http://codereview.appspot.com/4662062/diff/1/src/net/tawacentral/roger/secret...
File src/net/tawacentral/roger/secrets/PasswordStrength.java (right):

http://codereview.appspot.com/4662062/diff/1/src/net/tawacentral/roger/secret...
src/net/tawacentral/roger/secrets/PasswordStrength.java:54: }
do we need this level of granularity at the low end?  is it important to know
the password is very-weak vs weak?  maybe it would be better to reduce
granularity at the low end and even increase at the high end.  give people an
incentive to pick strong passwords :-)  so maybe something like:

weak, medium, strong, very strong, crazy/stupid/geek strong

http://codereview.appspot.com/4662062/diff/1/src/net/tawacentral/roger/secret...
src/net/tawacentral/roger/secrets/PasswordStrength.java:56: /** Calculate the
new strength of the password based on a new change in the
newline before Calculate...

http://codereview.appspot.com/4662062/diff/1/src/net/tawacentral/roger/secret...
src/net/tawacentral/roger/secrets/PasswordStrength.java:72: /** Helper used to
append to the current password strength score based on
newline before Helper...

http://codereview.appspot.com/4662062/diff/1/src/net/tawacentral/roger/secret...
src/net/tawacentral/roger/secrets/PasswordStrength.java:90: for (int ix = 0; ix
< sequence.length(); ix++) {
ix ?

http://codereview.appspot.com/4662062/diff/1/src/net/tawacentral/roger/secret...
File src/net/tawacentral/roger/secrets/PasswordStrengthTest.java (right):

http://codereview.appspot.com/4662062/diff/1/src/net/tawacentral/roger/secret...
src/net/tawacentral/roger/secrets/PasswordStrengthTest.java:66:
test(PasswordStrength.Strength.VERY_STRONG, "Space separated words are good!1");
line too long.

[stevet, 2011-07-24 22:49:45]

Addressed all issues.

Also reverted back to a simplified version of the PasswordStrength class (no
incremental strength calculation) and updated tests.

http://codereview.appspot.com/4662062/diff/1/res/values/strings.xml
File res/values/strings.xml (right):

http://codereview.appspot.com/4662062/diff/1/res/values/strings.xml#newcode142
res/values/strings.xml:142: <string name="password_strength_caption">Password
Strength:</string>
On 2011/06/29 02:56:14, rogerta wrote:
> Make this a format string, like:
> 
>   Password strength: {0}
> 
> then use MessageFormat to build the string.  This makes the message i18n
ready.

Done.

http://codereview.appspot.com/4662062/diff/1/src/net/tawacentral/roger/secret...
File src/net/tawacentral/roger/secrets/LoginActivity.java (right):

http://codereview.appspot.com/4662062/diff/1/src/net/tawacentral/roger/secret...
src/net/tawacentral/roger/secrets/LoginActivity.java:153:
strength.setVisibility(TextView.INVISIBLE);
On 2011/06/29 02:56:14, rogerta wrote:
> should this INVISIBLE or GONE?

GONE. Done.

http://codereview.appspot.com/4662062/diff/1/src/net/tawacentral/roger/secret...
src/net/tawacentral/roger/secrets/LoginActivity.java:300:
strength.setVisibility(TextView.INVISIBLE);
On 2011/06/29 02:56:14, rogerta wrote:
> GONE ?

Oops. Didn't realize there was a difference. GONE is what I want. Done.

http://codereview.appspot.com/4662062/diff/1/src/net/tawacentral/roger/secret...
src/net/tawacentral/roger/secrets/LoginActivity.java:390: String text =
getString(R.string.password_strength_caption) + " ";
On 2011/06/29 02:56:14, rogerta wrote:
> use MessageFormat here.

Done.

http://codereview.appspot.com/4662062/diff/1/src/net/tawacentral/roger/secret...
File src/net/tawacentral/roger/secrets/PasswordStrength.java (right):

http://codereview.appspot.com/4662062/diff/1/src/net/tawacentral/roger/secret...
src/net/tawacentral/roger/secrets/PasswordStrength.java:54: }
On 2011/06/29 02:56:14, rogerta wrote:
> do we need this level of granularity at the low end?  is it important to know
> the password is very-weak vs weak?  maybe it would be better to reduce
> granularity at the low end and even increase at the high end.  give people an
> incentive to pick strong passwords :-)  so maybe something like:
> 
> weak, medium, strong, very strong, crazy/stupid/geek strong

That makes sense. I'll change the wording to reflect that.

http://codereview.appspot.com/4662062/diff/1/src/net/tawacentral/roger/secret...
src/net/tawacentral/roger/secrets/PasswordStrength.java:56: /** Calculate the
new strength of the password based on a new change in the
On 2011/06/29 02:56:14, rogerta wrote:
> newline before Calculate...

Done.

http://codereview.appspot.com/4662062/diff/1/src/net/tawacentral/roger/secret...
src/net/tawacentral/roger/secrets/PasswordStrength.java:72: /** Helper used to
append to the current password strength score based on
On 2011/06/29 02:56:14, rogerta wrote:
> newline before Helper...

Done.

http://codereview.appspot.com/4662062/diff/1/src/net/tawacentral/roger/secret...
src/net/tawacentral/roger/secrets/PasswordStrength.java:90: for (int ix = 0; ix
< sequence.length(); ix++) {
On 2011/06/29 02:56:14, rogerta wrote:
> ix ?

Old habit of mine to use ix for "IndeX" instead of i. Makes it easier to search
in a text file than "i". I'll go with whatever's consistent with your code.

http://codereview.appspot.com/4662062/diff/1/src/net/tawacentral/roger/secret...
File src/net/tawacentral/roger/secrets/PasswordStrengthTest.java (right):

http://codereview.appspot.com/4662062/diff/1/src/net/tawacentral/roger/secret...
src/net/tawacentral/roger/secrets/PasswordStrengthTest.java:66:
test(PasswordStrength.Strength.VERY_STRONG, "Space separated words are good!1");
On 2011/06/29 02:56:14, rogerta wrote:
> line too long.

Done.

[Roger Tawa (Google), 2011-07-26 03:52:49]

lgtm

Thanks for contributing Steve.  I'll patch this in soon.  I'll implement the
suggestion i mention below and see that the code looks like.

http://codereview.appspot.com/4662062/diff/5001/src/net/tawacentral/roger/sec...
File src/net/tawacentral/roger/secrets/LoginActivity.java (right):

http://codereview.appspot.com/4662062/diff/5001/src/net/tawacentral/roger/sec...
src/net/tawacentral/roger/secrets/LoginActivity.java:388: String text =
getString(R.string.password_strength_caption) + " ";
do you need the trailing space?

http://codereview.appspot.com/4662062/diff/5001/src/net/tawacentral/roger/sec...
src/net/tawacentral/roger/secrets/LoginActivity.java:421: }
you don't like the idea of putting the text and colour as properties of the str
instance?

[stevet, 2011-07-26 12:50:22]

Oops - I feel like I missed a comment or something. Let's discuss offline
what this entails and I can probably edit the patch for you.

On Mon, Jul 25, 2011 at 11:52 PM, <rogerta@google.com> wrote:

> lgtm
>
> Thanks for contributing Steve.  I'll patch this in soon.  I'll implement
> the suggestion i mention below and see that the code looks like.
>
>
> http://codereview.appspot.com/**4662062/diff/5001/src/net/**
>
tawacentral/roger/secrets/**LoginActivity.java<http://codereview.appspot.com/4662062/diff/5001/src/net/tawacentral/roger/secrets/LoginActivity.java>
>
> File src/net/tawacentral/roger/**secrets/LoginActivity.java (right):
>
> http://codereview.appspot.com/**4662062/diff/5001/src/net/**
>
tawacentral/roger/secrets/**LoginActivity.java#newcode388<http://codereview.appspot.com/4662062/diff/5001/src/net/tawacentral/roger/secrets/LoginActivity.java#newcode388>
> src/net/tawacentral/roger/**secrets/LoginActivity.java:**388: String text
> =
>
> getString(R.string.password_**strength_caption) + " ";
> do you need the trailing space?
>
> http://codereview.appspot.com/**4662062/diff/5001/src/net/**
>
tawacentral/roger/secrets/**LoginActivity.java#newcode421<http://codereview.appspot.com/4662062/diff/5001/src/net/tawacentral/roger/secrets/LoginActivity.java#newcode421>
> src/net/tawacentral/roger/**secrets/LoginActivity.java:**421: }
> you don't like the idea of putting the text and colour as properties of
> the str instance?
>
>
>
http://codereview.appspot.com/**4662062/<http://codereview.appspot.com/4662062/>
>

[stevet, 2011-07-27 00:47:02]

Okay so somehow I managed to miss your original comment about using the Enum to
do all the work.

This is certainly a lot cleaner, although I was trying to keep UI out of the
PasswordStrength class and let the Activity managing the view handle UI stuff.

The only issue is that now my unit tests don't run in Eclipse as I've introduced
an Android dependency to the PasswordStrength class (android.graphics.Color),
and it turns out that running vanilla JUnit tests in Eclipse is a LOT simpler
than running Android JUnit tests.

So if you don't mind, you can commit these changes and I'll fix the unit tests
in a separate CL later on (spent a bit of time looking into it with no luck, and
don't have time to look into it until next week).


Steve

http://codereview.appspot.com/4662062/diff/5001/src/net/tawacentral/roger/sec...
File src/net/tawacentral/roger/secrets/LoginActivity.java (right):

http://codereview.appspot.com/4662062/diff/5001/src/net/tawacentral/roger/sec...
src/net/tawacentral/roger/secrets/LoginActivity.java:388: String text =
getString(R.string.password_strength_caption) + " ";
On 2011/07/26 03:52:49, rogerta wrote:
> do you need the trailing space?

Done.

http://codereview.appspot.com/4662062/diff/5001/src/net/tawacentral/roger/sec...
src/net/tawacentral/roger/secrets/LoginActivity.java:421: }
On 2011/07/26 03:52:49, rogerta wrote:
> you don't like the idea of putting the text and colour as properties of the
str
> instance?

Done.

[Roger Tawa (Google), 2011-07-27 01:32:56]

That's awesome Steve, thanks.  I'll commit like this and you can fix the tests
next week.