20
\$\begingroup\$

I have a use case in which I need to check whether a given value is explicitly True or False:

def stringify(value):
    """Returns the string representation of the value."""

    if value is None:
        return '-'

    if value is True:
        return '✓'

    if value is False:
        return '✗'

    return str(value)

I know, that in most cases, where you just need the truthiness, you'd just do if value:. But here this will not suffice, since I want zero ints and floats to be displayed in their decimal form as well as empty lists, sets dicts as such. Is the above code acceptable or should I rather check using isinstance(value, bool) first and then do if value:?

What's the least surprising way to go?

Usage Context
The function is part of an API that converts database records to terminal output:
MariaDB → peewee → API → terminal

Example output:

$ termutil ls -T 1000
  ID  TID         CID  VID  OS                     IPv4 Address               Deployed  Testing  Tainted  Address                                           Annotation                      
 214    1        1000    -  HIDSL latest              XX.Y.Z.75                      -        ✓        ✗  Musterstraße 42, 123456 Irgendwo                      -                               
 215    2        1000    -  HIDSL latest              XX.Y.Z.76                      -        ✓        ✗  Musterstraße 42, 123456 Irgendwo                      -                               
 216    3        1000    -  HIDSL latest              XX.Y.Z.77                      -        ✓        ✗  Musterstraße 42, 123456 Irgendwo                      -                               
 217    4        1000    -  HIDSL latest              XX.Y.Z.78                      -        ✓        ✗  Musterstraße 42, 123456 Irgendwo                      -                               
 218    5        1000    -  HIDSL latest              XX.Y.Z.79                      -        ✓        ✗  Musterstraße 42, 123456 Irgendwo                      -                               
 219    6        1000    -  HIDSL latest              XX.Y.Z.80                      -        ✓        ✗  Musterstraße 42, 123456 Irgendwo                      -                               
 330    7        1000  399  HIDSL latest             XX.Y.Z.182                      -        ✗        ✗  Musterstraße 42, 123456 Irgendwo                      -                               
 508    8        1000    -  HIDSL latest             XX.Y.Z.189                      -        ✗        ✗  N/A                                                   -                       
\$\endgroup\$
6
  • 2
    \$\begingroup\$ Personally not fond of the end case here. What if I pass in '-', '✓', or '✗'? The returned value will be ambiguous. Or you could specify that you don't accept strings in your docstring or type contract. \$\endgroup\$ Commented Jul 19, 2018 at 13:44
  • 4
    \$\begingroup\$ @LukeSawczak You'll always have ambiguity when converting objects to strings if the object may be a string. Your example also applies to []"[]" and "[]""[]" etc. \$\endgroup\$ Commented Jul 19, 2018 at 13:52
  • \$\begingroup\$ -- Unless you catch that case and have defined behaviour for it :) \$\endgroup\$ Commented Jul 19, 2018 at 13:54
  • \$\begingroup\$ @LukeSawczak I don't mind this kind of ambiguity in my use case. \$\endgroup\$ Commented Jul 19, 2018 at 13:55
  • 11
    \$\begingroup\$ The best answer is in your question. I see no improvements below. \$\endgroup\$ Commented Jul 19, 2018 at 18:06

6 Answers 6

23
\$\begingroup\$

Because the code snippet is so small and missing any other context, it's hard to identify any problems in scaling / that may arise elsewhere in the code. Thus if you want a more tailored answer, you should add more code.

Yes, this is fine.

Regarding the isinstance() check, no. That only adds an extra level of indentation without justification.

\$\endgroup\$
0
20
\$\begingroup\$

Personally I'd use a dictionary, as they're pretty much the standard switch statement. If you just use a normal dictionary with just True as the key, then it has the down side of returning '✓' for both True and 1. And so you can make a dictionary with the key being a tuple of the value and the type.

_stringify_values = {
    (type(None), None): '-',
    (bool, True): '✓',
    (bool, False): '✗'
}

def stringify(value):
    try:
        return _stringify_values[(type(value), value)]
    except (KeyError, TypeError):
        return str(value)

You can also simplify the input if you want to:

_stringify_values = {
    None: '-',
    True: '✓',
    False: '✗'
}
_stringify_values = {(type(k), k): v for k, v in _stringify_values.items()}

If you plan on using this a lot, you can also make a simple class to contain all the special logic too. By using collections.abc.Mapping:

import collections


class TypedMapping(collections.abc.Mapping):
    def __init__(self, values):
        self._dict = {
            (type(k), k): v
            for k, v in values
        }

    def __getitem__(self, key):
        try:
            return self._dict[(type(key), key)]
        except TypeError as e:
            raise KeyError(str(e)) from e

    def __len__(self):
        return len(self._dict)

    def __iter__(self):
        return (k for _, k in self._dict)

stringify = TypedMapping([
    (None, '-'),
    (True, '✓'),
    (False, '✗'),
    (1, 'one')
])

for i in (None, True, False, 1, 'True', {}, ()):
    print(stringify.get(i, str(i)))
\$\endgroup\$
4
  • 32
    \$\begingroup\$ This in over-engineered solution looking for a problem where none exist. \$\endgroup\$
    – Will
    Commented Jul 20, 2018 at 1:32
  • \$\begingroup\$ The general overengineering aside, why in the world would you catch a KeyError eather than using return _stringify_values.get((type(value), value), str(value)) \$\endgroup\$ Commented Jul 20, 2018 at 6:35
  • 2
    \$\begingroup\$ @JohannesPille Because there is already the need to catch for TypeError when the key is unhashable; so no need to make the line less readable. \$\endgroup\$ Commented Jul 20, 2018 at 7:11
  • 2
    \$\begingroup\$ @Will I don't think using a dictionary and a try is over-engineered. The class is, which is why I prefaced it with "if you plan on using this a lot" . \$\endgroup\$
    – Peilonrayz
    Commented Jul 20, 2018 at 9:02
