This code add functionality to a GUI designed using PyQt. It cannot run alone, but I can provide the code. I am not asking for someone to provide criticism for every single function written here, but one or two tips would make me a happy camper. I would like to know if there is something I am making more complicated than it ought to be, or if there is a better way to structure this code.

About the program:

This program makes XML files to be used by a game as a mod. The game uses these XML files to add things to make in said game. Making such an XML is not hard, but if one plans to add something worthwhile, it can get long and tedious. This program aims to make it easier for both the modding community and myself. The program will open XML files to edit or create new ones. It will allow the user to save changes (the user can change the file name or directory at any time). Since this code adds functionality to the GUI, you would be looking at functions that aim to do different tasks like adding things to comboboxes, deleting a child tag from a parent tag, or create new child tags.

from lxml import etree
from PyQt4 import QtGui
import sys
import design
import modBuilder as mb
import errorMessages as em
from PyQt4.QtCore import *
from PyQt4.QtGui import *
import os
from copy import deepcopy
from six import string_types

class ExampleApp(QtGui.QMainWindow, design.Ui_MainWindow):
    def __init__(self):
        self.confirm_button.clicked.connect(lambda: self.find_tree_tags())
        self.addOSDependencyBttn.clicked.connect(lambda:self.add_needed_dependency('Operating System', self.os_dict, str(self.OSDEPBOX.currentText())))
        self.addVisualDependencyBttn.clicked.connect(lambda:self.add_needed_dependency('Visual', self.visual_dict, str(self.VISUALDEPBOX.currentText)))
        self.addAudioDependencyBttn.clicked.connect(lambda:self.add_needed_dependency('Audio', self.audio_dict, str(self.AUDIODEPBOX.currentText())))

        self.OSCHECKBOX.stateChanged.connect(lambda: self.dependency_check_status(self.OSCHECKBOX,self.addOSDependencyBttn))

        self.current_feature_index = 0
        self.os_dict = {}
        self.audio_dict = {}
        self.visual_dict = {}
        self.custom_Dependency_Dict = {}
        mainMenu = self.define_menubar()
        fileMenu = self.add_menu_to_menubar(mainMenu, '&File')
        self.define_action(mainMenu,fileMenu,"&Save As","",self.save_as)
        self.define_action(mainMenu,fileMenu,"&New Mod","Ctrl+N",self.create_New_Mod)

    def define_menubar(self):
        return self.menuBar()       

    def add_menu_to_menubar(self,menu,name):
        return menu.addMenu(name)       

    def add_actions_to_menubar(self,file_Menu,action):

    def define_action(self,menu_bar,file_menu,action_name,shortcut,function):
        action = QtGui.QAction(action_name, self)
        if shortcut != '':

    def close(self):
        choice = QtGui.QMessageBox.question(self, 'Warning',"You are about to exit. All unsaved progress will be lost. Do you want to continue?",QtGui.QMessageBox.Yes | QtGui.QMessageBox.No)
        if choice == QtGui.QMessageBox.Yes:                                                             # Checks if choice is equal to the user response of yes

    def execute_save(self):
        self.save(self.tree)                                                                            # Executes the function save

    def open_file_handler(self):
        self.file_name = QtGui.QFileDialog.getOpenFileName(self, 'Open File')
        self.directory = os.path.dirname(self.file_name)

    def open_file(self):    
        """ This function is tied to a "Open File" Button and what it does is open the xml file the user chooses and parses it. It then enables some other buttons, loads information to text fields 
            add information from the xml file to comboboxes and basically allows the user to see what matters and what he might want to change or delete in this XML file that will ultimately be used 
            as a mod in the game 'Software Inc'

            with open(self.file_name) as f:                                                              
                self.tree = etree.parse(f)                                                              
            self.features = self.tree.find('Features')                                              
            list_of_things_to_enable = [self.addFeatureBttn,self.funcName,self.categories_check_box]


            self.statusBar().showMessage('File Opened',1500)    
            if self.file_name == '':

    def save(self, tree):
        """ This function saves the file to current directory."""
            with open(self.file_name, 'wb+') as f:                                                      # Opens filename as f with the intention to write
                tree.write(f, pretty_print=True)                                                        # Writes tree into filename
            self.statusBar().showMessage('Saved',1500)                                                  # Shows message 'Saved'
            if self.file_name == '':                                                                        # Checks if filename is equal to empty string

    def save_as(self,tree):
        """ This function is tied to save As option and so 
            it will ask for a file name and a new directory in case the user wishes to change where the file is saved"""

            self.file_name = QtGui.QFileDialog.getSaveFileName(self, 'Save As')
            self.directory = QtGui.QFileDialog.getExistingDirectory(self, 'Choose your directory')  
            filename = os.path.basename(os.path.normpath(self.file_name))

            with open(self.file_name,'wb') as f:
                tree.write(f, pretty_print=True)

            directory = open(self.directory,'w')
            self.statusBar().showMessage('Failed to Save',1500)

    def create_new_mod(self):
        """ This function is tied to a button "Create new mod" and what it does is run code from a module imported and creates an xml files with the nessecary fields to make a mod.
            This file is created with a name given by the user in a directory chosen by the user and is then opened using the open function shown above"""

        self.file_name = QtGui.QFileDialog.getSaveFileName(self, 'Choose a name for your mod')
        number_Of_Features = self.numberOfFeatures.value()                                              # Gets the value of self.numberOfFeatures and stores it in numberOfFeatures     
        mb.createMod(number_Of_Features,self.file_name)                                                 # Executes the createMod function imported from modBuilder
        directory = QtGui.QFileDialog.getExistingDirectory(self, 'Choose your directory')


        self.statusBar().showMessage('New Mod Created',1500)                                            # Shows message 'New Mod Created'

    def populate_software_type_fields(self):
        """ This function will populate the text fields in the GUI that this program uses. It looks for the expected tags in the mod, gets their text, and displays it on the correct field
            so the user can see what it is and change it if desired."""

        tags_to_look_for = ['Name','Description','Unlock','Population','Random','OSSpecific','InHouse','Category','Iterative','Retention','NameGenerator']
        tag_text_list = []
        for tag in tags_to_look_for:

        set_text_list = [self.name_line_edit,self.description_line_edit,self.unlock_line_edit,self.population_line_edit,self.random_line_edit,self.OSS_line_edit,self.house_line_edit,self.category_line_edit,self.iterative_line_edit,self.retention_line_edit,self.name_generator_line_edit]

        for index,line in enumerate(set_text_list):

    def categories_check_status(self):
        """ This function checks if the categories checkbox is selected and performs the required actions.  If it is selected, then it must create a subElement called Categories so that the user may add a Category element, 
            and it will enable the required fields. However, if it is not checked then it disables the fields and sets the class varaible self.categories_status variable to False"""

        list_of_lines = [self.category_name_line_edit,self.c_description_le,self.c_unlock_le,self.popularity_le,self.time_scale_le,self.add_category_button,self.add_category_button,self.remove_category_button]
        status = self.categories_check_box.isChecked()
        if status:
            self.categories_status = True
            root = self.tree.getroot()
            if self.tree.find("Categories") is None:
            self.categories_status = False
            self.enable_multiple_objects(list_of_lines, False)

    def add_categories_to_combobox(self): 
        """ This feature clears the category combobox to avoid adding the same category multiple times. It then goes through each category in categories(That is, if the tag Categories exists) and adds the text value 
            of Name for each category. In simple terms, it looks for each category, looks for the obligatory Name tag, and gets the text found in the Name tag to display in the combobox."""

        if self.tree.find("Categories") is not None:                                                    
            for category in self.tree.find("Categories"):   

    def delete_category(self,category_to_delete):
        """ Deletes the selected category from the file and from the category combobox"""

        category_key = self.get_category_index(tree,feature_to_delete)                                  

    def remove_category_from_combobox(self,category_to_delete):
        """Removes category from combobox. It is used by the delete_category function"""

        category_index = self.category_combobox.findText(category_to_delete)                            

    def get_category_index(self, name):  
        """ Is used to find the index of a category."""

        for idx, category in enumerate(self.tree.find("Categories")):
            if category.find('Name').text == name:
                return idx          

    def add_category(self,Categories):
        """ This function will add a Category to the tag Categories. It then creates the tags needed and adds the right text to each."""
        category_index = self.category_combobox.count()
        dict_of_fields = {'Name':self.category_name_line_edit,'Description':self.c_description_le,'Unlock':self.c_unlock_le,'Popularity':self.popularity_le,'TimeScale':self.time_scale_le,'Retention':self.c_retention_le,'Iterative':self.c_iterative_le,'NameGenerator':self.name_generator_le}

        for tag,field in dict_of_fields.items():
            etree.SubElement(Categories[category_index],tag).text = str(field.text())


    def populate_feature_fields(self,feature_name):
        """ This function displayes the information of a feature selected by the user. The user selects a feature on a combobox and clicks on a button called "Edit Feature."
            This function will find the index of the feature using the name displayed in the combobox, it then looks for the expected tags and displays the text values for each tag in the right text field.
            Finally, this function creates a class variable sor self.dependencies. This is so other functions can use a variable holding the Dependency tag in the feature selected without having to use the .find 
            etree method over and over again. The dependencies of the feature are also added to a combobox"""

        self.current_feature_index = self.get_feature_index(feature_name) 
        feature = self.features[self.current_feature_index]

        tags_to_look_for = ['Name','Description','Unlock','DevTime','Innovation','Usability','Stability','CodeArt','Server']
        list_of_fields = [self.descEdit,self.unlockEdit,self.devtimeEdit,self.innovationEdit,self.usabilityEdit,self.stabilityEdit,self.codeartEdit,self.serverEdit]
        tag_dict.items = dict(zip(tags_to_look_for[1:], list_of_fields))

        for line,text in tag_dict.items():

        self.dependencies = (self.features[self.current_feature_index].find("Dependencies"))                       

    def find_tree_tags(self): 
        """ This function will remove the Categories tag if self.categories_status is False and if it exists. 
            It then looks for the expected tags, and assigns the required text."""

        if  self.categories_status is False:
            if self.tree.find("Categories") is not None 

        tags_to_look_for = ['Name','Description','Unlock','Population','Random','OSSpecific','InHouse','Category','Iterative','Retention','NameGenerator']

        field_list = [self.name_line_edit,self.description_line_edit,self.unlock_line_edit,self.population_line_edit,self.random_line_edit,self.OSS_line_edit,self.house_line_edit,self.category_line_edit,self.iterative_line_edit,self.retention_line_edit,self.name_generator_line_edit]

        tag_text_dict = dict(zip(tags_to_look_for,field_list))

        for tag,field in tag_text_dict: 
            self.tree.find(tag).text = str(field.text())                                                            
        self.statusBar().showMessage('Changes made',1500)                                                   

    def add_feature(self,name,count):
        """ This function will add a feature. It copies an already existing feature and adds it to the Features tag in the tree."""
        functionExists = self.check_if_feature_exists(name)                                         # Executes the function checkIfFeatureExists and stores the returned value in the variable functionExists
        if name != '' and  not functionExists:                                                  
            new_feature = deepcopy(self.features[0])                                                # Makes a copy of the first feature and stores it in the variable new_feature
            for tag in new_feature:                                                                 # Goes through every tag in new_feature
                tag.text = ''                                                                       # Sets the text value to an empty string
            new_feature.find('Name').text = name                                                    # Finds the tag Name and sets its text value to the value of the variable name
            self.features.insert(count, new_feature)                                                # Finds the tag name in the feature that will be added and gives it a text value of name
            self.add_features_to_combobox()                                                                 # Inserts a new feature
            self.statusBar().showMessage('Feature Created',1500)                                    
        elif name == '':                                                                            # Shows message 'Feature Created'
            em.showNoFeatureNameError(self)                                                         # Shows error message for no feature name

    def delete_feature(self,feature_to_delete):
        """ This function deletes the selected feature from the features tag and from the feature combobox """
        featureKey = self.get_feature_index(tree,feature_to_delete)                                 
        feature_index = self.featureComboBox.findText(self.featureComboBox.currentText())                           

    def check_if_feature_exists(self,name):         
        """ This function will check if there is a feature in features with a certain name and return True if it exists or False if it does not."""                                             
        for feature in self.features:                                                                       
            if feature.find('Name').text == name:
                return True
        return False                

    def get_feature_index(self, name):                                                              
        """This function gets the index of a certain feature"""
        for idx, feature in enumerate(self.features):
            if feature.find('Name').text == name:
                return idx

    def apply_changes_to_feature(self): 
        """ This function will update the information for the feature that was edited. The feature is updated to reflect the changes that were made on the GUI. 
            This then ends in the clearing of some dictionaries to prevent adding things more than one the next time and displays a message to let the user know
            that the changes were saved."""
            feature = self.features[self.current_feature_index]                                         # Finds feature in features and stores it in the variable feature
            feature_information_dict = {feature.find("Description"):self.descEdit,(feature.find("Unlock")) :self.unlockEdit,feature.find("DevTime") :self.devtimeEdit,feature.find("Innovation"):self.innovationEdit,feature.find("Usability") :self.usabilityEdit,feature.find("Stability"):self.stabilityEdit,feature.find("CodeArt") :self.codeartEdit,feature.find("Server"):self.serverEdit,}

            for field,content in feature_information_dict.items():                                  
                field.text = str(content.text())        

            depBoxList = [self.OSDEPBOX, self.VISUALDEPBOX, self.AUDIODEPBOX]                           

            if self.fromStatus:
                feature.attrib['From'] = str(self.fromLE.text())
            self.statusBar().showMessage('Saved changes to feature',1500)                           # Shows 'Saved changes to feature' message

    def add_features_to_combobox(self): 
        """  This function clears the feature combobox to avoid duplicates and goes through each feature in features and adds the text value for the name tag in each feature."""

        for feature in self.features:   
        list_of_things_to_enable = [self.featureComboBox,self.editFeatureBttn,self.dependencyComboBox,self.deleteDependencyBttn]                        

    def dependency_managing(self):
        """ This function manages dependencies. It creates a dependencies tag if it does not exist yet and then stores it in self.dependencies for easy access. After that it 
            will add the necessary dependencies to the selected feature, add custom dependencies, and adds dependencies to the dependencies combobox. """

        if self.dependencies is None:                                                                   
            self.dependencies = (self.features[self.current_feature_index].find("Dependencies"))                

    def get_dependency_index(self,name):                                                    
        """ This function gets the index of the dependency selected"""
        for idx, dependency in enumerate(self.dependencies):                                                            
            if dependency.text == name:                                                             
                return idx 

    def add_dependencies_to_combobox(self):
        """ This function clears the dependency combobox and then adds the text value for the name tag in each dependency in dependencies"""
            for index,dependency in enumerate(self.dependencies):
                self.dependencyComboBox.addItem(self.dependencies[index].text )                                 

    def add_Dependencies(self):
        """ This function adds the dependencies in a feature using dictionaries. Everytime a feature is loaded, its dependencies are added to dictionaries. There are three for each type of software
         that the dependencies can be categorized as.
        dependency_dict_list = [self.os_dict,self.visual_dict,self.audio_dict]  

        for dependency_dict in dependency_dict_list:                                                   #Goes through each dictionary in the dependency_dict_list
            for feature,Software in dependency_dict.items():                                            # The key is the feature and the value is the software.
                dependency_in_feature = self.check_if_dependency_is_in_feature(feature)                  # Checks if the dependency is already in the feature
                if dependency_in_feature:                                                                 # If it is then it skips it
                    etree.SubElement(self.dependencies,"Dependency",Software = Software).text = feature    # If it is not, then it adds the dependency
        self.add_dependencies_to_combobox()                                                              # Updates the dependency combobox

    def check_if_dependency_is_in_feature(self,feature):
        """ Checks if the dependency is in the current feature."""
        for dependency in self.dependencies:
            if dependency.text == feature:
                return True
        return False

    def add_custom_dependency(self,software,feature):
        """ adds a custom depdencency. Note: it executes the check_for_feature function to check if it already exists and takes the desired action"""
        if software != '' and feature != '':                                                        # Checks if key and value are both not equal to ''. They cannot be empty strings for this to work.
            em.showNoSoftwareErrorOrNoFeatureError(self)                                            # Shows message to let the user know he/she did not give a name for the Software or feature

    def check_for_feature(self,feature,software,custom_dependency_dict):
        """ This function checks if the feature the dependency depends on is part of the custom dependency dictionary which is used when adding custom dependencies"""
        if feature not in custom_dependency_dict[software]:
            list_of_features = custom_dependency_dict[software]
            is_string = isinstance(list_of_features,string_types)   
            if is_string == True:
                list_of_features = list(list_of_features)
            custom_dependency_dict[software] = list_of_features 
            self.statusBar().showMessage('Cannot add same dependency twice.',1500)                  # Shows message 'Cannot add same dependency twice.' 

    def add_custom_dependencies(self):
        """ This goes through the custom_Dependency_Dict and adds dependencies to the current feature"""

        for software,features in self.custom_Dependency_Dict.items():
            is_string = isinstance(features,string_types)   
            if is_string:
                etree.SubElement(self.dependencies,"Dependency", Software = software).text = features 
                for  feature in features:
                    etree.SubElement(self.dependencies,"Dependency", Software = software).text = feature 

    def check_for_attributes(self,feature):
        """ This function checks for attributes. Runc the first three functions to check the status of the vital,from, and forced checkboxes since they user might have not clicked on those 
            checkboxes and thus failed tu have runned these functions that create the required class variables self.Vital_Status, self.Forced_Status, and self.From_Status. Dependending on 
            those statuses it will give an attribute to the feature or not."""
        attribute_Dict = {'Vital':self.Vital_Status,'Forced':self.Forced_Status,'From':self.From_Status}
        for attribute,status in attribute_Dict.items():
            if status:
                feature.attrib[attribute] = 'TRUE'

    def enable_multiple_objects(list_of_things_to_enable,state):
        """ This function just iterates over a lists and enables or disables the fields in that list. Made it a function since it is used a few times."""
        for object_to_enable in list_of_things_to_enable:

    def add_needed_dependency(self,dependency_type,dependency_dict,dependency_selected):  

    def delete_dependency(self,number_Of_Dependencies,dependency_name):
        """ This function deletes a dependency from dependencies and from the dependency combobox."""
            index = self.get_dependency_index(dependency_name)    

            if self.dependencyComboBox.count() == 0:                                                    
                self.statusBar().showMessage('There are no dependencies to delete',1500)                

    def dependency_check_status(self,checkbox,button):       
        """ This function enables a button is a checkbox is checked. Triggered by the statechange of some checkboxes."""                                                             
                if checkbox.isChecked:

    def check_forced_checkbox_status(self):
        """ Checks the status of the forced checkbox and then enables or disables the other two checkboxes dependending on the the value of forced status"""
        ForcedStatus = self.FORCEDCHECKBOX.isChecked()                                                  # Assigns the value of self.FORCEDCHECKBOX.isChecked() to ForcedStatus 
        self.FROMCHECKBOX.setEnabled(not ForcedStatus)                                                  # Takes into account whether the FORCEDCHECKBOX is checked to enable or disable the FROMCHECKBOX    
        self.VITALCHECKBOX.setEnabled(not ForcedStatus)                                                 # Takes into account whether the FORCEDCHECKBOX is checked to enable or disable the VITALCHECKBOX
        return self.Forced_Status

    def check_vital_checkbox_status(self):
        """ Checks the status of the vital checkbox and then enables or disables the FORCEDCHECKBOX dependending on the the value of vital status"""
        VitalStatus = self.VITALCHECKBOX.isChecked()                                                    # Assigns the value of self.VITALCHECKBOX.isChecked() to ForcedStatus 
        self.FORCEDCHECKBOX.setEnabled(not VitalStatus)                                                 # Takes into account whether the VITALCHECKBOX is checked to enable or disable the FORCEDCHECKBOX
        return self.Vital_Status

    def check_from_checkbox_status(self):       
        """ Checks the status of the from checkbox and then enables or disables the FORCEDCHECKBOX dependending on the the value of from status"""          
        FromStatus = self.FROMCHECKBOX.isChecked()                                                      # Assigns the value of self.FROMCHECKBOX.isChecked() to ForcedStatus 
        self.FORCEDCHECKBOX.setEnabled(not FromStatus)                                                  # Takes into account whether the FROMCHECKBOX is checked to enable or disable the FORCEDCHECKBOX                                                                
        return self.From_Status

    def clear_fields(self):
        clearSet = {self.descEdit, self.unlockEdit, self.devtimeEdit, self.innovationEdit, self.usabilityEdit, self.codeartEdit, self.serverEdit, self.stabilityEdit,self.os_dict,self.visual_dict,self.audio_dict,self.custom_Dependency_Dict]}
        for item in clearSet:

