5
\$\begingroup\$

I'm debating with my colleagues which code is better: user_agent is a class instance. The class itself comes from a third-party library so we don't have any control over it.

1.

def _get_ua_device_type(user_agent):
    property_to_type = {'is_mobile': 'mobile', 'is_tablet': 'tablet', 'is_pc': 'pc', 'is_bot': 'bot'}
    for prop, type in property_to_type.items():
        if get_attr(user_agent, prop) is True:
            return type
    return 'unknown'

2.

def _get_ua_device_type(user_agent):
    for type in ('mobile', 'tablet', 'pc', 'bot'):
        if get_attr(user_agent, f'is_{type}') is True:
            return type
    return 'unknown'

3.

def _get_ua_device_type(user_agent):
    if user_agent.is_mobile:
        return 'mobile'
    if user_agent.is_tablet:
        return 'tablet'
    if user_agent.is_pc:
        return 'pc'
    if user_agent.is_bot:
        return 'bot'
    return 'unknown'

Things we discussed:

  1. Static code is easier to understand than dynamic.
  2. Static code is easier to test.
  3. Dynamic code is declarative and more idiomatic than metameric ifs.
  4. Dynamic code will be as hard/easy to understand regardless of the number of choices.
  5. It's hard to anticipate the number of choices up-front so why start with a more complex dynamic form?
  6. Assuming that predicate names are derived from type names is bad.
  7. Where is the line when code is spaghetti-like and should be rewritten in a more dynamic way?
  8. What about Zen of Python (“Explicit is better than implicit.“, “Flat is better than nested.“, “Readability counts.”, “Simple is better than complex.”)

Do you have your own thoughts/suggestions?

\$\endgroup\$

closed as off-topic by Peilonrayz, 200_success, Graipher, Toby Speight, t3chb0t Mar 27 at 15:45

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – Peilonrayz, 200_success, Graipher, Toby Speight, t3chb0t
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 3
    \$\begingroup\$ What is user_agent? \$\endgroup\$ – Peilonrayz Mar 26 at 15:18
  • 1
    \$\begingroup\$ @Graipher That's not why I'm asking for its name. The code may already have defined this function. Why recommend new code when you can just use say user_agent.agent for example. \$\endgroup\$ – Peilonrayz Mar 26 at 15:26
  • 2
    \$\begingroup\$ @mnowotka: Please add those details to the question, so it is not missing any context. \$\endgroup\$ – Graipher Mar 26 at 15:39
  • 2
    \$\begingroup\$ Also please don't modify your code after getting answers. Have a look at what you can and cannot do after receiving answers here: codereview.stackexchange.com/help/someone-answers \$\endgroup\$ – Graipher Mar 26 at 15:40
  • 1
    \$\begingroup\$ @Graipher - sorry about that, I'm still new here. I removed it because this is not the essence of my question. \$\endgroup\$ – mnowotka Mar 26 at 15:41
4
\$\begingroup\$

Equivalence considerations

1 is not the same as 2 or 3 for older versions of Python. In 2 and 3, you have an ordered execution sequence that determines the priority of returned variables. In 1, you have a dictionary whose order is inherently unknown. This might be OK for you, or it might not. If it isn't, a workaround is to use an OrderedDict.

For newer versions of Python (3.6+) this should not be an issue.

Drop boolean redundancy

No matter what you do, you should probably stop writing is True if you know that the target variable is already boolean. If the variable can have mixed type (i.e. integer or boolean), which is bad but sadly often possible in Python, and if you care about this, then you need to keep is True.

Ownership

Do you control the design of user_agent? If so, then you can enforce a stable interface, and my favourite of your options is 2, simply because it's more concise. If not, then only (3) would work well with static analysis to catch a changing interface.

Other options

try:
    return next(type for type in ('mobile', 'tablet', 'pc', 'bot')
                if get_attr(user_agent, f'is_{type}'))
except StopIteration:
    return 'unknown'

The bigger problem