14
\$\begingroup\$

Even though return cuts the chain short if one matches, I would prefer to use an elif chain here, because it is more obvious, but YMMV (especially with automatic linters):

def stringify(value):
    """Returns the string representation of the value."""
    if value is None:
        return '-'
    elif value is True:
        return '✓'
    elif value is False:
        return '✗'
    return str(value)

Apart from that there is no real alternative, because all other approaches will fail on the difference between 0 and False, which you can only get with is or on unhashable types (like list).

The only other possibility I see is replacing the values for True, False, None after the conversion to str, unsing str.translate, but this would be unable to differentiate None and "None".

\$\endgroup\$
8
  • 2
    \$\begingroup\$ I actually just got rid of the elifs because of the noise that pylint was making from it. \$\endgroup\$ Commented Jul 19, 2018 at 11:56
  • 5
    \$\begingroup\$ That's a case for telling the linter to be quiet, rather than modifying your code to placate it. \$\endgroup\$
    – chepner
    Commented Jul 19, 2018 at 15:38
  • 1
    \$\begingroup\$ pylint's default settings are terrible. \$\endgroup\$ Commented Jul 19, 2018 at 19:32
  • 1
    \$\begingroup\$ IMO using elif actually makes the code a little worse, because it implies a reachable else branch. Introducing tautologies like that doesn't clarify anything to me: each time I read code with a redundant statement like that I lose a liiittle bit of faith in whoever wrote that having a clear mental model of their own control flow. Code can be "too clever", but it can also sometimes be too ...dumb? \$\endgroup\$
    – Will
    Commented Jul 20, 2018 at 1:22
  • 1
    \$\begingroup\$ @Will As I said, YMMV. I generally agree with you, but here I like it because it is clear at a glance that the cases are mutually exclusive. Somehow that is less obvious for me with the original if chain. \$\endgroup\$
    – Graipher
    Commented Jul 20, 2018 at 6:01
9
\$\begingroup\$

I might use nested conditional expressions, which can be formatted rather cleanly:

def stringify(value):
    return ('-' if value is None else
            '✓' if value is True else
            '✗' if value is False else
            str(value))

For a small set of hard-coded checks, the readability outweighs the linear search to find the matching case.

\$\endgroup\$
1
  • \$\begingroup\$ -1 for the inline conditionals, but +2 for "For a small set of hard-coded checks, the readability outweighs the linear search to find the matching case.". Could not agree more. \$\endgroup\$
    – Quelklef
    Commented Jul 19, 2018 at 16:53
6
\$\begingroup\$

A remark about the function name and the docstring:

def stringify(value):
   """Returns the string representation of the value."""
  • stringify: May or may not be okay, possibly to_string would be nicer.
  • value: What value? Does it have to be a numeric value? A value of any kind, but no object? The variable name does not make this fully clear

Returns the string representation of the value.

This does not document what the function does, because after reading I do not know ...

  • What values are allowed? Are there any restrictions?
  • What defines a "string representation" of a value? for floats alone there are a lot of possible representations. Nested structures, objects and so on can have many possible representations as well, from [object] up to a full serialization.
  • Why should I use this function instead of str/unicode? What is the difference?
  • Is there any special handling for some data types? I do not know, that your representation of True will be an unicode character and not "True", "1", or "true" which would follow the principle of least surprise.

In addition it mixes a special handling for bool values with a default handling for everything else. Possibly it should be bool_to_unicodesymbol and raise a ValueError if the input is no boolean value.

\$\endgroup\$
2
\$\begingroup\$

To remove the ambiguity resulting from inputs like '-' and '[1, 2]', I suggest replacing your str(value) call with a repr(value) call.

If you wish to extend this to the point where a linear search might cause unnecessary delays, you will need to use the id of these singletons to avoid issues when stringifying unhashable types:

try:
    # Using the id() of singletons to support unhashable types
    return appropriately_named_lookup_table[id(value)]
except KeyError:
    return repr(value)

Make sure you document this if you end up doing it, and keep in mind that although it might seem like it works for integers in range(-5, 256) this is a CPython implementation detail only.

\$\endgroup\$
5
  • \$\begingroup\$ +1 for considering repr , but the code seems me more useful for "user" than "developer" \$\endgroup\$ Commented Jul 20, 2018 at 6:47
  • 1
    \$\begingroup\$ The OP said that ambiguity was unavoidable. \$\endgroup\$
    – wizzwizz4
    Commented Jul 20, 2018 at 6:50
  • \$\begingroup\$ As I commented above, the ambiguity is not a problem at all. On the contrary, I don't want the ' surriunding my strings. \$\endgroup\$ Commented Jul 20, 2018 at 13:00
  • \$\begingroup\$ You could use a defaultdict instead of catching exceptions. \$\endgroup\$
    – allo
    Commented Jul 23, 2018 at 7:33
  • \$\begingroup\$ @allo Really? I didn't know that supported passing an argument! But no, since it'd have to use a ctypes cast from the id, which just happens to be the address in CPython but isn't required to. \$\endgroup\$
    – wizzwizz4
    Commented Jul 23, 2018 at 21:47

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