1
\$\begingroup\$

This code works and does exactly what I want it to, but I feel that it is inelegant. It seems like there should be a way to use get() and/or setdefault(), for example. All comments and suggestions are appreciated!

Here is a function that builds a dict. The dict's values are also dicts. Some of the values are empty dicts {} and some are dicts that have a {key:value} pair.

def my_function(my_string, my_key):

    d = {}

    for i in range(3):
        d['test' + str(i)] = {}

    d['test3'] = {'a_key': 'a_value'}

    # d = { 'test0': {}, 'test1': {}, 'test2': {}, 'test3': {'a_key': 'a_value'} }

    if my_string != '':
        d[my_key] = {my_string: None}

    else:
        # check if d[my_key] exists
        try:
            # d[my_key] exists and it is not empty, don't overwrite
            if d[my_key]:
                pass
            # d[my_key] exists, but it is empty, write NULL
            else:
                my_string = 'NULL'
                d[my_key] = {my_string: None}

        # d[my_key] did not exist, create it and write NULL
        except KeyError:
            my_string = 'NULL'
            d[my_key] = {my_string: None}

    return d


# my_string not blank, my_key is new
f = my_function('hello', 'test4')
print(f)

# my_string not blank, my_key exists
f = my_function('hello', 'test0')
print(f)

# my_string blank, my_key exists and its value is not empty
f = my_function('', 'test3')
print(f)

# my_string blank, my_key exists and its value is empty
f = my_function('', 'test0')
print(f)

# my_string blank, my_key is new
f = my_function('', 'test4')
print(f)

Typical (and desired) output:

{'test2': {}, 'test3': {'a_key': 'a_value'}, 'test0': {}, 'test1': {}, 'test4': {'hello': None}}
{'test2': {}, 'test3': {'a_key': 'a_value'}, 'test0': {'hello': None}, 'test1': {}}
{'test2': {}, 'test3': {'a_key': 'a_value'}, 'test0': {}, 'test1': {}}
{'test2': {}, 'test3': {'a_key': 'a_value'}, 'test0': {'NULL': None}, 'test1': {}}
{'test2': {}, 'test3': {'a_key': 'a_value'}, 'test0': {}, 'test1': {}, 'test4': {'NULL': None}}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I hope you get some helpful reviews. \$\endgroup\$ – SirPython Jan 21 '16 at 0:57
  • \$\begingroup\$ @SirPython Thanks; I hope so too. It's kind of a weird question, but it is real code that I use, and it does work, so I hope it fits the requirements for Code Review. \$\endgroup\$ – Riccati Jan 21 '16 at 3:14
  • \$\begingroup\$ By the way, everything under def and up to and including return should be indented one more tab, but I couldn't find an easy way to do that in the online editor. \$\endgroup\$ – Riccati Jan 21 '16 at 3:19
  • \$\begingroup\$ @Riccati Generally the easiest way to do that is to write up your full question outside of the editor. :P \$\endgroup\$ – SuperBiasedMan Jan 21 '16 at 9:39
  • \$\begingroup\$ Thank you all for these very useful answers! It will take me some time to think about them, but after I do I will post comments, mark one as accepted, etc. \$\endgroup\$ – Riccati Jan 21 '16 at 16:42
4
\$\begingroup\$

Tests

It's a good thing you've provided tests and expected output so that one can check the behavior, it is even better if one can run the tests without having to check that the output is correct. Only a tiny bit of reorganisation is required to get :

# my_string not blank, my_key is new
assert my_function('hello', 'test4') == {'test2': {}, 'test3': {'a_key': 'a_value'}, 'test0': {}, 'test1': {}, 'test4': {'hello': None}}

# my_string not blank, my_key exists
assert my_function('hello', 'test0') == {'test2': {}, 'test3': {'a_key': 'a_value'}, 'test0': {'hello': None}, 'test1': {}}

# my_string blank, my_key exists and its value is not empty
assert my_function('', 'test3') == {'test2': {}, 'test3': {'a_key': 'a_value'}, 'test0': {}, 'test1': {}}

# my_string blank, my_key exists and its value is empty
assert my_function('', 'test0') == {'test2': {}, 'test3': {'a_key': 'a_value'}, 'test0': {'NULL': None}, 'test1': {}}

# my_string blank, my_key is new
assert my_function('', 'test4') == {'test2': {}, 'test3': {'a_key': 'a_value'}, 'test0': {}, 'test1': {}, 'test4': {'NULL': None}}

# New test from Sjoerd Job Postmus's comment
assert my_function('something not empty', 'test3') == {'test1': {}, 'test0': {}, 'test3': {'something not empty': None}, 'test2': {}}

Improvements

if my_string != '': could be rewritten more concisely : if my_string:.

Also, you could use get to retrievement elements in a "safe" way.

Thus, the code becomes :

if my_string:
    d[my_key] = {my_string: None}
else:
    if not d.get(my_key):
        d[my_key] = {'NULL': None}
return d

Also, your dict initialisation could be done with a dictionnary comprehension. Then, your whole function would become:

def my_function(my_string, my_key):
    d = {'test' + str(i): {} for i in range(3)}
    d['test3'] = {'a_key': 'a_value'}

    if my_string:
        d[my_key] = {my_string: None}
    elif not d.get(my_key):
        d[my_key] = {'NULL': None}
    return d

Is this as simple as it can be ? Not quite ? The check for d.get(my_key) is actually to distinguish "test3" from the other values. It could be easier to just do the obvious thing instead of trying to retrieve stuff from the dict :

def my_function(my_string, my_key):
    d = {'test' + str(i): {} for i in range(3)}
    d['test3'] = {'a_key': 'a_value'}
    if my_string:
        d[my_key] = {my_string: None}
    elif my_key != 'test3':
        d[my_key] = {'NULL': None}
    return d
\$\endgroup\$
  • 1
    \$\begingroup\$ When you move the d['test3'] = {'a_key': 'a_value'} to the bottom, you actually change the behavior of my_function('something not empty', 'test3'). \$\endgroup\$ – Sjoerd Job Postmus Jan 21 '16 at 12:53
2
\$\begingroup\$

I think that defaultdicts are more suited for what you are trying to achieve.

You build one with a factory for missing values, and each time you access missing elements the factory is called to create a default one.

You basically want to create empty dictionaries for missing values:

from collections import defaultdict

my_dict = defaultdict(dict)
my_dict['test3']['a_key'] = value

print(my_dict) # defaultdict(<class 'dict'>, {'test3': {'a_key': 'value'}})

Your utility function can thus only check for the second key validity and insert into the defaultdict regardless of whether the primary key already exist or not.

def my_function(my_string, my_key):
    d = defaultdict(dict)
    # Populate there with default values if need be
    ...
    # Write the new dictionary
    d[my_key][my_string if my_string != '' else 'NULL'] = None
    return d

If you want to not overwrite values that are already defined, you can use setdefault which will insert None (or any other value if you use the 2 parameters call) when the key is not defined and not touch the value otherwise:

def my_function(my_string, my_key):
    d = defaultdict(dict)
    # Populate there with default values if need be
    ...
    # Write the new dictionary
    d[my_key].setdefault(my_string if my_string else 'NULL')
    return d

There are a few more things about your original code:

  • if my_string != '' is better written if my_string. Empty string are evaluated to False in a boolean context, every other ones are True.
  • You can remove the redundancy involved in your try .. except by defining new_dict = {'NULL': None} before the try and assigning d[my_key] = new_dict when you need to.
  • You can improve naming a whole lot by choosing names that indicate what functions and variables do rather than what they are.

But the worst problem is that the behaviour of your function seems odd. Namely, you create a new dictionary each time you call it, so there is no content in it, so no need to worry about existing values. I don't get the purpose at all. Perhaps you could pass the dictionary as a parameter and modify its content in place.

Something like:

def set_default(storage, category, key):
    storage[category].setdefault(key if key else 'NULL')

associated with:

def create_default_storage():
    storage = defaultdict(dict)
    for i in range(3)
        storage['test'+str(i)]
    storage['test3']['a_key'] = 'value'
    return storage

and you can use them like:

storage1 = create_default_storage()
set_default(storage1, 'test4', 'hello')
set_default(storage1, 'test2', '')

