Refactoring a loop in Python

Question

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

this whole data structure looks awful. what are you trying to achieve? why do you operate on Dict[List[Dict] ? what if you simply try IF NOT NAME ... ELSE doForLoopFunction — Dariusz Krynicki
– Dariusz Krynicki, Commented Dec 5, 2021 at 18:47
aside: in python3 you can loop the list with for i in self.data['big_list']: — chickity china chinese chicken
– chickity china chinese chicken, Commented Dec 5, 2021 at 18:48
If you get the first name as a default, then you will only do one iteration of the for loop, so it won't be that inefficient. — Luke Nelson
– Luke Nelson, Commented Dec 5, 2021 at 18:50
Further, from the amount of code you've shown us, my immediate instinct is that the way to improve it would be to have data that is organized in a better way, but without knowing your data model and use case, there's not much help to provide. — Luke Nelson
– Luke Nelson, Commented Dec 5, 2021 at 18:52
It's a json file external to my program. Something like this: github.com/ethereum-lists/chains/blob/master/_data/chains/… — vaidab
– vaidab, Commented Dec 5, 2021 at 20:05

Charles Dassonville · Accepted Answer · 2021-12-05 19:02:56Z

2

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?

edited Dec 5, 2021 at 19:02

answered Dec 5, 2021 at 18:57

Charles Dassonville

2071 silver badge5 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

vaidab Over a year ago

Ignore the class part please, I'm checking in an imported json file for a name and use save the data relateed to that name in the class (name, address, etc). The json is not mine, it's a standard one and I can't modify. I'd rather simplify the code than add context, I think it'll get confusing rather than simpler. Please ignore the class part, if it would be a simple function, it's very innefficient to do this:

Nechoj · Accepted Answer · 2021-12-05 20:59:59Z

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 ;-).

Collectives™ on Stack Overflow

Refactoring a loop in Python

2 Answers 2

1 Comment

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Related