CODEWITHSUNDEEP
pythonloopsconditional-statements
Joshua Strot
Joshua Strot

Reputation: 2531

Is there an alternative to this code that looks neater and is more comprehensible?

I have this code which loops through a set, and checks to see if one of the items in the set is a folder, and if it is, it checks to see which folder it is and then proceeds to perform an action based on what folder it is. I'm not really sure how to explain why there are two loops, so I'm hoping you guys can look at it and understand why I did that, because it doesn't work without it.

You can see why I'm wondering if it can be cleaned up...

In this case, the value of neededdirs is

set(['Pictures', 'Downloads', 'Public', 'Desktop'])

and here's the main code which needs to be cleaned up.

neededdirs = folders.findKeyDir('active') #declares the set
for x in neededdirs: #starts the main loop
    for y in neededdirs: #starts the second loop
        if folders.getObject(neededdirs, y, 'bool'): #checks to see if the the option in the set is a folder.
            neededname = folders.getObject(neededdirs, y, 'name') #retrieves the name of the item in the set.
            if neededname == "Desktop": #this and all elif's after just check its name.
                self.folderheader1.setText(_translate("MainWindow", "Status: Active", None)) #this, the line after, and all others like it just change the text on an item if it evaluates to true.
                self.folderactive.setChecked(True)
            elif neededname == "Documents":
                self.folderheader2.setText(_translate("MainWindow", "Status: Active", None))
                self.folderactive_2.setChecked(True)
            elif neededname == "Downloads":
                self.folderheader3.setText(_translate("MainWindow", "Status: Active", None))
                self.folderactive_3.setChecked(True)
            elif neededname == "Music":
                self.folderheader4.setText(_translate("MainWindow", "Status: Active", None))
                self.folderactive_4.setChecked(True)
            elif neededname == "Pictures":
                self.folderheader1_2.setText(_translate("MainWindow", "Status: Active", None))
                self.folderactive_5.setChecked(True)
            elif neededname == "Public":
                self.folderheader1_3.setText(_translate("MainWindow", "Status: Active", None))
                self.folderactive_6.setChecked(True)
            elif neededname == "Templates":
                self.folderheader1_4.setText(_translate("MainWindow", "Status: Active", None))
                self.folderactive_7.setChecked(True)
            elif neededname == "Videos":
                self.folderheader1_5.setText(_translate("MainWindow", "Status: Active", None))
                self.folderactive_8.setChecked(True)

Thanks in advance.

Upvotes: 3

Views: 111

Answers (2)

PaulMcG
PaulMcG

Reputation: 63747

DRY = "Don't Repeat Yourself". When you find yourself writing code by copy/paste, stop and think how what you are doing could be parameterized, and either extracted out as a function of its own, or supported with some kind of data structure, or both. While this code isn't much shorter than what you wrote, it is easier to follow what is happening in the body of the loop, and adding new folders and associated form elements will be a snap. (Note that I violated PEP-8 here by using spaces to align the values in the FolderComponent tuples - in this case, I think it is of merit, as it helps highlight any variations from your naming scheme that might indicate a typo or other bug, and will help you maintain it as you make changes in the future.)

from itertools import product
from collections import namedtuple

neededdirs = folders.findKeyDir('active') #declares the set

# define name->component table; using a namedtuple for these
# so you can access different tuple fields by meaningful name 
# instead of by numeric index
FolderComponent = namedtuple('FolderComponent', 'name textbox checkbox')
components = [
    FolderComponent('Desktop',   self.folderheader1, self.folderactive),
    FolderComponent('Documents', self.folderheader2, self.folderactive_2),
    FolderComponent('Downloads', self.folderheader3, self.folderactive_3),
    FolderComponent('Music',     self.folderheader4, self.folderactive_4),
    FolderComponent('Pictures',  self.folderheader5, self.folderactive_5),
    FolderComponent('Public',    self.folderheader6, self.folderactive_6),
    FolderComponent('Templates', self.folderheader7, self.folderactive_7),
    FolderComponent('Videos',    self.folderheader8, self.folderactive_8),
    ]

#start the main loop
for x,y in product(neededdirs, repeat=2):

    #check to see if the the option in the set is a folder.
    if folders.getObject(neededdirs, y, 'bool'): 

        #retrieve the name of the item in the set.
        neededname = folders.getObject(neededdirs, y, 'name')

        # set form fields
        for comp in components:
            if neededname == comp.name:
                comp.textbox.setText(_translate("MainWindow", "Status: Active", None)
                comp.checkbox.setChecked(True)
                break

Upvotes: 0

Frerich Raabe
Frerich Raabe

Reputation: 94429

All your if/elif branches differ only in the object on which setText and setChecked is called. You could factor that out into something like:

knownFolders = {
    "Desktop": (self.folderheader1, self.folderactive),
    "Documents": (self.folderheader2, self.folderactive_2),
    ...
}

You could then replace your if/elif chain with:

if neededname in knownFolders:
    header, checkBox = knownFolders[neededname]
    header.setText(_translate("MainWindow", "Status: Active", None))
    checkBox.setChecked(True)

Also, judging by the comments in the code, I think you could replace the two nested loops with a single list comprehension like:

relevantFolders = [folders.getObject(neededdirs, d, 'name')
                   for d in neededdirs
                   if folders.getObject(neededdirs, d, 'bool')]

Upvotes: 4

Related Questions