2
\$\begingroup\$

I have this function which parses Beautiful Soup elements:

def get_info():
    holder = []
    problem_dict = {}
    selection_container = []
    for moselect in base_page.find_all('div', attrs={'class': 'Moselect'}):
        if moselect.find('div', attrs={'id': 'noSize'}):
            holder.append(moselect.find_all('div')[1:])

    for held_item in holder[0]:
        if held_item.find_all('option')[1:]:
            for m in held_item.find_all('option')[1:]:

                append_dict(selection_container, m)

                if len(m.text.split(' ')) > 1:
                    stock_info = {
                        'message': reduce(lambda x, y: x + ' ' + y, m.text.split(' ')[1:])[1:-1],
                        'status': 'pre-order'
                    }
                    problem_dict.update({'stock': stock_info})

        else:
            for m in held_item.find_all('option'):

                append_dict(selection_container, m)

                if len(m.text.split(' ')) > 1:
                    stock_info = {
                        'message': reduce(lambda x, y: x + ' ' + y, m.text.split(' ')[1:])[1:-1],
                        'status': 'pre-order'
                    }
                    problem_dict.update({'stock': stock_info})

    problem_dict.update(dict(selections=selection_container))
    return problem_dict

It will return dict with:

problem_dict = {'selection': [{'key': 'value'},{'key': 'value'},]}

and sometimes it returns:

problem_dict = {'selection': [{'key': 'value'},{'key': 'value'}], 'stock': {'key': 'value', 'key': 'value'} }

How can I refactor and optimize this method?

\$\endgroup\$
  • \$\begingroup\$ Could you provide some more information about what this function is supposed to do? Possibly give a some example input and what you expect to be output? \$\endgroup\$ – BeetDemGuise Jul 10 '14 at 11:52
  • \$\begingroup\$ @BeetDemGuise It will return dict with problem_dict = {'selection': [{'key': 'value'},{'key': 'value'},]} and some times it returns {'selection': [{'key': 'value'},{'key': 'value'}], 'stock': {'key': 'value', 'key': 'value'} } \$\endgroup\$ – user2734570 Jul 10 '14 at 12:08
4
\$\begingroup\$

There are a couple things in this method that can be refactored to remove a lot of duplicate code:

  1. Firstly, you have this if statement:

    if held_item.find_all('option')[1:]:
        ...
    else:
        ...
    

    What this is really checking for is if there is more than 1 option found. So, instead of having to write nearly identical code to handle the cases of 0 options or at least 1 option, we can simply store the find_all result and modify it based on a quick length check:

    options = held_item.find_all('option')
    if len(options) > 1:
        options.pop(0)
    
    for option in options:
        append_dict(selection_container, option)
    
        # If there is a non-zero amount of spaces, there will be at least
        # 2 partitions when split.
        if option.text.count(' '):
            stock_info = {'message': reduce(lambda x, y: x + ' ' + y, option.text.split(' ')[1:])[1:-1],
                          'status': 'pre-order'}
        problem_dict.update({'stock': stock_info})
    
  2. Now, take a look at this line of code:

    stock_info = {
        'message': reduce(lambda x, y: x + ' ' + y, m.text.split(' ')[1:])[1:-1],
        'status': 'pre-order'
    }
    

    I have two points to make about this line of code:

    • As a quick disclaimer about this first point:

      The line is still compliant with PEP8, so this is just a personal preference.

      With that out of the way, I try to never to put an opening {/(/[ at the end of a line unless I have to. Also, I never put the closing }/)/] on a line by itself. So I would format the line like this:

      stock_info = {'message': reduce(lambda x, y: x + ' ' + y, m.text.split(' ')[1:])[1:-1],
                    'status': 'pre-order'}
      
    • My second point about this line of code is about how you create message. They way you use reduce is not quite necessary. From what I can tell, you want to take everything after the first space, then everything of that section except the first and last characters. This can be easily done using the split function:

      stock_info = {'message': m.split(' ', 1)[-1][1:-1] ... 
      

      Or you can use list splicing:

      # Below is reduced version of:
      #    m[m.index(' ') + 1:][1:-1]
      stock_info = {'message': m[m.index(' ') + 2:-1] ...
      

Past these two points, there isn't much to improve the algorithm from what information you gave. If you wanted to reduce the number of lines, you could write the first for loop as a list comprehension:

holder = [moselect.find_all('div')[1:]
          for moselect in base_page.find_all('div', attrs={'class': 'Moselect'})
          if moselect.find('div', attrs={'id': 'noSize'})]

Style-wise, you could improve a few of your variable names (m, holder, and selection_container specifically) as some of them are either ambiguous or slightly misleading.


Here is your code with my suggestions implemented:

def get_info():
    problem_dict = {}
    selection_container = []
    holder = [moselect.find_all('div')[1:]
              for moselect in base_page.find_all('div', attrs={'class': 'Moselect'})
              if moselect.find('div', attrs={'id': 'noSize'})]

    for held_item in holder[0]:
        options = held_item.find_all('option')
        if len(options) > 1:
            options.pop(0)

        for option in options:
            append_dict(selection_container, option)

            # If there is a non-zero amount of spaces, there will be at least
            # 2 partitions when split.
            if option.text.count(' '):
                stock_info = {'message': {'message': m[m.index(' ') + 2:-1],
                              'status': 'pre-order'}
            problem_dict.update({'stock': stock_info})

problem_dict.update(dict(selections=selection_container))
return problem_dict
\$\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.