Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Complexity really high for some types of code. #1494

Closed
Dreamsorcerer opened this issue Jul 4, 2020 · 2 comments
Closed

Complexity really high for some types of code. #1494

Dreamsorcerer opened this issue Jul 4, 2020 · 2 comments
Labels
bug Something isn't working

Comments

@Dreamsorcerer
Copy link
Contributor

Dreamsorcerer commented Jul 4, 2020

I'm finding certain kinds of code produce a score above the default 14, even though they don't look that complex to me, and I would really struggle to simplify them without making the code harder to read and maintain. The 2 cases that often seem to do this for me is translations and sqlalchemy queries.

So, I'm just wondering if there's some improvements that should be made to the complexity checker, or do I just need to increase the maximum? I have no familiarity with the complexity checker, so I'm unsure if this is desired behaviour or not.

Examples:
climate = {"camp": _("Camp"), "dog": _("Dog"), "off": _("Off")}
15

changed = any(getattr(old_vehicle, k) != v for k, v in vehicle.items())
17

t = self._table_vehicles
where = sa.and_(t.c.user_id == user_id, t.c.id == vehicle_data["id"])

second line scores 17.

ret = await conn.execute(t.select().where(t.c.user_id == user_id))
16

where = sa.and_(vehicles.c.user_id == user_id, vehicles.c.id == vehicle)
15

await conn.execute(t.update().where(where).values(tracking_strategy=form["strategy"]))
16

v["state_str"] = states.get(v["state"], v["state"].capitalize())
15

I've also found a function that wemake is reporting with a score of 43! Yet, not a single line in the function exceeds the threshold of 14. I thought a function score would be something like the average of all lines in the function. mccabe (C901) reports this same function as 10 (which sounds reasonable to me). If you want to look over that function as well, it is:

    async def _check_typing(self, api_data: Mapping[str, object],
                            dict_type: Type[Mapping[str, object]]) -> None:
        ann = get_type_hints(dict_type)

        for k, v in api_data.items():
            if k not in ann:
                logging.warning("Key missing from %s: %s (%s)", dict_type, k, v)  # noqa: WPS323
                continue

            ann_type = ann[k]
            if get_origin(ann_type) is Literal:
                if not any(v == arg for arg in get_args(ann_type)):
                    logging.warning("Missing Literal value for (%s) %s: %s", dict_type, k, v)  # noqa: WPS323
                continue
            elif get_origin(ann_type) is list:
                v = cast(List[object], v)
                if v and not isinstance(v[0], get_args(ann_type)[0]):
                    logging.warning("Wrong type for (%s) %s: %s", dict_type, k, v)  # noqa: WPS323
                continue
            elif get_origin(ann_type) is None and issubclass(ann_type, dict):  # TypedDict
                await self._check_typing(cast(Dict[str, object], v), ann_type)
                continue

            if get_origin(ann_type) is Union:
                ann_type = get_args(ann_type)

            if not isinstance(v, ann_type):
                logging.warning("Incorrect annotation for (%s) %s: %s", dict_type, k, v)  # noqa: WPS323
@Dreamsorcerer Dreamsorcerer added the bug Something isn't working label Jul 4, 2020
@sobolevn
Copy link
Member

Hi! Thanks a lot for your report!

So, I'm just wondering if there's some improvements that should be made to the complexity checker, or do I just need to increase the maximum?

The algorithm works correctly, let's talk aout interpreting its results.

climate = {"camp": _("Camp"), "dog": _("Dog"), "off": _("Off")}

This line is considered a hard one, because _ is a call. Let's say that it is not i18n. Then this line is complex!
Solution:

climate = {
    "camp": _("Camp"), 
    "dog": _("Dog"), 
    "off": _("Off"),
}

Let's take some other example:

await conn.execute(t.update().where(where).values(tracking_strategy=form["strategy"]))

Solution:

await conn.execute(
    t.update().where(where).values(
         tracking_strategy=form["strategy"],
    )
)

Way more readable, isn't it?

But, you can always just increase the complexity threshold. 👍

@Dreamsorcerer
Copy link
Contributor Author

I don't think I realised that it was looking at physical lines, rather than code lines. So, those examples make sense.

I'm still a little confused on my last example about function complexity though. Normally the mccabe and wemake complexities produce similar numbers (±2), but in this example wemake gives a score of 43 versus mccabe's 10. My target is usually 5, so this is obviously a complex function, but the result of 43 just seems unreasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants