2
\$\begingroup\$

My code takes a list of dictionaries as an input. Each of these dictionaries contains a person identified by name and a region code region, as well as financial account information associated with that region code for that person. The output structure is supposed to be a dictionary with one element for each person. This element should be a dictionary that stores the account information for that person with the regions acting as the keys.

Here is my input data:

input_data = [
    {'name': 'kotler', 'type': 'CARD', 'currency': 'USD', 'region': 'UnitedStates'}, 
    {'name': 'kotler', 'type': 'All', 'currency': 'CAD', 'region': 'UnitedStates'}, 
    {'name': 'kotler', 'type': 'All', 'currency': 'AUD', 'region': 'UnitedStates'}, 
    {'name': 'Philip', 'type': 'CARD', 'currency': 'USD', 'region': 'UnitedStates'}]

The final output looks like this:

respo = {
    "kotler": {"UnitedStates": {"CARD": ["USD"], "All": ["CAD", "AUD"]}, "name": "kotler"}, 
    "Philip": {"UnitedStates": {"CARD": ["USD"]}, "name": "Philip"}}

This is my code that goes through the list input_data, and creates a new dictionary respo that restructures the input in the expected way so that it produces the correct output:

respo = {}

for i in input_data:
    if i.get("name") in respo.keys():
        if i.get("region") in respo[i.get("name")].keys():
            if i.get("type") in respo[i.get("name")][i.get("region")].keys():
                respo[i.get("name")][i.get("region")][i.get("type")].append(i.get("currency"))
            else:
                respo[i.get("name")][i.get("region")].update({i.get("type"):[i.get("currency")]})
        else:
            respo[i.get("name")].update({i.get("region"):{i.get("type"):[i.get("currency")]}})
    else:
        respo[i.get("name")] = {i.get("region"):{i.get("type"):[i.get("currency")]},"name":i.get("name")}

My method appears to be working. But the code isn't easy to read, and I think there may be ways to make it better. Any suggestions?

\$\endgroup\$
0

2 Answers 2

5
\$\begingroup\$

There are several possible improvements. First, I'd like to mention some points that pertain to the general style of your code. Then, I'm going to discuss some aspects of your code that are unnecessarily complex. I'll then show my alternative to your code, followed by a discussion of why I think your required output may not be optimal.

Formating and style

  1. Variable naming: input_data, respo, and i are all names that don't really help understanding your code. i is particularly misleading as it's a name that's usually used for integer variables e.g. in indexes or for loops. Instead, I'd suggest to use more telling names. Of course, this depends on the intended use of your code, which you didn't specify though.

  2. You use both single and double quotation marks to delimit strings. Either one is acceptable, but you should be consistent.

  3. More generally, your code formatting can be improved by paying attention to the Python style guide as described in PEP 8. In addition the first two points I made, it also discusses issues such as the use of whitespace after e.g. the key separator : as well as a suggested maximu line length of 79 characters.

Code

  1. If you want to check whether a dictionary contains a certain key, you don't need to call the keys() method. A simple test like if key in dct: is already sufficient.

  2. The use of get() for every dictionary access impairs the the readability of your code. The primary purpose of this method is that it doesn't raise a KeyError if the key doesn't exist, but returns a default value (None unless specified otherwise). This can be useful if your input data isn't always well-formed (e.g. if one of the elements may contains key-value pairs for name and region, but not for account_type or currency). However, for your example data, that's not really the case, so there's nothing to be gained from using the less explicit get().

  3. Many of your if instructions are unnecessary, at least for the input data that you've provided. For instance, every element in your input list has a region field. Hence, it's guaranteed that the dictionaries stored in your output structure will also contain the region code. But still, your code contains an explicit test for that, i.e. if i.get("region") in respo[i.get("name")].keys(). If you can trust that your input data will always be complete, you can leave out this if condition.

  4. Related to the previous point, you may want to use defaultdict from the collections module. This data type is an expansion of the dict type: if you try to access an element using a non-existing key, the interpreter doesn't raise a KeyValue exception but automatically creates a default element for that key. For details, please consult the documentation.

Suggested alternative

Here is my suggested alternative for your code. It conforms to the PEP 8 coding style guidelines by using consistent string quote characters, more useful names, proper whitespace and appropriate overall formatting. It also makes the handling of your list and dictionaries more explicit by (a) storing values that are used more than once in variables and (b) by using defaultdict where appropriate.

from collections import defaultdict

customers = {}

for account in input_data:
    name = account["name"]
    region = account["region"]
    account_type = account["type"]
    currency = account["currency"]

    if name not in customers:
        customers[name] = {"name": name, region: defaultdict(list)}

    customers[name][region][account_type].append(currency)

Comment on expected output

I don't know how you're going to use the output structure (respo in your question, and customers in my solution), but there's one thing that I noticed: The field name in the dictionaries that are stored in your output variable stores the same string that is used as a key to access that dictionary. This redundancy is something of a code smell: typically, if you can access one of the dictionaries stored in respo, you already know the value of name, so why store it? Now you'll have to make sure that whenever either name or the key in respo changes, the other value changes as well.

It also makes handling the data contained in the dictionary more complicated. For instance, if you want to process all customers in a tabular form, you'll always have to include a special case that filters out the name key:

print(f"{'NAME':15} {'REGION':15} {'TYPE':15} CURRENCIES")

for name in customers:
    for key in customers[name]:
        # the next if condition is only needed due to the redundant key:
        if key != "name":   
            for account_type in customers[name][key]:
                currencies = customers[name][region][account_type]
                print(f"{name:15} {region:15} {account_type:15} "
                      f"{', '.join(currencies)}")

If you don't store the name in customers again, i.e. if there was no name key for each dictionary stored in customers, you could easily change the transformation code so that customers is also a defaultdict, so my solution could be simplified even more, like so:

from collections import defaultdict

customers = defaultdict(lambda: defaultdict(lambda: defaultdict(list)))

for account in input_data:
    name = account["name"]
    region = account["region"]
    account_type = account["type"]
    currency = account["currency"]
    customers[name][region][account_type].append(currency)

print(f"{'NAME':15} {'REGION':15} {'TYPE':15} CURRENCIES")

for name in customers:
    for region in customers[name]:
        for account_type in customers[name][region]:
            currencies = customers[name][region][account_type]
            print(f"{name:15} {region:15} {account_type:15} "
                  f"{', '.join(currencies)}")

As you can see, both generating the structure as well as traversing through it becomes almost trivial now.

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

This isn't any better interms of doing the same thing with less code, but it does remove the nesting making it much cleaner.

respo = {}
for i in input_data:
    name = i.get("name")
    region = i.get("region")
    my_type = i.get("type")
    currency = i.get("currency")

    if name not in respo.keys():
        respo[name] = {region: {my_type: [currency]}, "name": name}
        continue

    if region not in respo[name].keys():
        respo[name].update({region: {my_type: [currency]}})
        continue

    if my_type not in respo[name][region].keys():
        respo[name][region].update({my_type: [currency]})
        continue

    respo[name][region][my_type].append(currency)
\$\endgroup\$
1
  • 3
    \$\begingroup\$ Welcome to Code Review! You have presented an alternative solution, but haven't reviewed the code. Please edit to show what aspects of the question code prompted you to write this version, and in what ways it's an improvement over the original. It may be worth (re-)reading How to Answer. \$\endgroup\$ Mar 15 at 12:00

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.