8
\$\begingroup\$

I just found a now deleted question on Stack Overflow and solved the problem. The OP was asking for a way to get all the values contained by all 'PLU' keys. I solved it this way but because I am a newbie in python I was wondering if it would be an optimal/better way of doing this, maybe using iterators, I don't know.

Result should be:

234
32
33
334
61

def getPLU(data):
    for item in data:
        if type(data[item]) == dict:
            getPLU(data[item])
        else :
            if item == 'PLU':
                print (data[item])

    pass

def Main():
    times = 0
    Menu = {
      'PLU' : '234',
      'Salad': {
        'salad': {
            'ceaser':{
                'PLU': '32'
            },
            'italian':{
                'PLU': '33'
            }
        }
      },
      'Dessert': {
        'cookie': {
          'PLU': '334',
          'NAME': 'cookie ',
        }
      },
      'Appetizer': {
        'extra sauce': {
          'PLU': '61',
          'NAME': 'extra sauce',
        }
      }
    }
    getPLU(data=Menu)
    #print (getPLU(Menu))



if __name__ == '__main__':
    Main()
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I hope you get some good answers. \$\endgroup\$ – Phrancis Aug 15 '18 at 20:45
  • \$\begingroup\$ Thanks for the welcome and thanks for all the answers posted. Also thanks for the corrections made to the question. \$\endgroup\$ – Nestor Aug 16 '18 at 16:39
6
\$\begingroup\$

Great job so far! I have a few suggestions for your code:

  1. Avoid writing functions with side effects such as printed output. Side effects make a function much less reusable. Instead, you may return a generator that yields the next entry. This gives the caller maximum control over what to do with the result set: print it, iterate one by one, just get the first few items without searching the entire structure, etc.

  2. Consider adhering more strictly to Python naming conventions. For example, functions should be lower_camel_cased. Since your function returns multiple PLUs, the function name seems more accurately written as get_plus. You can remove the pass keyword in your function and pay close attention to spacing, for example in the print (data[item]) function call and your else : block.

  3. Clean up lines like

    else :
        if item == 'PLU':
            print (data[item])
    

    Which is logically equivalent to:

    elif item == 'PLU':
        print(data[item])
    
  4. Use data.items() to iterate through your dictionary's keys and values rather than only keys. This allows cleaner syntax to access values.

  5. Make your function more general for maximum reusability. Fetching a list of values by key from a dictionary is a task that should work for any key. Why not make "PLU" a search parameter? If you want to keep the original get_plus version, write both functions and have get_plus wrap the generalized version.

  6. isinstance may be a more accurate choice than type if you wish to allow collection subclasses of dictionary to use your function.

Here's my version for consideration:

def find_by_key(data, target):
    for key, value in data.items():
        if isinstance(value, dict):
            yield from find_by_key(value, target)
        elif key == target:
            yield value


def main():
    menu = {
      'PLU' : '234',
      'Salad': {
        'salad': {
            'ceaser':{
                'PLU': '32'
            },
            'italian':{
                'PLU': '33'
            }
        }
      },
      'Dessert': {
        'cookie': {
          'PLU': '334',
          'NAME': 'cookie ',
        }
      },
      'Appetizer': {
        'extra sauce': {
          'PLU': '61',
          'NAME': 'extra sauce',
        }
      }
    }

    for x in find_by_key(menu, "PLU"):
        print(x)


if __name__ == '__main__':
    main()
\$\endgroup\$
5
\$\begingroup\$

Your code is well organized and read well but there are a few oddities:

  1. Read PEP8 and apply official coding conventions so that your code read like Python code;
  2. Remove that useless pass;
  3. Don't print your results in the function that compute them, instead return them to the caller so your function is reusable (in your case, yielding them might be more appropriate);
  4. Don't check for a specific type using type: favor isinstance; better would be to not check for a type at all but for a feature: call the items method of what would appear to be a dict and work with that, discard anything else that raise an AttributeError;
  5. A more generic function would accept the key to search for as parameter;
  6. A recursive approach cannot handle arbitrary large structures due to the recursion limit, an iterative approach can (even though it would rarely be an issue in practice).

Proposed improvements:

def retrieve_nested_value(mapping, key_of_interest):
    mappings = [mapping]
    while mappings:
        mapping = mappings.pop()
        try:
            items = mapping.items()
        except AttributeError:
            # we didn't store a mapping earlier on so just skip that value
            continue

        for key, value in items:
            if key == key_of_interest:
                yield value
            else:
                # type of the value will be checked in the next loop
                mappings.append(value)


def main():
    menu = {...}
    for plu in retrieve_nested_value(menu, 'PLU'):
        print(plu)


if __name__ == '__main__':
    main()
\$\endgroup\$
3
\$\begingroup\$

Nice recursive solution! Just a few suggestions:

  • You should structure you entire code better and set up individual functions to set up the menu and get the prices, not do it all in main.
  • Ideally, you would add a docstring that describes what your function does and what the parameters are. I personally like the numpy docstring style.
  • Function names should be snake_case, not CamelCase, so it would be get_plu(data)
  • data is a very generic term, you should use something more descriptive such as menu
  • If you want the type comparison to also work for inherited types, you should use isinstance(data[item], dict) instead of type(data[item]) == dict
  • Remove the space in print (data[item])
  • Your function should return something meaningful or None. Perhaps you want to also save the prices in a list or set, or you want to at least return 0, which commonly means that the function ran successfully.
\$\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.