CODEWITHSUNDEEP
pythonrefactoring
vaidab
vaidab

Reputation: 59

Refactoring a loop in Python

I know this is not good coding and I'm looking to improve it. I want to get the first name by default if no name is supplied. My hack still enters the for loop and if it would be a big one it would be innefficient. But if I do all the attributions that I'm doing in the for loop's inside if, again, in the outside if, I would duplicate code. And I wouldn't use a function just to set attributes so I'm not sure how to proceed here.

    if not name:
        name = self.data['big_list'][0]['name']

    for i in range(len(self.data['big_list'])):
        if self.data['big_list'][i]['name'] == name:
            self.name = name
            self.address = self.data['big_list'][i]['address']
            ...
            return

Upvotes: 0

Views: 244

Answers (2)

Nechoj
Nechoj

Reputation: 1582

Here are some comments (that might be insuficient because I do not really understand what you want to achieve with the program).

The purpose of the for loop seems to be to find an item of the list self.data['big_list'] that meets the condition self.data['big_list'][i]['name'] == name, get some data and then terminate. Each entry of self.data['big_list'] is a dict.

This is a good job for a technique called list comprehension, which is much faster than for-looping.

The expression

filtered = [item for item in self.data['big_list'][1:] if item['name'] == name]

results in a list of dicts that are not the first one and meet the name-condition. The used expression self.data['big_list'][1:] is all of self.data['big_list'] but the first one. This technique is called slicing of lists. I assume you are not interested in the first entry, because you got the name from this first entry and search for other entries with the same name (which your for-loop doesn't, btw).

There may be more than one entry in filtered, or, none. I assume you are only interested in the first match, because your program does return when a match happens. Therefore, the second part of the program would be

if len(filtered) > 0:
    first_match = filtered[0]
    self.name = name
    self.address = first_match['address']
    ...

This way the structure of the program is more clear and other readers can better understand what the program does. Furthermore, it is much faster ;-).

Upvotes: 1

Charles Dassonville
Charles Dassonville

Reputation: 207

if think there is a general bad practice or misunderstanding of class: there is a class right? and you check if your instance has the same name of one of the names in your list, and set itself.

-> in your instance it seems you have large data, containing a list of people AND attributes for one person. That does not sound correct.

Let say the class is called Person.

  • you can create an init() method, which takes one row of your data['big_list']. instead of setting each attribute in a loop.
  • you might also want to create a equals() method which checks if a person is the same as someone else. (check duplicates)
  • consider taking your large_data out of that class.

Could you provide us with a little more context?

Upvotes: 2

Related Questions