9
\$\begingroup\$

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
\$\endgroup\$
7
  • \$\begingroup\$ What are the constraints on your string? 2+ letters and 0-4 numbers? \$\endgroup\$ Mar 25, 2023 at 19:57
  • \$\begingroup\$ @DeathIncarnate Correct. Should that be written somewhere? \$\endgroup\$
    – DW98
    Mar 25, 2023 at 20:57
  • \$\begingroup\$ See J_H's answer (i.e. yes, it definitely needs to be written somewhere). Imagine you come back to this in 5 years, do you want to have to read the code to understand what's going on or would you like a helpful, human language description of what this is looking for? \$\endgroup\$ Mar 25, 2023 at 21:00
  • \$\begingroup\$ @DeathIncarnate That makes sense. Would you write it as a docstring or comment? Something like """Checks if string argument is valid; must start with 2+ letters and end with 0-4 numbers""" ? \$\endgroup\$
    – DW98
    Mar 25, 2023 at 21:44
  • \$\begingroup\$ Docstrings are powerful, check out doctest, sphinx docs and even humble help \$\endgroup\$ Mar 25, 2023 at 22:49

2 Answers 2

13
\$\begingroup\$
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.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you for the constructive answer, there's a ton of helpful info to aid my learning. I will save and refer back whenever needed. \$\endgroup\$
    – DW98
    Mar 25, 2023 at 20:54
  • 3
    \$\begingroup\$ In terms of "is_valid_foo", I would argue this depends on the context that the function is found in. If it's part of a class called "foo" or a file called "foo.py" then it probably doesn't need the "_foo" since that is provided by the wider namespace and such repetition is undesirable. I would also consider using half-open intervals for this kind of comparison bad practice, half-open is used because of 0-indexing and this a comparison to a length. Otherwise, good answer +1 \$\endgroup\$ Mar 26, 2023 at 9:34
7
\$\begingroup\$

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')
    )
\$\endgroup\$
8
  • 6
    \$\begingroup\$ "Early returns are often called short-circuiting [...]" --> It's called a "guard clause". Short-circuit is used for boolean conditions, where you reach to a result for the entire boolean expression without having to execute all the steps in the expression. \$\endgroup\$ Mar 26, 2023 at 23:11
  • 1
    \$\begingroup\$ @IsmaelMiguel Sure, guard clause is fine too. Short-circuiting has multiple meanings and usages – human language works that way. \$\endgroup\$
    – FMc
    Mar 26, 2023 at 23:49
  • 1
    \$\begingroup\$ I mean, you're right. You are short-circuiting the execution of the function, but that isn't it's usual usage. That's why I said "it used for". The "It's [...]" should be phrased differently. Comments at 1am aren't a good idea... The most common names are "early return" and "guard clause". Maybe you could reflect that into the answer? \$\endgroup\$ Mar 27, 2023 at 21:55
  • 1
    \$\begingroup\$ I'm familiar with short-circuiting as specifically a language feature where boolean operators can terminate early if the result is apparent from calculating the left hand side of the conditional statement, leaving the right hand side unevaluated. For exiting a function early based on validation, I'm only familiar with the term "guard clause". I definitely understand what you mean, but I agree with @IsmaelMiguel that "short circuit" is a nonstandard term for a "guard clause". The wikipedia page does not mention it at all: en.wikipedia.org/wiki/Guard_(computer_science). \$\endgroup\$
    – Blackhawk
    Mar 27, 2023 at 22:24
  • 1
    \$\begingroup\$ If you have the time, I would ask that you do further research on the terminology and based on your own findings either accept or reject the feedback that "guard clause" is the better term here to potentially improve the answer. \$\endgroup\$
    – Blackhawk
    Mar 27, 2023 at 22:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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