def main():
    app = QtGui.QApplication(sys.argv)
    global form
    form = ExampleApp()
if __name__ == '__main__':

Your code lines run on long, especially with inline comments and docstrings. You should read the official style guide PEP0008, it has tons of useful style and readability notes.

You test strings with != '' multiple times, but Python's truthiness means this is unnecessary. Python will evaluate empty string as False, while strings containing any nonzero number of characters are True. So if string != '': is the same as if string:.

You also have one case of calling if condition: pass; else: code. This is unnecessary when you can just call if not condition: code and remove the need for any else block in the first place. This may come from a misunderstanding of pass, but at it's most basic it does nothing. It's only necessary when you're supposed to include a block of code but have no need of one. For example, if you want to ignore an error and do nothing at all:

except ValueError:

In this case, it's a SyntaxError to have no code after an except call, so you need to at least include pass to fulful Python's syntax requirements. But if you're doing anything at all, the pass becomes redundant, eg:

except ValueError:
    print("An error occurred")

pass is now redundant here, since without it no SyntaxError is raised anyway.

In check_if_feature_exists you could use any to simplify your code a lot. any is a function that runs a test on each element of a collection to see if it evaluates as True or False. It's faster and can be more readable than a for loop. In your case, it could be rewritten like this:

def check_if_feature_exists(self,name):         
    """ This function will check if there is a feature in features with a certain name and return True if it exists or False if it does not."""

    if any(feature.find('Name').text == name for feature in self.features):
        return True
    return False                

Now it's condensed into a single line, and still reads as clearly as the for loop (clearer in my opinion, as it's closer to english). The other advantage is that you no longer need the if part, you can just return the result of any:

    return any(feature.find('Name').text == name 
               for feature in self.features)
  • \$\begingroup\$ Thanks! This was very useful. Did not know I could use any. Will give PEP 0008 a look. \$\endgroup\$ – Oscar Reyes Jan 25 '16 at 18:44

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.