Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

The following code works and it prints out only True as it is supposed to, for all the different cases:

class User(object):
  def __init__(self, key):
    self.key = key

def is_ok(user_dbs, self_db):
  count = len(user_dbs)
  if not self_db:
    return not count
  return (count != 1 or self_db.key == user_dbs[0].key) and count != 2

print 'Should print only True:'
print is_ok([],                 None)    is True   # found nothing and no self
print is_ok([],                 User(1)) is True   # found nothing and self
print is_ok([User(1)],          None)    is False  # found one and no self
print is_ok([User(1)],          User(1)) is True   # found one and it is you
print is_ok([User(2)],          User(1)) is False  # found one and it is not you
print is_ok([User(1), User(2)], None)    is False  # found two and no self
print is_ok([User(1), User(2)], User(1)) is False  # found two and one of them is you
print is_ok([User(2), User(1)], User(1)) is False  # found two and one of them is you
print is_ok([User(2), User(3)], User(1)) is False  # found two and none of them is you

The whole thing is to check if the username is available for a new user or when an existing user is updating his current username.

The user_dbs can have maximum 2 records as it has a limit in the actual query. So here is the breakdown:

  • If there are 2 records then we know for sure that the particular username is not available.
  • If there are 0 records then we know for sure that this username is available.
  • If there is 1 record then:
    • If it's a new user then the username is not available
    • If it's an existing user and that user is yourself then the username is available, otherwise not.

So can we optimize somehow the is_ok function above?

Try it live: repl.it/XHC

share|improve this question
1  
This is the kind of thing that database uniqueness constraints are built for. Is there a reason you are not using them instead? –  cbojar Aug 19 at 20:57
1  
@cbojar Yep.. there is no such thing on Google App Engine's Datastore unless you are using that for a key and if so it's not changeable.. –  Lipis Aug 19 at 21:11
    
I figured there was a reason, but I didn't want to jump to conclusions in either direction. –  cbojar Aug 19 at 21:16

6 Answers 6

up vote 3 down vote accepted

Your is_ok method can be optimized by eliminating the counting:

def is_ok(user_dbs, self_db):
    return not user_dbs or self_db.key == user_dbs[0].key and not user_dbs[1:]

Or, if you implement User.__eq__ as @mleyfman suggested (great idea btw), then you can write like this:

def is_ok(user_dbs, self_db):
    return not user_dbs or self_db in user_dbs and not user_dbs[1:]

I also recommend turning the whole thing into proper unit tests, rather than reading the output to check it:

import unittest
from test.test_support import run_unittest


class User(object):
    def __init__(self, key):
        self.key = key

    def __eq__(self, other):
        return self.key == other.key if isinstance(other, User) else False


def is_ok(user_dbs, self_db):
    if not self_db:
        return not user_dbs
    return not user_dbs or self_db in user_dbs and not user_dbs[1:]


class TestAddingUser(unittest.TestCase):
    def test_find_nothing_and_no_self(self):
        self.assertTrue(is_ok([], None))

    def test_find_nothing_and_self(self):
        self.assertTrue(is_ok([], User(1)))

    def test_found_one_and_no_self(self):
        self.assertFalse(is_ok([User(1)], None))

    def test_found_one_and_it_is_you(self):
        self.assertTrue(is_ok([User(1)], User(1)))

    def test_found_one_and_it_is_not_you(self):
        self.assertFalse(is_ok([User(2)], User(1)))

    def test_found_two_and_no_self(self):
        self.assertFalse(is_ok([User(1), User(2)], None))

    def test_found_two_and_one_is_you(self):
        self.assertFalse(is_ok([User(1), User(2)], User(1)))
        self.assertFalse(is_ok([User(2), User(1)], User(1)))

    def test_found_two_and_none_is_you(self):
        self.assertFalse(is_ok([User(2), User(3)], User(1)))


def test_main():
    run_unittest(TestAddingUser)


if __name__ == '__main__':
    test_main()
share|improve this answer
    
It is indeed a good thing :) –  Lipis Aug 19 at 22:00
    
hehe.. Thank you for that, but I didn't want to add bunch of things for this little test and wanted to keep it as small as possible.. yet accurate for testing.. –  Lipis Aug 19 at 22:07
    
Plus mine works on repl.it :D –  Lipis Aug 19 at 22:10
    
This is pretty spot on. I didn't want to touch the tests as I figured, minor script - but janos brought it up to production levels :) –  jsanc623 Aug 20 at 1:26
    
Thanks for the edit and indeed this is great for production level. You can also check my edit, which is just verifies that @mleyfman is great! –  Lipis Aug 20 at 5:33