storage2 = create_default_storage()
set_default(storage2, 'test3', 'a_key')
set_default(storage2, 'test0', '')

print(storage1)
print(storage2)

and the output is:

defaultdict(<class 'dict'>, {'test4': {'hello': None}, 'test3': {'a_key': 'value'}, 'test0': {'NULL': None}, 'test1': {}, 'test2': {}})
defaultdict(<class 'dict'>, {'test0': {'NULL': None}, 'test3': {'a_key': 'value'}, 'test2': {}, 'test1': {}})
\$\endgroup\$
2
\$\begingroup\$

There are indeed some inelegant things here that can be shortened. For instance, you build d using a loop, but you could use a dictionary comprehension. They're just like list comprehensions if you've ever used them, a for loop collapsed into a single expression that's used to build a dictionary (in this case). Here's how it would look:

d = {'test' + str(i): {} for i in range(3)}

This will basically set each key value pair as 'test' + str(i) for the key and {} for the value for each i in range(3). Just like you originally had, but now all in one line.

A more elegant way to test if a string is empty is to use if my_string. This returns False for an empty string, but True for any other string. You could also prevent having to nest a level of indentation by using return from in here, since you don't need to do anything else if my_string != '':

if my_string:
    d[my_key] = {my_string: None}
    return d

Instead of pass, I'd use return d again in your if d[my_key] block as that communicates what's happening clearly again.

In the else block, instead of having the duplicate code you could just raise KeyError so that the except block gets used. As for that except block, there's no need to set my_string and then use it in the assignment, just use the string literal:

    except KeyError:
        d[my_key] = {'NULL': None}

Another note, your test if d[my_key] as it's currently written will only be False if my_key is 'test3', because any other existing value will just get an empty dictionary that evaluates as False. It's not entirely clear if that's the intended behaviour or you just weren't aware of Python's truthiness, but d['test0'] returing {} will evaluate as False.

\$\endgroup\$
  • \$\begingroup\$ You put all the points I would. But wouldn't the else with the except be more readable if they used an elif rather than a else try except: elif my_key not in d or not d[my_key]:. \$\endgroup\$ – Peilonrayz Jan 21 '16 at 10:52
  • \$\begingroup\$ @JoeWallis Actually I think if not d.get(my_key) is the clearest, which Josay suggested in their answer. \$\endgroup\$ – SuperBiasedMan Jan 21 '16 at 10:58
  • \$\begingroup\$ After reading their answer I would agree it's the clearest. Silly me \$\endgroup\$ – Peilonrayz Jan 21 '16 at 11:02
  • \$\begingroup\$ @JoeWallis Silly me, I didn't recommend anything for it. :P \$\endgroup\$ – SuperBiasedMan Jan 21 '16 at 11:02
0
\$\begingroup\$

Thank you all for your interesting comments.

Testing with assert is very helpful.

if my_string: is nice too.

So is the dict comprehension.

But the thing I was struggling with the most was the part that was solved by:

elif not d.get(my_key):
    d[my_key] = {'NULL': None}

Very nice! I knew there was something I could do with d.get() but I wasn't sure what it was. And it took me several minutes to fully understand elif not d.get(my_key): even when you pointed it out to me!

I did not appreciate that one could use the fact that d.get(my_key) returns None if my_key is not in d and also returns None if d.get(my_key) == {}.

Apologies to Mathias Ettinger. There is more to the story of this code and that is why it is hard to understand why I want a function like my_function. But I thought it was best to strip out the non-essentials and focus on the dict operations.

I will upvote all the answers and accept Josay's.

Here is the improved code:

def my_function(my_string, my_key):

    d = {'test' + str(i): {} for i in range(3)}
    d['test3'] = {'a_key': 'a_value'}

    # d = { 'test0': {}, 'test1': {}, 'test2': {}, 'test3': {'a_key': 'a_value'} }

    # my_string is not empty
    if my_string:
        d[my_key] = {my_string: None}

    # my_string is empty
    # not d.get(my_key)  is True  if my_key is not in d
    # not d.get(my_key)  is True  if my_key is {}
    elif not d.get(my_key):
        d[my_key] = {'NULL': None}

    return d
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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