0

Trying not to use too many variables in code, I came up with the code below. It looks horrible. Any ideas on how to format it nicely? Do I need to use more variables?

I write code like this a lot, and it'd help to see what methods people usually resort to have readable code while making creating less variables

exceptions = []

# find all the distinct parent exceptions (sorted) and add to the list
#   with their children list
for parent in collection.find(
    {'tags': 'exception'}).sort('viewPriority').distinct('parentException'):
    group_info = {'groupName': parent,
                  'children': [{'value': ex['value'],
                                'label': ex['label'],}
        for ex in collection.find({'tags': 'exception',
                                   'parentException': parent}
                                 ).sort('viewPriority')],
                 }
    exceptions.append(group_info)
xcorat
  • 1,434
  • 2
  • 17
  • 34
  • 1
    Yes create more variables. Why don't you think it's a good idea? The most important thing is usually readbility – John La Rooy Oct 08 '13 at 23:16
  • It feels weird to create a variable to be only used once, no? – xcorat Oct 08 '13 at 23:17
  • 1
    Sure sometimes it does at first, especially if you are used to a language that needs variables to be declared etc. In Python you are just creating an extra reference. It gets cleaned up later automatically or you can `del` it if you need to (perhaps it's holding on to a lot of memory) – John La Rooy Oct 08 '13 at 23:20
  • Also, besides being unreadable… does this code actually work? In Python, it's idiomatic for methods that do things like `sort` to work in-place and return `None`; something that makes and returns a copy should be called `sorted`, and something that mutates and returns `self` shouldn't exist in the first place. Python is intentionally not designed to allow this kind of "fluent" method chaining, and if you fight against it, you're going to have a hard time writing clean code. – abarnert Oct 08 '13 at 23:54
  • @abarnert, I assumed it was something like an ORM type query – John La Rooy Oct 09 '13 at 00:14
  • its `pymongo`. MongoDB works in javascript, so the direct translation of methods i guess makes them return objects. (It actually doesn't work, and I think it's should, given that sorted returns a cursor, but that's a different reason). [cursor](http://api.mongodb.org/python/current/api/pymongo/cursor.html) – xcorat Oct 09 '13 at 05:17

2 Answers2

2

I would break your logic up into functions

def get_children(parent):
    result = collection.find({'tags': 'exception', 'parentException': parent})
    result = result.sort('viewPriority')
    return [{'value': ex['value'], 'label': ex['label']} for ex in result]

def get_group_info(parent):     
    return {'groupName': parent, 'children': get_children(parent)}

result = collection.find({'tags': 'exception'})
result = result.sort('viewPriority').distinct('parentException')

exceptions = [get_group_info(parent) for parent in result]

As a bonus, you can easily unittest get_children and get_group_info

John La Rooy
  • 295,403
  • 53
  • 369
  • 502
  • Agreed 100%. Breaking it up into functions is usually even better than breaking it up into expressions stashed in intermediate variables. They're usually easier to name, they can be reused, they can be unit tested independently, they make debugging easier (no need to put a breakpoint or `print` in the middle of a huge function), … – abarnert Oct 08 '13 at 23:52
  • +1 for `result = ...` & `result = result.sort(..` That's actually neat. – xcorat Oct 11 '13 at 18:29
0

Definitely difficult to get this to look any good, here is my best attempt at keeping the line lengths short and maintaining readability:

exceptions = []

# find all the distinct parent exceptions (sorted) and add to the list
#   with their children list
for parent in (collection.find({'tags': 'exception'})
               .sort('viewPriority').distinct('parentException')):
    group_info = {
        'groupName': parent,
        'children': [{'value': ex['value'], 'label': ex['label'],}
                     for ex in (collection.find({'tags': 'exception',
                                                 'parentException': parent})
                                .sort('viewPriority'))],
    }
    exceptions.append(group_info)
Andrew Clark
  • 202,379
  • 35
  • 273
  • 306