a lot of and's in if statement (python)-CodePudding

Is it usually considered as badly written code when you have a lot of conditions in one if statement? I'm doing some advent of code stuff (right now 2020 day 4), and this seemed like the easiest and fastest solution to my problem, but it looks a little weird.

def check_for_valid_passports(inp):
    num_of_valid_passports = 0
    for x in inp:
        if "byr" in x and "iyr" in x and "eyr" in x and "hgt" in x and "hcl" in x and "ecl" in x and "pid" in x:
            num_of_valid_passports  = 1

    return num_of_valid_passports

CodePudding user response：

I wouldn't consider this the best answer (it's also not very pythonic sorry), but the way I personally would implement this would be like below.

def check_for_valid_passports(inp):
    num_of_valid_passports = 0
    for x in inp:
        checks = ['bry', 'iyr', 'eyr', 'hgt', 'hcl', 'ecl', 'pid'] # List of checks that need to pass
        add = True # Default assumes all checks pass
        for check in checks:
            if check not in x:
                add = False # If any of the checks fail then set a flag to false
        if add:
            num_of_valid_passports  = 1

    return num_of_valid_passports

I also haven't tested if this works because I don't know what the input is supposed to look like.

CodePudding user response：

You can use sum and all as follows:

>>> def check_for_valid_passports(inp):
...     return sum(all(s in i for s in (
...         "byr", "iyr", "eyr", "hgt", "hcl", "ecl", "pid"
...     )) for i in inp)
...
>>> check_for_valid_passports(["byriyreyrhgthcleclpid", "foo", "pid", "byriyreyrhgthcleclpid"])
2

If the elements of inp are sets (or can easily be converted to sets) you can potentially simplify this down further by using set operations instead of all and in.

CodePudding user response：

You can refactor it using sum and all:

def check_for_valid_passports(inp):
    substrings = ("byr", "iyr", ..., "ecl", "pid")
    return sum(1 for value in inp if all(substring in value for substring in substrings)))

The generator expression will, for each word W, yield 1 every time all subtrings inside substrings are contained inside W. Then you can sum all those 1's returning the result.

CodePudding user response：

I would consider this as bad code. Why? Because code quality is about readability and about whether or not the code is easy to understand and grasp, even for people who didn't write it originally.

You can write this much more beautiful with python's build in features, similar to this answer in stackoverflow:

def check_for_valid_passports(inp):
    num_of_valid_passports = 0
    for x in inp:
        if all(foo in x for foo in ('byr', 'iyr', ...)):
             num_of_valid_passports  = 1
    return num_of_valid_passports

Then you can improve this further. You have a for-loop with only one simple if statement here, so you might consider using list comprehension with something like this:

def check_for_valid_passports(inp):
    valid_items = ('byr', 'iyr', ...)
    return len([inps for inps in inp if all([foo in inps for foo in valid_items])])

This should actually work! For this one you might argue, that this is not an improvement in readability, so yeah... With a good comment (and better variables names) about what is done this may be fine, though.

One important aspect you SHOULD REALLY consider is improving your variables names! I couldn't come up with much better than "foo" and "foobar" here - why? because you use "inp" and "x" in the original code and gave me no glance of a chance to guess what that actually is? ;)