Check validity of string, used return False too often? (CS50 plates.py)

Question

Firstly, yes I'm a beginner. The code does function correctly but it looks ugly and I'm unsure if it is easily readable.

I think my issue is the fact I've used 'return False' 5 times in a single function. Could this be written better? If so, is it very bad or just a slight tweak(s)? `

def is_valid(s):
    length = len(s)
    # Length between 2 and 6
    if length < 2 or length > 6:
        return False
    # False for punctuation
    if not s.isalnum():
        return False
    # First 2 char must be letters
    if not s[0:2].isalpha():
        return False
    # Numbers cannot start with 0
    # Numbers cannot be in the middle of letters
    for i in range(length):
        if s[i].isdigit():
            if s[i] == '0':
                return False
            elif not s[i:].isdigit():
                return False
            else:
                break
    return True

Answer 1

def is_valid(s):

You're not giving us many hints, there. What is s? What kind of validity criteria? As an engineer, why would I want to call this function?

This function is crying out for a helpful """docstring""".

Consider using a longer name: is_valid_foo (I do not yet know what "foo" might be.)

Consider adding an optional type annotation: s: str. May as well end with ...(s: str) -> bool: while you're at it, though the helpful is_ prefix already tells us the return type.

Just like i and j are often good choices for local variables that are indexes, s can be a good choice for a string. But here s is part of your public API, so the documentation burden is much higher. The function name didn't spell it out, so the parameter name should definitely offer at least one word's worth of hint.

---

    # Length between 2 and 6
    if length < 2 or length > 6:

Delete the # comment, as it doesn't tell us anything beyond what the code has already said. We use comments to describe the "why", and code to describe the "how".

Consider chaining the comparisons:

    if not(2 <= length <= 6):

---

I've used 'return False' 5 times

That's not so bad. You have slightly complex rules, and the code is clear.

Here is one idea to help with clarity. Express the rules in the positive rather than the negative, breaking out a helper function for each one, and string them together as conjuncts:

def starts_with_alpha(s: str) -> bool:
    return s[:2].isalpha()

def is_valid(s: str) -> bool:
    return (is_valid_length(s)
            and s.isalnum()
            and starts_with_alpha(s)
            and is_alpha_then_number(s))

The idea is to make it read like an English sentence, so there's little or no need for comments.

---

You didn't offer any requirements in English, but they are apparent from the code.

Another approach to satisfying the requirements would be to express them as a regex:

import re

valid_re = re.compile(r'^[a-z]{2,}(?P<num>([1-9]\d*)?)$')

    ...
    return (is_valid_length(s)
            and valid_re.search(s.lower()))

---

The code as written is clear and achieves its design goals.

I would be happy to delegate or accept maintenance tasks for it.

Before merging it down to main the signature / docstring should be improved, so maintenance engineers can immediately see what the function is good for without reading through all of its code.

Answer 2

A function with several returns is not necessarily a problem. Early returns are often called short-circuiting and they can be effective in keeping code readable: the intuition is to handle easy checks first before proceeding to more difficult matters.

When feasible, give functions more explicit names. The current name tells us you are validating something, but that's about it. Be more specific with names when possible, especially at interface points (a function name is inherently one: it's the interface between the caller and the work of the function).

In Python, iterating via indexes is rarely useful. Python sequences (including strings) are directly iterable. If you need the discrete values from a sequence (characters in this case), iterate directly over the string. If you also need indexes, use enumerate().

The hard part: first letters then digits. Most of the complexity in your current approach deals with determining whether the license plate consists of letters then numbers. Your current code is not too difficult to follow. You can simplify the code and help its readability in two ways: (1) slightly rephrase the comment to clarify that digits are optional and (2) return immediately to emphasize that we reach our decision after seeing the first digit.

def is_valid_vanity_plate(s):
    # Basic checks:
    # - length
    # - ASCII letters and digits
    # - start with 2 letters
    ok = (
        2 <= len(s) <= 6 and
        s.isalnum() and
        s[0:2].isalpha()
    )
    if not ok:
        return False

    # Digits, if any, must come at the end and cannot start with zero.
    for i, c in enumerate(s):
        if c.isdigit():
            return c != '0' and s[i:].isdigit()
    return True

The hard part: an alternative. You could use a regular expression to perform most of the validation: two or more letters, optionally followed by a non-zero digit and then some other digits. I suspect that many Python programmers would handle it this way.

import re

PLATE_RGX = re.compile(r'^[a-z]{2,}([1-9]\d*)?$', re.IGNORECASE)

def is_valid_vanity_plate(s):
    return bool(
        2 <= len(s) <= 6 and
        PLATE_RGX.search(s)
    )

The hard part: another alternative. A different technique is to classify the string into sub-sequences of non-digits and digits. That can be done directly with itertools.groupby, which takes any iterable and a function to perform the classifying. Once you have those parts, the validation logic is quite simple.

def is_valid_vanity_plate(s):
    # Basic checks (same as above).
    ...

    # Break string apart into sub-sequences of non-digits and digits.
    parts = [
        ''.join(g)
        for _, g in groupby(s, str.isdigit)
    ]

    # Digits, if any, must be in the second part and cannot start with zero.
    n = len(parts)
    return (
        n == 1 or
        (n == 2 and parts[1][0] != '0')
    )