SIM106: Handle error cases early

[MartinThoma]

Explanation

Exceptions and error cases should be handled early to prevent deeply-nested code.

Example

# Bad
if cond:
    a
    b
    c
else:
    raise Exception

# Good
if not cond:
    raise Exception

a
b
c

[MartinThoma]

    If(
        test=Name(id='cond', ctx=Load()),
        body=[
            Expr(
                value=Name(id='a', ctx=Load()),
            ),
            Expr(
                value=Name(id='b', ctx=Load()),
            ),
            Expr(
                value=Name(id='c', ctx=Load()),
            ),
        ],
        orelse=[
            Raise(
                exc=Name(id='Exception', ctx=Load()),
                cause=None,
            ),
        ],
    ),

[MartinThoma]

        If(
            test=Name(id='cond', ctx=Load()),
            body=[
                Expr(
                    value=Name(id='a', ctx=Load()),
                ),
                Expr(
                    value=Name(id='b', ctx=Load()),
                ),
                Expr(
                    value=Name(id='c', ctx=Load()),
                ),
            ],
            orelse=[
                Raise(
                    exc=Name(id='Exception', ctx=Load()),
                    cause=None,
                ),
            ],
        ),

[MartinThoma]

https://github.com/MartinThoma/flake8-simplify/commit/d998c5e280b059b5f4defc76935fca8eb843c682

[GitRon]

Hi @MartinThoma! I just ran into another thing :)

I often use a "else Exception" when I want to make 110% sure to avoid any definition gaps. I will quite obscurify my code if I do the (never happens) case on top with all the things that my variable might not be.

if a == 1:
    do_stuff()
if a == 2:
    do_stuff()
if a == 3:
    do_stuff()
if a == 4:
    do_stuff()
else:
    raise RuntimeError('Invalid paramter')

Any ideas about that? 😃

[MartinThoma]

Oh, interesting! I had this with NotImplemented ( https://github.com/MartinThoma/flake8-simplify/issues/14 ), but I can see that you might want to do this with parameters as well.

In this simple example, I would actually write it like this:

if a not in [1, 2, 3, 4]:
    raise ValueError(f'Invalid parameter a={a}')

if a == 1:
    do_stuff1()
if a == 2:
    do_stuff2()
if a == 3:
    do_stuff3()
if a == 4:
    do_stuff4()

But I guess your real case is more complicated? Could you maybe share an example so that I can get a better understanding?

[GitRon]

Hi @MartinThoma - it's not more complicated I think but I really don't like creating a long list for the exit condition. Especially when the numbers are constants, which are very long to write down like MyModel.SomeValueChoice.MY_FANCY_CHOICE. If you have many of them, the exit condtion will be super unreadable. So IMHO I think it's perfectly ok to write it the way it is shown in my example. 😃

[MartinThoma]

I can see your point. As a general remark: Although I try to make this plugin "agreeable" in the sense that typically one can take the rules as they are, such cases will always come. Please take the rules as an opinionated recommendation. It is absolutely understandable if you disable rules that don't fit your general style.

Specifically, in this case, I have a hard time seeing how this could become hard to read if you also use the black formatter. You can give this a name and then do this:

valid_values = [
    MyModel.SomeValueChoice.MY_FANCY_CHOICE1,
    MyModel.SomeValueChoice.MY_FANCY_CHOICE2,
    MyModel.SomeValueChoice.MY_FANCY_CHOICE3,
    MyModel.SomeValueChoice.MY_FANCY_CHOICE4,
]

if a not in valid_values:
    raise ValueError(f"Invalid parameter a={a}")

if a == MyModel.SomeValueChoice.MY_FANCY_CHOICE1:
    do_stuff1()
if a == MyModel.SomeValueChoice.MY_FANCY_CHOICE2:
    do_stuff2()
if a == MyModel.SomeValueChoice.MY_FANCY_CHOICE3:
    do_stuff3()
if a == MyModel.SomeValueChoice.MY_FANCY_CHOICE3:
    do_stuff4()

You can even combine this with Literal values for type checking

[GitRon]

@MartinThoma True, this might be the way to go. Still, I'd duplicate all choices so I doesn't feel so DRY anymore. In a regular function, I'd agree that exit conditons should go first to avoid nested if-conditions. But in an if? Why you think that it's a bad pattern? 😃

[MartinThoma]

In a regular function, I'd agree that exit conditons should go first to avoid nested if-conditions.

Thank you! I was all the time feeling that something is missing in that rule - that was it! It needs a check that it's the first thing in a function (maybe allowing few exceptions :thinking: ... hm, not sure )

[GitRon]

Cool, glad I could help 😃

Another question: The other day I was trying out some new rules for the https://github.com/rocioar/flake8-django package. Do you maybe have experience with flaking django model attributes?

[alk-acezar]

Other solution:

handlers = {
    MyModel.SomeValueChoice.MY_FANCY_CHOICE1: do_stuff1,
    MyModel.SomeValueChoice.MY_FANCY_CHOICE2: do_stuff2,
    MyModel.SomeValueChoice.MY_FANCY_CHOICE3: do_stuff3,
    MyModel.SomeValueChoice.MY_FANCY_CHOICE4: do_stuff4,
}

handler = handlers.get(a, lambda: ValueError(f"Invalid parameter a={a}"))
handler()

I it's OK for you I'd like to contribute a pr where simple if/else raises SIM106 while if/elif/else doesn't. Does it looks like a good compromise?

[MartinThoma]

@alk-acezar I'm unhappy with SIM106. What do you think abou removing it altogether? Does it actually provide value?

[ghost]

It's useful in some cases IMO, but I do find myself having to add # noqa: SIM106 many times in cases where I would have to repeat the list of possible values at the beginning.