0

I am trying to compare a list of forbidden folders read from a file. But we have to check if the task has the parent ID of the folder and then if that folder matches the forbidden folder. The lists I am looping through can contain many items.

for task in tasks:
    #Check the task is on their timesheet
    if task["id"] == entry["item_id"]:
        for folder in folders:
            #See if the task is inside the folder
            if task["parent_id"] == folder["id"]:
                for forbiddenFolder in forbiddenFolders:
                    #If the folder is on the list
                    if forbiddenFolder == folder["name"]:
                        body_of_email +=( "Using a task within a folder that is not permiited " + forbiddenFolder + "\r\n" )
                        folder["name"]
                        break

This code uses three nested for loops, which might be slow. Can I make this more efficient?

Li-aung Yip
  • 12,320
  • 5
  • 34
  • 49
JP29
  • 633
  • 4
  • 13
  • 25
  • 1
    Nested loops are really normally only a problem when you are iterating over tons of stuff - in this case you are searching for a path down, essentially, so it isn't horribly inefficient as in most cases you fail out early. – Gareth Latty Jun 15 '12 at 22:57
  • Welcome to SO! I trimmed your question down a little - there's no need to say "thanks in advance" at the end of each question. – Li-aung Yip Jun 15 '12 at 23:26

4 Answers4

3

You can reduce the number of lines of code and make it easier to understand like this:

tasks_id = [task in tasks if task["id"] == entry["item_id"]]
folders_dict = dict()
for folder in folders:
    folders_dict[folder["id"]] = folder

for task in tasks_id:
    if task["parent_id"] in folders_dict.keys() and folder_dict[task["parent_id"]] in forbiddenFolders:
        body_of_email +=( "Using a task within a folder that is not permiited " + forbiddenFolder + "\r\n" )
Ansari
  • 8,168
  • 2
  • 23
  • 34
2

What you're trying to do here is look up an item (task, folder) based on the id. Python's dictionaries provide an easy way to do this. You will only save if you'll be doing the searches several times (e.g. if there are many tasks with the same id, or if you'll be running the function several times).

Additionally, for forbiddenFolders you just have a list of names (you're not looking up an item, you're just checking if it's present) for which Python's sets are suitable.

Anyway, here is how you build the dictionaries and sets:

tasks_dict = dict((task['id'], task) for task in tasks)
folders_dict = dict((folder['id'], folder) for folder in folders)
forbidden_folders_set = set(forbiddenFolders)

Now, task = tasks_dict[id] is a task such that task['id'] == id, and similarly for folders, so you can replace the loops above with these expressions. The set doesn't allow this, but it allows you to check for presence with folder in forbidden_folders_set.

(Bear in mind that each of those dict(...) operations may take longer than running through one of the for loops above, but they are an investment for faster lookup in future.)

if entry['item_id'] in tasks_dict:
    task = tasks_dict[entry['item_id']]
    if task['parent_id'] in folders_dict:
        folder = folders_dict[task['parent_id']]
        if folder in forbidden_folders_set:
            body_of_email += ...

The x in y and ..._dict[x] operations above are very efficient.

Rodrigo Queiro
  • 1,324
  • 8
  • 15
1

A method which lets you keep the original datastructures is the following:

for task in (t for t in tasks if entry["item_id"]==t["id"]):
    for folder in (f for f in folders if task["parent_id"]==f["id"]):
        for forbiddenFolder in (ff for ff in forbiddenFolders if ff==folder["name"]):
            body_of_email += "Using a task within a folder that is not permitted %s \r\n"% forbiddenFolder
            break

This makes use of generators, which are very memory-efficient and thus preserve speed, and conditional for-loops of the form x for x in range(y) if condition. I recommend taking a look at both.

Community
  • 1
  • 1
0

taking a step back from the intricate details in the original inquiry, and put attention on just some "Alternatives to nested for loops" in general, how about the following:

tasks = {
    'design': 'later',
    'testing': 'desired',
    'vacation': 'fall'
}

folders = {
    'summer': 'red',
    'fall': 'gold',
    'winter': 'white',
    'spring': 'green'
}

def apply_some_logic(a, b, c, d):
    return '_'.join([a, b, c, d]) if b == c else (a, b, c, d)


if __name__ == '__main__':
    results = (apply_some_logic(a, b, c, d) for a, b in tasks.items() for c, d in folders.items())
    for result in results:
        print(result)


$ python3.6 --version
Python 3.6.4

output:
('design', 'later', 'Summer', 'Red')
('design', 'later', 'Fall', 'Gold')
('design', 'later', 'Winter', 'White')
('design', 'later', 'Spring', 'Green')
('implementation', 'urgent', 'Summer', 'Red')
('implementation', 'urgent', 'Fall', 'Gold')
('implementation', 'urgent', 'Winter', 'White')
('implementation', 'urgent', 'Spring', 'Green')
('testing', 'desired', 'Summer', 'Red')
('testing', 'desired', 'Fall', 'Gold')
('testing', 'desired', 'Winter', 'White')
('testing', 'desired', 'Spring', 'Green')
('vacation', 'Fall', 'Summer', 'Red')
vacation_Fall_Fall_Gold
('vacation', 'Fall', 'Winter', 'White')
('vacation', 'Fall', 'Spring', 'Green')
Down the Stream
  • 639
  • 7
  • 10