I came up with a different solution by utilizing Python's magic method for == which is __eq__(self, other).

I define it in the User class like so:

class User(object):
    def __init__(self, key):
        self.key = key
    def __eq__(self, other):
        return self.key == other.key if isinstance(other, User) else False

This allows you to do the obvious User(1) == User(2) testing but also this allows you to test membership in a collection using the in keyword:

User(1) in [User(1)] # This will return True
User(1) in [User(2)] # This will return False

We can then change the condition slightly to make the is_ok function like this:

def is_ok(user_dbs, self_db):
    count = len(user_dbs)
    if not self_db:
        return not count
    return count == 1 and self_db in user_dbs or count == 0

This has the advantage of being a bit more readable, and hence Pythonic. This might come in handy elsewhere as well.

Another minor point is that if you ever rename key to something else or change the structure of your User class, this code will still work (remember to update __eq__!), while most of the alternatives will break.

share|improve this answer
    
that is also interesting approach! :) You have a typo: ..in user_dbs.. –  Lipis Aug 19 at 21:53
    
Oh, sorry about that, it is now fixed. –  mleyfman Aug 19 at 21:54
    
You could also include the repl.it link for easier tests (repl.it/XHC/3) and I also improved the your __eq__ –  Lipis Aug 19 at 21:58
    
That is neat! I updated the __eq__ method to use a ternary as you suggested. –  mleyfman Aug 19 at 22:00
    
+1 This is great, but the end of your ternary should be else False instead of else return False (which doesn't compile) –  janos Aug 20 at 5:28

The only real optimization I see if taking advantage of Pythons english comprehension to unify the return statement:

So, is_ok would read:

def is_ok(user_dbs, self_db):
  count = len(user_dbs)
  return not count if not self_db \
      else ((count != 1 or self_db.key == user_dbs[0].key) and count != 2)

Here's the repl.it showing it works with the unified return statement: http://repl.it/XHB

share|improve this answer
    
Yes that is one thing, but I was hoping for something better.. :) –  Lipis Aug 19 at 21:27
1  
@Lipis I certainly understand that :) janos' answer seems pretty spot on –  jsanc623 Aug 20 at 1:24

If you have performance problems I don't think you can gain anything from the snippet you have shown.

Anyway I would write the test:

  (count != 1 or self_db.key == user_dbs[0].key) and count != 2

as

  count != 2 and (count != 1 or self_db.key == user_dbs[0].key)

so that in the case count==2 there is no need to access self_db and user_dbs. This is particularly convinient if the case count==2 is more frequent than count==0.

share|improve this answer
    
No there are no performance issues and maybe the optimize wans't the best word.. I'm looking maybe for a smaller signature or a more readable thingy.. but I'm guessing there is not much we can do.. getting the count in front is a good point though :) –  Lipis Aug 19 at 21:50

This function can be simplified to:

def is_ok(user_dbs, self_db):
    return not user_dbs or (len(user_dbs) == 1 and
                            self_db is not None and
                            user_dbs[0].key == self_db.key)

EDIT: Forgot to check if self_db is None. Fixed, kudos to Lipis.

share|improve this answer
1  
But this doesn't pass all the required tests –  janos Aug 20 at 5:18
    
@janos is right. After running some tests in App Engine I have a small edit in the original question (sorry for that but I didn't think about it in a first place). So the last equality can be changed to: self_db in user_dbs and there will be no more errors. Plus since the AND operations are being evaluated first the parenthesis is not needed so the whole thing can look like this: return not user_dbs or len(user_dbs) == 1 and self_db in user_dbs repl.it/XHC/7 –  Lipis Aug 20 at 5:30
3  
and I also think that the comment about sanity is not really needed.. –  Lipis Aug 20 at 5:36
    
@Lipis If I'm the one who's not thinking straight, please explain which test my answer fails. Otherwise, I think my comment about sanity stands ._. –  Will Aug 20 at 8:05
    
@Will Here you are.. repl.it/XHC/9 But the comment about sanity doesn't stand either your code succeeds or fails.. –  Lipis Aug 20 at 8:09

You write the nice breakdown, clear and transparent. What makes you think your program should not be like that?

def is_available(this_user, db_users):

    # If there are 0 records then we know for sure that this username is available.
    if not db_users:
        return True

    # If there are 2 records then we know for sure that the particular username is not available.
    if len(db_users) > 1:
        return False

    # If it's a new user then the username is not available
    if not this_user:
        return False

    # If it's an existing user and that user is yourself then the username is available, otherwise not.
    return this_user.key == db_users[0].key

Now compare this to some concise, smart and magic solution and imagine yourself reading this code six months later. What would be more understandable?

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.