Code review comment for lp:~elopio/osqa-sst-helpers/data

Some naming remarks:

50 + def make_for_test(klass, new_user=False):

Well, we know it's for test since the whole class is for tests.

53 + test_user = klass._make_random_test_user_from_configuration()

55 + test_user = klass._make_test_user_from_configuration()

Since all use cases are from configuration, I'd rather name this method 'make_from_configuration'.

58 + @classmethod
59 + def _make_random_test_user_from_configuration(klass):
60 + random_user_uuid = str(uuid.uuid1())
61 + full_name = 'Test user ' + random_user_uuid
62 + email_address = settings.EMAIL_ADDRESS_PATTERN % random_user_uuid
63 + password = settings.SSO_TEST_ACCOUNT_PASSWORD
64 + return klass(full_name, email_address, password)
65 +
66 + @classmethod
67 + def _make_test_user_from_configuration(klass):
68 + full_name = settings.SSO_TEST_ACCOUNT_FULL_NAME
69 + email_address = settings.SSO_TEST_ACCOUNT_EMAIL
70 + password = settings.SSO_TEST_ACCOUNT_PASSWORD
71 + return klass(full_name, email_address, password)

I think having these methods hurt readability and make for more duplication:

49 + @classmethod
58 + @classmethod
66 + @classmethod

63 + password = settings.SSO_TEST_ACCOUNT_PASSWORD
64 + return klass(full_name, email_address, password)

70 + password = settings.SSO_TEST_ACCOUNT_PASSWORD
71 + return klass(full_name, email_address, password)

So I'd rather go with these methods inlined in
'make_from_configuration'. This way the reader can focus on the still short
enough 'make_from_configuration'.

An additional trap in your implementation is that the two methods are not in
the order they are used (so, as a reader, if I don't pay attention enough I
won't notice and read the wrong method when reading 'make_for_test').

'_make_random_test_user_from_configuration' and
'_make_test_user_from_configuration' being this long (note that I can't cite
them both on a 80 chars long line), it's hard to spot the differing
'random', that's, for me, an example of why names too long hurt readability
instead of helping it. Rmeebmer that popele are good at recgniozing words
based on the first and last letetr even if the letters inside the words are
suhffled (you managed to read this sentence without too much trouble aren't
you ? ;).

I.e.:

....

wth, I cannot branch lp:osqa-sst-helpers ? What is this project about ? 8-/
(that was rhetorical, the development focus needs to be set in lp).

anyway:

=== modified file 'sso_sst_helpers/data.py'
--- sso_sst_helpers/data.py 2012-10-10 21:07:11 +0000
+++ sso_sst_helpers/data.py 2012-10-11 07:47:48 +0000
@@ -30,25 +30,13 @@
         return '%s(%r)' % (self.__class__, self.__dict__)

     @classmethod
- def make_for_test(klass, new_user=False):
+ def make_from_configuration(klass, new_user=False):
         test_user = None
         if new_user:
- test_user = klass._make_random_test_user_from_configuration()
+ full_name = settings.SSO_TEST_ACCOUNT_FULL_NAME
+ email_address = settings.SSO_TEST_ACCOUNT_EMAIL
         else:
- test_user = klass._make_test_user_from_configuration()
- return test_user
-
- @classmethod
- def _make_random_test_user_from_configuration(klass):
- random_user_uuid = str(uuid.uuid1())
- full_name = 'Test user ' + random_user_uuid
- email_address = settings.EMAIL_ADDRESS_PATTERN % random_user_uuid
- password = settings.SSO_TEST_ACCOUNT_PASSWORD
- return klass(full_name, email_address, password)
-
- @classmethod
- def _make_test_user_from_configuration(klass):
- full_name = settings.SSO_TEST_ACCOUNT_FULL_NAME
- email_address = settings.SSO_TEST_ACCOUNT_EMAIL
- password = settings.SSO_TEST_ACCOUNT_PASSWORD
+ random_user_uuid = str(uuid.uuid1())
+ full_name = 'Test user ' + random_user_uuid
+ email_address = settings.EMAIL_ADDRESS_PATTERN % random_user_uuid
         return klass(full_name, email_address, password)

bzr diffstat for the above says:

 sso_sst_helpers/data.py | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

The resulting method is 11 lines long instead of 21 for the 3 methods, less
to read, more readable.