9
\$\begingroup\$

I coded a python solution for the question below. I am after the fastest solution. Also, I would be grateful if you could point out parts that are not pythonic / inefficient.

Question: A website requires the users to input username and password to register. Write a program to check the validity of password input by users. Following are the criteria for checking the password:

  1. At least 1 letter between [a-z]
  2. At least 1 number between [0-9]
  3. At least 1 letter between [A-Z]
  4. At least 1 character from [$#@]
  5. Minimum length of transaction password: 6
import warnings
password = 'P@ss1234'
def check_number(s):
    ''' Check whether the input string is a digit. '''
    try: 
        int(s)
        return True
    except:
        # do not catch error
        return False
def check_validity(pw):
    ''' Return True if input pw is valid, and return False if invalid.'''
    special_chars = ['$','#','@']
    if isinstance(pw,str): pw=list(pw) # I could have appointed to a diff var name
    else: warnings.warn('Password has to be a string object.')
    res = False
    valid_dict={'small_let':False, 'num':False, 'special_chars':False, 
                'cap_let':False, 'len':False } # is using a dict efficient?
    if len(pw)>= 6: valid_dict['len']=True
    for i in pw:
        if i.islower(): valid_dict['small_let'] = True
        if i in special_chars: valid_dict['special_chars'] = True
        if i.isupper(): valid_dict['cap_let'] = True
        if not valid_dict['num']:  valid_dict['num'] = check_number(i)
    if all(valid_dict.values()): res = True
    return res

print check_validity(password)

P.S: I am not interested in a module / package that can do this faster or more securely. There does not actually exist a website or a username, this is a simple coding exercise.

Also, linked to this post.

\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to codereview. This is a nice first question. I'm sure you'll get some nice feedback on this. \$\endgroup\$ – Grajdeanu Alex. Sep 30 '16 at 8:15
  • \$\begingroup\$ instead of creating the functioncheck_number(s) you can use built-in s.isdigit() \$\endgroup\$ – valentin Sep 30 '16 at 11:12
  • 5
    \$\begingroup\$ The developer should push back against these criteria because they are counter to the aim of improving security. See nakedsecurity.sophos.com/2016/08/18/… and youtube.com/watch?v=ReTi9dYuPWc \$\endgroup\$ – bdsl Sep 30 '16 at 12:32
  • \$\begingroup\$ @bdsl I hope because this is a coding exercise the OP doesn't plan to use this for real, but it is definitely a good signpost for other readers. \$\endgroup\$ – Brian J Sep 30 '16 at 14:20
  • 1
    \$\begingroup\$ I'm sure the OP doesn't plan to use this, but people learn from exercises, and I think this exercises teaches people to make bad websites. \$\endgroup\$ – bdsl Sep 30 '16 at 14:59
12
\$\begingroup\$
  1. You shouldn't use bare excepts, this is as it can prevent things like KeyboardInterrupt. That's just bad.
  2. password is a better variable name than pw, at first I was quite confused about what it was.
  3. You can return early if you know the password is not valid. This can save you a lot of checks in your program.
  4. Your program goes through each character and checks if it's one of the four valid groups. Instead you can check if it shares any items with that set. If it doesn't it's not valid.
  5. You're creating warnings un-neededly, currently, excluding the cast, the functionality is the same if it's a list. Instead of a warning you should either raise an error, or return.

    This is as if I pass ['P', 'a', '$', '2', 1, 2] it probably shouldn't return True, which it might on a different implementation. Instead you can either hard limit your input to strings, or check if the contents of the input are strings. Or both.

In short, just change password to a set, and use & with the groups it needs to be a part of. This can remove the need to make the function check_number, and makes the code much more readable:

import string

def check_validity(password):
    ''' Return True if input pw is valid, and return False if invalid.'''
    if isinstance(password, str) \
            or not all(isinstance(c, str) for c in password):
        return False

    if len(password) < 6:
        return False

    password = set(password)
    checks = [
        set(string.ascii_lowercase),
        set(string.ascii_uppercase),
        set(string.digits),
        {'$', '#', '@'},
    ]

    for check in checks:
        if not check & password:
            return False
    return True

print check_validity('P@ss1234')
\$\endgroup\$
  • \$\begingroup\$ This is great, thank you. By "limiting the use unnecessarily" , are you referring to the string type check and the warning in the beginning of check_validity? \$\endgroup\$ – Zhubarb Sep 30 '16 at 8:57
  • \$\begingroup\$ @Zhubarb Yeah I am, I should probably rephrase that as, you're not limiting the usage. But you're advising against a certain usage. \$\endgroup\$ – Peilonrayz Sep 30 '16 at 8:58
  • \$\begingroup\$ OK, fair enough. And wrt to checks list you create (with sets of items), I think it is quite elegant and definitely a lot easier to read. But when I compare it to my solution (with all the inefficiencies you point out addressed), the checks list solution runs slightly slower. \$\endgroup\$ – Zhubarb Sep 30 '16 at 9:00
5
\$\begingroup\$

At the moment, you're checking every character for every condition. If it's a lower case character, it's not going to be any of the rest of them, so you can simply abort that iteration of the loop and move onto the next character. Something like:

if i.islower(): 
    valid_dict['small_let'] = True
    continue

You can then arrange your checks in the order that is most likely to happen. I'd imagine:

  • Lower case letters
  • Upper case letters
  • Numbers
  • Special characters

Would be a better order...

As an aside, whilst the approach taken by @Joe Wallis is probably going to be more performant, and you've said you're not interested in using additional packages, this is the type of validation that regular expressions are good at representing:

import re
password = 'P@ss1234'
required_pattern = re.compile('(?=.{6,})(?=.*[a-z])(?=.*[A-Z])(?=.*[0-9])(?=.*[@#$])')

def check_validity(pw):
    return required_pattern.match(pw) != None

print check_validity(password)
\$\endgroup\$
  • \$\begingroup\$ This solution looks nice, but not very flexible. If one day I want to know why my password got rejected, is it possible / simple to implement ? \$\endgroup\$ – Etsitpab Nioliv Sep 30 '16 at 12:11
  • \$\begingroup\$ "this is the type of validation that regular expressions are good at representing" Funny you should say this. The original concept of regex would have a serious time doing this sort of validation. Even with extended regexes, your solution is not particularly readable. \$\endgroup\$ – unperson325680 Sep 30 '16 at 12:34
1
\$\begingroup\$
except:
    # do not catch error

This is a good example for sometimes it is better to write no comment at all.

You do catch the error. In fact, you catch any errors, even the ones that you didn't anticipate.

\$\endgroup\$
0
\$\begingroup\$

That us an old post which is well answered, however I would like to add a short note:

You can simplify and, in the same time, avoid bugs related to the improper use of exception handling within the check_number() function -highlighted by previous answers- by making use of isinstance():

>>> password = 2
>>> isinstance(password, int)
True
>>> password = '2'
>>> isinstance(password, int)
False

So that function would become:

def check_number(s):
   return isinstance(s, int)
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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