Easing mod-making for a game

This program is meant to ease making mods for a game. A mod is made with XML. I used the lxml library to handle making the XML. I used PyQt for the GUI and this code is the functionality of the GUI. Most functions do just what their name says: open file opens a file, save saves, addFunction makes sure that button that says add function actually adds a function.

This code is long and so as much as help to improve one function would be awesome. It works, but I know this code can be improved I just need feedback. I am no expert yet, so anything would be awesome feedback.

It needs a GUI .py file to be run. It is long and takes too much space, but if it is a must then I guess I can include it anyways.

This program basically helps make XML files with certain tags for a game. The game uses XML files to add features created by the users. Making these XML files is extremely easy but tedious and so I decided to make this program to achieve just that. It works so far as it is expected, but it has became very large, and I fear I am making it more complicated than it has to be and that it could be less complex.

```
from lxml import etree
from PyQt4 import QtGui
import sys
import design
import dialog
import modBuilder as mb
import modtoolbar as mtb
import errorMessages as em
from PyQt4.QtCore import *
from PyQt4.QtGui import *
from os import path
from copy import deepcopy
import os
try:
from collections import OrderedDict
except ImportError:
OrderedDict = dict
import time

class ExampleApp(QtGui.QMainWindow, design.Ui_MainWindow):
def __init__(self):
super().__init__()
QtGui.QApplication.setStyle(QtGui.QStyleFactory.create('plastique'))
self.setupUi(self)
self.openBttn.clicked.connect(lambda:self.openFileHandler())
self.saveBttn.clicked.connect(lambda:self.save(self.tree))
self.newMBttn.clicked.connect(lambda:self.createNewMod())
self.confBttn.clicked.connect(lambda: self.setNewSoftwareTypeValues())

The biggest problem with this code is that I have no idea what it does.

Complicated code is not inherently bad – sometimes we’re just solving thorny problems, and we need an in-depth solution. But it’s very hard for me to work out what this code is doing, and how it’s supposed to work.

The only comments in the program are just telling me what the code does – they should explain why it behaves that way. Docstrings should tell me what functions do, and how to use them. There should be a module doctoring explaining how to use the file. And so on.

Good documentation serves to mitigate complication. If there are comments to explain why it’s been written this way, and to guide me through, it becomes much easier to follow.

It’s hard to say whether the code could be simpler without knowing what it’s supposed to do.

And to add a bit of substance to those bones, here are some specific corrections I can suggest without knowing what the code does.

-
Read PEP 8, particularly the parts about spacing. There are parts where text has been rammed together (newlines between methods, the checkboxDict on line 464, arguments to functions) that just make it harder to read.

If you run something like flake8 over your code, it will help identify these style/readability issues. It can also help to identify typos (for example – look carefully at addOSDependency() and work out why it will always throw a NameError).

-
There are a lot of checks in your program of the form:

if variable == True:
    do_thing()

It’s cleaner and more Pythonic to drop the == True part and just write:

if variable:
    do_thing()

-
Towards the end of the script, I see blocks of code like this:

if ForcedStatus == True:                                                                        # Checks if ForcedStatus is True
    self.fromStatus = True                                                                      # Sets fromStatus to True
    self.FORCEDCHECKBOX.setEnabled(False)                                                       # Disables FORCEDCHECKBOX
else:
    self.fromStatus = False                                                                     # Sets fromStatus to False
    self.FORCEDCHECKBOX.setEnabled(True)

The repetition isn’t great, and we can simplify the code to get rid of it:

self.fromStatus = ForcedStatus
self.FORCEDCHECKBOX.setEnabled(not ForcedStatus)

-
Make sure your functions always return a meaningful result. For example, you have a function checkIfFeatureExists, which returns True if it finds a feature with the given name. It returns None if it doesn’t – it would be nicer to return False.

Speaking of that function, you can simplify the loop it uses like so:

def check_if_feature_exists(self, features, name):   
    for feature in features:
        feature_name = feature.find('Name')      
        if feature_name.text == name:
            return True
    return False

Notice that I’ve also been able to drop two arguments from the method signature that weren’t being used.

-
Likewise, you can tidy up getFeatureIndex by using Python's enumerate() function – let it do the work of tracking the index variable.

def get_feature_index(self, features, name):  
    for idx, feature in enumerate(features):
        feature_name = feature.find('Name')
        if feature_name.text == name:
            return idx

-
Try not to pass around redundant information – for example, the original getDependencyIndex() takes both dependencies and numberOfDependencies as arguments. You only need the first argument – you can derive the second.

This ensures you’re not caught out by callers passing bad data.

-
A nitpick, but in the clearThings() method, you have a variable called clearList which is really a set. If you can avoid little inconsistencies like this, your code will be easier to read.

StackExchange Code Review Q#116462, answer score: 7