is that you're representing an idea of a type with multiple booleans when it should be a single enum-style variable. If you can change your user agent to do this, do this. If not, you may want to consider writing a multivariate-boolean-to-single-enum shim class to sanitize the rest of your business logic.

\$\endgroup\$
  • 1
    \$\begingroup\$ 1 is the same as 2 and 3 in Python 3.5+ IIRC and f-strings are available in 3.6+. \$\endgroup\$ – Peilonrayz Mar 26 at 15:37
  • \$\begingroup\$ @Peilonrayz That's a consequence of some implementations that isn't guaranteed pre-Python 3.7. Read stackoverflow.com/questions/39980323/… . \$\endgroup\$ – Reinderien Mar 26 at 15:39
  • \$\begingroup\$ I removed the is True part because this is not a point of my question. \$\endgroup\$ – mnowotka Mar 26 at 15:39
  • 1
    \$\begingroup\$ @Reinderien: It is guaranteed in cPython 3.6+ and any Python implementation starting from Python 3.7+, as noted in the link you posted and from the question we can only assume any Python 3.6+, so technically you are not wrong. Nevertheless I think making your first point a bit milder might be a good idea. Something like "1 is not the same as 2 or 3, depending on your Python version and implementation". \$\endgroup\$ – Graipher Mar 26 at 15:49
2
\$\begingroup\$

This is a very good question. Thanks for asking.


Review

  • I'm ignoring is True as others have mentioned it.
  • Second code piece is reasonably better than first one, since you have further reduced duplicated information.
  • I prefer second one over others personally and it's readable for me.
  • Third one is too verbose. But it is readable nevertheless.

Boundaries should be clean

  • If you are using set of similar function to interact with user_agent it would be better to create a wrapper class such as UserAgent in your code.
    • It is a good practice to wrap third-party behaviour.
    • This gives you more control and it becomes easier for you to maintain your code even when you update third-party libraries.

Code at the boundaries needs clear separation and tests that define expectations. We should avoid letting too much of our code know about the third-party particulars. It’s better to depend on something you control than on something you don’t control, lest it end up controlling you.

Clean Code by Robert C. Martin

Is explicit better than implicit?

  • This should be carefully interpreted. It depend on the context. In your scenario even though third piece of code is more explicit it is poorer than other code because it has duplicate information therefore violating Don't Repeat Yourself.

Duplication (inadvertent or purposeful duplication) can lead to maintenance nightmares, poor factoring, and logical contradictions.

http://wiki.c2.com/?DontRepeatYourself

\$\endgroup\$
1
\$\begingroup\$

In your first two solutions you compare with is True in if get_attr(user_agent, prop) is True / if get_attr(user_agent, f'is_{type}') is True. Explicitly comparing to True/False is usually frowned upon in Python (contrary to "Explicit is better than implicit"), you want to accept any truthy or falsey value. So just do if get_attr(user_agent, f'is_{type}').

As to which is better, that is indeed a tough call. None of them are really nice, all of them have some disadvantages:

  1. I would only choose the first one if the mapping from attribute name to types was less regular.

  2. I would personally prefer number two. It is succinct and readable. All three implementations depend on the implementation of the user_name class, anyway, by hardcoding the names, so none of them is more robust in that regard. It also scales more easily with the number of cases, since you only need to add it to the tuple (maybe eventually pulling it out into a separate variable if there are too many).

  3. If there were less possibilities, the third one is definitely the most readable version. It is also the one where adding more types is the most tedious, since you copy&paste code around.

\$\endgroup\$
  • \$\begingroup\$ I removed the is True part because this is not a point of my question. \$\endgroup\$ – mnowotka Mar 26 at 15:39
  • 2
    \$\begingroup\$ @mnowotka: Nevertheless, I rolled back your edit because it invalidates the answers you already got (see my comment on the question for a link to the help center). Code Review allows answers to comment on any and all aspects of the code (even if that is not exactly what the asker wants to hear). \$\endgroup\$ – Graipher Mar 26 at 15:42
  • \$\begingroup\$ That's pitty because it distracts people from my actual question :( \$\endgroup\$ – mnowotka Mar 26 at 15:42

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