5
\$\begingroup\$

These functions are more or less the same. They are doing the sign in process for a flask application. A user is retrieved from a database (identified through email address) and the password is checked for validity (bcrypt). If either of those failed, an error should be displayed for the user in the template (signin.html).

What is considered to be more readable or elegant. Are there other, nicer solutions?

@app.route('/signin', methods=['POST'])
@public
def do_signin():
    user = get_user_by_email([request.form.get('email')])
    invalid = user is None or not bcrypt.checkpw(
        request.form.get('password').encode('utf-8'),
        user['password'].encode('utf-8'),
    )

    if invalid:
        error = 'Invalid Email or Password'
    else:
        session[SESSION_TOKEN] = add_token(user['id'])
        return redirect(url_for('index'))
    return render_template('signin.html', error=error or None)

or

@app.route('/signin', methods=['POST'])
@public
def do_signin():
    user = get_user_by_email([request.form.get('email')])
    error = 'Invalid Email or Password' if user is None or not bcrypt.checkpw(
        request.form.get('password').encode('utf-8'),
        user['password'].encode('utf-8'),
    ) else None

    if error is None:
        session[SESSION_TOKEN] = add_token(user['id'])
        return redirect(url_for('index'))
    return render_template('signin.html', error=error or None)

or

@app.route('/signin', methods=['POST'])
@public
def do_signin():
    user = get_user_by_email([request.form.get('email')])

    if user is None:
        error = 'Invalid Email or Password'
    elif not bcrypt.checkpw(
        request.form.get('password').encode('utf-8'),
        user['password'].encode('utf-8')
    ):
        error = 'Invalid Email or Password'
    else:
        session[SESSION_TOKEN] = add_token(user['id'])
        return redirect(url_for('index'))
    return render_template('signin.html', error=error or None)
\$\endgroup\$
3
\$\begingroup\$

There are a few things bothering me in one version or an other:

  • the error=error or None is unnecessary as error is either assigned 'Invalid Email or Password' or None, stick to the simpler error=error;
  • the ): on its own line in the third version is more than disturbing, it feels like a syntax error and force the reader to go through the code again to understand what is going on. Instead, put it at the end of the previous line and include more indentation to distinguish the condition from the rest.;
  • each version seem to consider the success case as an error of the error case, that is the flow of reading the code does not feel as if it's a signin function (this seems to be the bad outcome) but rather a deny_access function;
  • the third version seems to accept invalid password and deny entry on successful passwords.

Instead, I would test each failing condition and bail out with a specific error message at that point rather than making the error case the "natural" exit point of the function:

@app.route('/signin', methods=['POST'])
@public
def do_signin():
    user = get_user_by_email([request.form.get('email')])
    if user is None:
        return render_template('signin.html', error='Invalid Email')

    if not bcrypt.checkpw(
            request.form.get('password').encode('utf-8'),
            user['password'].encode('utf-8')):
        return render_template('signin.html', error='Invalid Password')

    session[SESSION_TOKEN] = add_token(user['id'])
    return redirect(url_for('index'))
\$\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.