6
\$\begingroup\$

I am developing a plugin for a GIS (Geographic Information Systems) software, QGIS, which uses Python. I have created a function which performs an update (i.e. inserts files, or shapefiles, into their respective groups in QGIS which were not already loaded). Essentially, the steps are broken down as follows:

  • Step 1
    • For the "Policy constraints" directory:
      • Get a list to walk through the directory and check if the names of groups match the name of the directories.
        • If there is a match, remove it from the list, otherwise;
        • Load the files, add a style symbology based on their score attribute (if no existing score attribute then one is created), and make it visible.
    • Repeat for the "Technical constraints" directory.

  • Step 2
    • For the "Policy constraints" directory:
      • Get a list to store the names of loaded shapefiles and another list to store the names of all shapefiles in the directory.
        • Create a final list which contains the shapefiles not loaded and load these, add a style symbology based on their score attribute...etc.
    • Repeat for the "Technical constraints" directory.
    • Repeat for the "Context" directory but only load the shapefiles, no styling is applied, nor making it visible.
    • Repeat for the "Area of interest" directory and load the shapefiles, load the style symbology from a file but do not make it visible.

  • If no directories were empty, show a successful message, otherwise a warning message.

The code contains a number of repetitions such as stlying, checking if the attribute exists otherwise add a new one, and multiple os.walks(). What is the best way to make the code more consise?

Here is the code:

# Define root and groups
root = QgsProject.instance().layerTreeRoot()
policy_group = root.findGroup('Policy')
technical_group = root.findGroup('Technical')
area_of_interest_group = root.findGroup('Area of interest')
context_group = root.findGroup('Context')
bridge = iface.layerTreeCanvasBridge()

# Use CRS of city shapefile
for fname in glob.glob(QgsProject.instance().readPath("./") + '/Processing scripts/Shapefile conversion/City/*.shp'):
    initial_crs = QgsVectorLayer( fname, '', 'ogr' ).crs().authid()
    get_crs = int(initial_crs.split(":",1)[-1].split()[0])

# Walk through folders and directories
pol_root, pol_dirs, pol_files = os.walk(QgsProject.instance().readPath("./") + '/Constraints/Policy constraints').next()
tech_root, tech_dirs, tech_files = os.walk(QgsProject.instance().readPath("./") + '/Constraints/Technical constraints').next()

# Set message depending on level
message_level = 1
#############################################################
# Step 1: Adding new groups of layers if group does not exist
#############################################################
# For Policy constraints
# Check if existing group name matches constraint folder name
for group in policy_group.children():
    # If it matches, remove from list
    if group.name() in pol_dirs:
        pol_dirs.remove(group.name())
    else:
        pass
# Check if list is empty
if not pol_dirs:
    pass
else:
    # Else add group containing folder name and its shapefiles
    for folder_name in pol_dirs:
        # Before adding group and its shapefiles, check if folder is empty
        # If folder contains shapefiles
        if glob.glob(pol_root + "/" + folder_name + "/*.shp"):
            group = policy_group.addGroup(folder_name)
            for shapefile in glob.glob(pol_root + "/" + folder_name + "/*.shp"):
                layer = QgsVectorLayer(shapefile, os.path.splitext(os.path.basename(shapefile))[0], "ogr" )
                idx = layer.fieldNameIndex("Score")   
                crs = layer.crs()
                crs.createFromId(get_crs)
                layer.setCrs(crs)
                QgsMapLayerRegistry.instance().addMapLayer(layer, False)
                group.insertChildNode(-1, QgsLayerTreeLayer(layer))                 
                # Set up layer symbology corresponding to saved score
                symbols = layer.rendererV2().symbols()
                symbol = symbols[0]  
                if idx != -1:                  
                    if layer.minimumValue(idx) == 1 and layer.maximumValue(idx) == 1:
                        symbol.setColor(QColor('#dbffdb'))
                    if layer.minimumValue(idx) == 2 and layer.maximumValue(idx) == 2:
                        symbol.setColor(QColor('#f0ab64'))
                    if layer.minimumValue(idx) == 3 and layer.maximumValue(idx) == 3:
                        symbol.setColor(QColor('#963634'))
                    if layer.minimumValue(idx) == 4 and layer.maximumValue(idx) == 4:
                        symbol.setColor(QColor('#1d1b10'))
                    elif layer.minimumValue(idx) != layer.maximumValue(idx):
                        style_rules = (
                            ('Possible', """"Score" IS NULL OR "Score" = 1""", '#dbffdb'),
                            ('Intermediate', """"Score" = 2""", '#f0ab64'),
                            ('Sensitive', """"Score" = 3""", '#963634'),
                            ('Showstopper', """"Score" = 4""", '#1d1b10'),
                        )
                        # Create a new rule-based renderer
                        symbol = QgsSymbolV2.defaultSymbol(layer.geometryType())
                        renderer = QgsRuleBasedRendererV2(symbol)
                        # Get the "root" rule
                        root_rule = renderer.rootRule()
                        for label, expression, color_name in style_rules:
                            # Set outline colour to match that of polygon fill colour
                            # create a clone (i.e. a copy) of the default rule
                            rule = root_rule.children()[0].clone()
                            # set the label, expression and color
                            rule.setLabel(label)
                            rule.setFilterExpression(expression)
                            rule.symbol().setColor(QColor(color_name))
                            # Append the rule to the list of rules
                            root_rule.appendChild(rule)
                        # Delete the default rule
                        root_rule.removeChildAt(0)
                        # Apply the renderer to the layer
                        layer.setRendererV2(renderer)
                        iface.legendInterface().setLayerExpanded(layer, False)
                    layer.triggerRepaint()
                    iface.legendInterface().refreshLayerSymbology(layer)
                    iface.legendInterface().setLayerVisible(layer, True)
                else:
                    layer.startEditing()
                    layer.dataProvider().addAttributes( [ QgsField("Score", QVariant.Int) ] )
                    layer.updateFields()
                    for feat in layer.getFeatures():
                        layer.changeAttributeValue(feat.id(), layer.fieldNameIndex('Score'), '1')
                    layer.commitChanges()
                    symbol.setColor(QColor('#dbffdb'))
                    iface.legendInterface().refreshLayerSymbology(layer)
                    iface.legendInterface().setLayerVisible(layer, True)
            message_level = 1
        else:
            # If folder is empty
            message_level = 2

# For Technical constraints
# Check if existing group name matches constraint folder name
for group in technical_group.children():
    # If it matches, remove from list
    if group.name() in tech_dirs:
        tech_dirs.remove(group.name())
    else:
        pass
# Check if list is empty
if not tech_dirs:
    pass
else:
    # Else add group containing folder name and its shapefiles
    for folder_name in tech_dirs:
        # Before adding group and its shapefiles, check if folder is empty
        # If folder contains shapefiles
        if glob.glob(tech_root + "/" + folder_name + "/*.shp"):
            group = technical_group.addGroup(folder_name)
            for shapefile in glob.glob(tech_root + "/" + folder_name + "/*.shp"):
                layer = QgsVectorLayer(shapefile, os.path.splitext(os.path.basename(shapefile))[0], "ogr" )
                idx = layer.fieldNameIndex("Score")                    
                crs = layer.crs()
                crs.createFromId(get_crs)
                layer.setCrs(crs)
                QgsMapLayerRegistry.instance().addMapLayer(layer, False)
                group.insertChildNode(-1, QgsLayerTreeLayer(layer))                        
                # Set up layer symbology corresponding to saved score
                symbols = layer.rendererV2().symbols()
                symbol = symbols[0]   
                if idx != -1:                 
                    if layer.minimumValue(idx) == 1 and layer.maximumValue(idx) == 1:
                        symbol.setColor(QColor('#dbffdb'))
                    if layer.minimumValue(idx) == 2 and layer.maximumValue(idx) == 2:
                        symbol.setColor(QColor('#f0ab64'))
                    if layer.minimumValue(idx) == 3 and layer.maximumValue(idx) == 3:
                        symbol.setColor(QColor('#963634'))
                    elif layer.minimumValue(idx) != layer.maximumValue(idx):
                        style_rules = (
                            ('Favourable', """"Score" IS NULL OR "Score" = 1""", '#dbffdb'),
                            ('Likely', """"Score" = 2""", '#f0ab64'),
                            ('Unlikely', """"Score" = 3""", '#963634'),
                        )
                        # Create a new rule-based renderer
                        symbol = QgsSymbolV2.defaultSymbol(layer.geometryType())
                        renderer = QgsRuleBasedRendererV2(symbol)
                        # Get the "root" rule
                        root_rule = renderer.rootRule()
                        for label, expression, color_name in style_rules:
                            # Set outline colour to match that of polygon fill colour
                            # Create a clone (i.e. a copy) of the default rule
                            rule = root_rule.children()[0].clone()
                            # Set the label, expression and color
                            rule.setLabel(label)
                            rule.setFilterExpression(expression)
                            rule.symbol().setColor(QColor(color_name))
                            # Append the rule to the list of rules
                            root_rule.appendChild(rule)
                        # Delete the default rule
                        root_rule.removeChildAt(0)
                        # Apply the renderer to the layer
                        layer.setRendererV2(renderer)
                        iface.legendInterface().setLayerExpanded(layer, False)
                    layer.triggerRepaint()
                    iface.legendInterface().refreshLayerSymbology(layer)
                    iface.legendInterface().setLayerVisible(layer, True)
                else:
                    layer.startEditing()
                    layer.dataProvider().addAttributes( [ QgsField("Score", QVariant.Int) ] )
                    layer.updateFields()
                    for feat in layer.getFeatures():
                        layer.changeAttributeValue(feat.id(), layer.fieldNameIndex('Score'), '1')
                    layer.commitChanges()
                    symbol.setColor(QColor('#dbffdb'))
                    iface.legendInterface().refreshLayerSymbology(layer)
                    iface.legendInterface().setLayerVisible(layer, True)
            if message_level == 2:
                message_level = 2
            else:
                message_level = 1
        else:
            # If folder is empty
            message_level = 2


###################################################################
# Step 2: Update existing groups with newly added layers to folders
###################################################################
# For Policy constraints
pol_root, pol_dirs, pol_files = os.walk(QgsProject.instance().readPath("./") + '/Constraints/Policy constraints').next()
# Create empty list
pol_list = []
# Find all layers loaded in QGIS
for group in policy_group.children():
    for pol_layer in group.children():
        pol_list.append(pol_layer.layer().source())
new_pol_list = [x.encode('UTF8') for x in pol_list]
second_new_pol_list = [l.replace('\\', '/') for l in new_pol_list]
# Create empty list
pol_dir_list = []
# Find all layers in constraint directories
for folder_name in pol_dirs:
    for shapefile in glob.glob(pol_root + "/" + folder_name + "/*.shp"):
        pol_dir_list.append(shapefile)
# Format the list to match same format as the list above
new_pol_dir_list = [l.replace('\\', '/') for l in pol_dir_list]
# Find paths of shapefiles in directories which are not loaded in QGIS
pol_missing_shapefiles = [x for x in new_pol_dir_list if x not in second_new_pol_list]
# For all shapefiles not loaded in QGIS, add them to relevant group with symbology
for shapefile in pol_missing_shapefiles:
    paths = os.path.dirname(shapefile)
    group_name = paths.rsplit('/', 1)[-1]
    group = root.findGroup(group_name)
    layer = QgsVectorLayer(shapefile, os.path.splitext(os.path.basename(shapefile))[0], "ogr" )
    idx = layer.fieldNameIndex("Score")   
    crs = layer.crs()
    crs.createFromId(get_crs)
    layer.setCrs(crs)
    QgsMapLayerRegistry.instance().addMapLayer(layer, False)
    group.insertChildNode(-1, QgsLayerTreeLayer(layer))
    # Set up layer symbology corresponding to saved score
    symbols = layer.rendererV2().symbols()
    symbol = symbols[0] 
    if idx != -1:            
        if layer.minimumValue(idx) == 1 and layer.maximumValue(idx) == 1:
            symbol.setColor(QColor('#dbffdb'))
        if layer.minimumValue(idx) == 2 and layer.maximumValue(idx) == 2:
            symbol.setColor(QColor('#f0ab64'))
        if layer.minimumValue(idx) == 3 and layer.maximumValue(idx) == 3:
            symbol.setColor(QColor('#963634'))
        if layer.minimumValue(idx) == 4 and layer.maximumValue(idx) == 4:
            symbol.setColor(QColor('#1d1b10'))
        elif layer.minimumValue(idx) != layer.maximumValue(idx):
            style_rules = (
                ('Possible', """"Score" IS NULL OR "Score" = 1""", '#dbffdb'),
                ('Intermediate', """"Score" = 2""", '#f0ab64'),
                ('Sensitive', """"Score" = 3""", '#963634'),
                ('Showstopper', """"Score" = 4""", '#1d1b10'),
            )
            # Create a new rule-based renderer
            symbol = QgsSymbolV2.defaultSymbol(layer.geometryType())
            renderer = QgsRuleBasedRendererV2(symbol)
            # Get the "root" rule
            root_rule = renderer.rootRule()
            for label, expression, color_name in style_rules:
                # Set outline colour to match that of polygon fill colour
                # Create a clone (i.e. a copy) of the default rule
                rule = root_rule.children()[0].clone()
                # Set the label, expression and color
                rule.setLabel(label)
                rule.setFilterExpression(expression)
                rule.symbol().setColor(QColor(color_name))
                # Append the rule to the list of rules
                root_rule.appendChild(rule)
            # Delete the default rule
            root_rule.removeChildAt(0)
            # Apply the renderer to the layer
            layer.setRendererV2(renderer)
            iface.legendInterface().setLayerExpanded(layer, False)
        layer.triggerRepaint()
        iface.legendInterface().refreshLayerSymbology(layer)
        iface.legendInterface().setLayerVisible(layer, True)
    else:
        layer.startEditing()
        layer.dataProvider().addAttributes( [ QgsField("Score", QVariant.Int) ] )
        layer.updateFields()
        for feat in layer.getFeatures():
            layer.changeAttributeValue(feat.id(), layer.fieldNameIndex('Score'), '1')
        layer.commitChanges()
        symbol.setColor(QColor('#dbffdb'))
        iface.legendInterface().refreshLayerSymbology(layer)
        iface.legendInterface().setLayerVisible(layer, True)
    if message_level == 2:
        message_level = 2
    if message_level == 1:
        message_level = 1


# For Technical constraints
tech_root, tech_dirs, tech_files = os.walk(QgsProject.instance().readPath("./") + '/Constraints/Technical constraints').next()
# Create empty list
tech_list = []
# Find all layers loaded in QGIS
for group in technical_group.children():
    for tech_layer in group.children():
        tech_list.append(tech_layer.layer().source())
new_tech_list = [x.encode('UTF8') for x in tech_list]
second_new_tech_list = [l.replace('\\', '/') for l in new_tech_list]
# Create empty list
tech_dir_list = []
# Find all layers in constraint directories
for folder_name in tech_dirs:
    for shapefile in glob.glob(tech_root + "/" + folder_name + "/*.shp"):
        tech_dir_list.append(shapefile)
# Format the list to match same format as the list above
new_tech_dir_list = [l.replace('\\', '/') for l in tech_dir_list]
# Find paths of shapefiles in directories which are not loaded in QGIS
tech_missing_shapefiles = [x for x in new_tech_dir_list if x not in second_new_tech_list]
# For all shapefiles not loaded in QGIS, add them to relevant group with symbology
for shapefile in tech_missing_shapefiles:
    paths = os.path.dirname(shapefile)
    group_name = paths.rsplit('/', 1)[-1]
    group = root.findGroup(group_name)
    layer = QgsVectorLayer(shapefile, os.path.splitext(os.path.basename(shapefile))[0], "ogr" )
    idx = layer.fieldNameIndex("Score")  
    crs = layer.crs()
    crs.createFromId(get_crs)
    layer.setCrs(crs)
    QgsMapLayerRegistry.instance().addMapLayer(layer, False)
    group.insertChildNode(-1, QgsLayerTreeLayer(layer))
    # Set up layer symbology corresponding to saved score
    symbols = layer.rendererV2().symbols()
    symbol = symbols[0]                 
    if idx != -1:                 
        if layer.minimumValue(idx) == 1 and layer.maximumValue(idx) == 1:
            symbol.setColor(QColor('#dbffdb'))
        if layer.minimumValue(idx) == 2 and layer.maximumValue(idx) == 2:
            symbol.setColor(QColor('#f0ab64'))
        if layer.minimumValue(idx) == 3 and layer.maximumValue(idx) == 3:
            symbol.setColor(QColor('#963634'))
        elif layer.minimumValue(idx) != layer.maximumValue(idx):
            style_rules = (
                ('Favourable', """"Score" IS NULL OR "Score" = 1""", '#dbffdb'),
                ('Likely', """"Score" = 2""", '#f0ab64'),
                ('Unlikely', """"Score" = 3""", '#963634'),
            )
            # Create a new rule-based renderer
            symbol = QgsSymbolV2.defaultSymbol(layer.geometryType())
            renderer = QgsRuleBasedRendererV2(symbol)
            # Get the "root" rule
            root_rule = renderer.rootRule()
            for label, expression, color_name in style_rules:
                # Set outline colour to match that of polygon fill colour
                # Create a clone (i.e. a copy) of the default rule
                rule = root_rule.children()[0].clone()
                # Set the label, expression and color
                rule.setLabel(label)
                rule.setFilterExpression(expression)
                rule.symbol().setColor(QColor(color_name))
                # Append the rule to the list of rules
                root_rule.appendChild(rule)
            # Delete the default rule
            root_rule.removeChildAt(0)
            # Apply the renderer to the layer
            layer.setRendererV2(renderer)
            iface.legendInterface().setLayerExpanded(layer, False)
        layer.triggerRepaint()
        iface.legendInterface().refreshLayerSymbology(layer)
        iface.legendInterface().setLayerVisible(layer, True)
    else:
        layer.startEditing()
        layer.dataProvider().addAttributes( [ QgsField("Score", QVariant.Int) ] )
        layer.updateFields()
        for feat in layer.getFeatures():
            layer.changeAttributeValue(feat.id(), layer.fieldNameIndex('Score'), '1')
        layer.commitChanges()
        symbol.setColor(QColor('#dbffdb'))
        iface.legendInterface().refreshLayerSymbology(layer)
        iface.legendInterface().setLayerVisible(layer, True)
    if message_level == 2:
        message_level = 2
    if message_level == 1:
        message_level = 1


# For Context layers
context_root, context_dirs, context_files = os.walk(QgsProject.instance().readPath("./") + '/Geospatial information/Context').next()
# Create empty list
context_list = []
# Find all layers loaded in QGIS
for context_layer in context_group.children():
    try:
        context_list.append(context_layer.layer().source())
    except AttributeError:
        pass
new_context_list = [x.encode('UTF8') for x in context_list]
second_new_context_list = [l.replace('\\', '/') for l in new_context_list]
# Create empty list
context_dir_list = []
# Find all layers in context directory
for context_file in glob.glob(context_root + "/*"):
    context_dir_list.append(context_file)
# Format the list to match same format as the list above
new_context_dir_list = [l.replace('\\', '/') for l in context_dir_list]
# Find paths of shapefiles in directories which are not loaded in QGIS
context_missing_files = [x for x in new_context_dir_list if x not in second_new_context_list]
# For all shapefiles not loaded in QGIS, add them to relevant group with symbology
for file in context_missing_files:
    if file.lower().endswith('.shp'):
        vlayer = QgsVectorLayer(file, os.path.splitext(os.path.basename(file))[0], "ogr" )
        crs.createFromId(get_crs)
        vlayer.setCrs(crs)
        QgsMapLayerRegistry.instance().addMapLayer(vlayer, False)
        context_group.insertChildNode(-1, QgsLayerTreeLayer(vlayer))
        iface.legendInterface().setLayerVisible(vlayer, False)
        order = bridge.customLayerOrder()
        order.insert( 0, order.pop(order.index(vlayer.id())))
        bridge.setCustomLayerOrder(order) 
    if file.lower().endswith('.tif'):
        fileName = file
        fileInfo = QFileInfo(fileName)
        baseName = fileInfo.baseName()
        crs.createFromId(get_crs)
        rlayer.setCrs(crs)
        QgsMapLayerRegistry.instance().addMapLayer(rlayer, False)
        rlayer = QgsRasterLayer(fileName, baseName)
        context_group.insertChildNode(-1, QgsLayerTreeLayer(rlayer))
        iface.legendInterface().setLayerVisible(rlayer, False)
        order = bridge.customLayerOrder()
        order.insert( 0, order.pop(order.index(rlayer.id())))
        bridge.setCustomLayerOrder(order) 


# For Scope layers
scope_root, scope_dirs, scope_files = os.walk(QgsProject.instance().readPath("./") + '/Geospatial information/Area of interest').next()
# Create empty list
scope_list = []
# Find all layers loaded in QGIS
for scope_layer in area_of_interest_group.children():
    scope_list.append(scope_layer.layer().source())
new_scope_list = [x.encode('UTF8') for x in scope_list]
second_new_scope_list = [l.replace('\\', '/') for l in new_scope_list]
# Create empty list
scope_dir_list = []
# Find all layers in scope directory
for shapefile in glob.glob(scope_root + "/*.shp"):
    scope_dir_list.append(shapefile)
# Format the list to match same format as the list above
new_scope_dir_list = [l.replace('\\', '/') for l in scope_dir_list]
# Find paths of shapefiles in directories which are not loaded in QGIS
scope_missing_shapefiles = [x for x in new_scope_dir_list if x not in second_new_scope_list]
# For all shapefiles not loaded in QGIS, add them to relevant group with symbology
for shapefile in scope_missing_shapefiles:
    layer = QgsVectorLayer(shapefile, os.path.splitext(os.path.basename(shapefile))[0], "ogr" )
    crs = layer.crs()
    crs.createFromId(get_crs)
    layer.setCrs(crs)
    QgsMapLayerRegistry.instance().addMapLayer(layer, False)
    area_of_interest_group.insertChildNode(-1, QgsLayerTreeLayer(layer))
    layer.loadNamedStyle(self.plugin_dir + '/styles/scope_style.qml')
    iface.legendInterface().refreshLayerSymbology(layer)
    iface.legendInterface().setLayerVisible(layer, False)
    if message_level == 2:
        message_level = 2
    if message_level == 1:
        message_level = 1

###################################################################  
# Show messages based on level        
#if message_level == 0: 
#    iface.messageBar().pushSuccess( u'Nothing to update', '' )
iface.messageBar().clearWidgets()
if message_level == 1:
    iface.messageBar().pushSuccess( u'Project updated', '' )
if message_level == 2:
    iface.messageBar().pushWarning( u'Project updated but some constraint(s) were not added due to \
                empty directory', '' )
\$\endgroup\$
  • \$\begingroup\$ Is it likely to add say a step 3+ that would work over directories like your other steps? \$\endgroup\$ – Peilonrayz Aug 21 '17 at 12:26
  • \$\begingroup\$ @Peilonrayz - Apologies but I'm not sure what you mean. Can you clarify please? :) \$\endgroup\$ – Joseph Aug 21 '17 at 12:30
  • 1
    \$\begingroup\$ Currently step 1 and step 2 work on the "Policy constraints" directory, amongst others, is it likely that you'd add another step to this (at a later date), that also works on the "Policy constraints" directory, or one of the other directories? \$\endgroup\$ – Peilonrayz Aug 21 '17 at 12:33
  • \$\begingroup\$ @Peilonrayz - Ahh thanks, yes it would be likely. The plugin would be an ongoing development and would have new functionality implemented at later stages. So if there's a method to introduce these new functions easily then it is more than welcome ;) \$\endgroup\$ – Joseph Aug 21 '17 at 12:37
  • 1
    \$\begingroup\$ Do you need to glob the shapefiles like that? The usual OGR tools treat a directory full of shapefiles as a single resource (so I can write ogrinfo -l data/, for example); have you checked whether QgsVectorLayer will do likewise? \$\endgroup\$ – Toby Speight Aug 22 '17 at 14:22
6
\$\begingroup\$

Wow. That's a lot of code. The very first thing that needs to be done with it is modularization. You need to structure your code such that named functions and/or classes encapsulate shared functionality and make it more readable. For example, if I'm writing a function that takes some user input, validates it, and performs a lot of operations, I'm going to end up with 50+ lines of code that is really hard to understand. If I were to put that in functions, e.g.

def foo_the_bar(bar):
    user_foo = None
    while not user_foo:
        user_foo = get_user_foo()

    return do_the_foo(user_foo, bar)

This is much easier to understand for anyone (especially you) to read when you come back to it later than

def foo_the_bar(bar):
    user_foo = None
    while not user_foo:
        user_foo = raw_input("What is your foo? ")
        if len(user_foo) < 10:
            print "That isn't a good foo"
            user_foo = None
        # many other validation conditions

    user_foo = initialize_foo(user_foo)
    # do other operations
    return boo.fooer(user_foo)

This is, admittedly, a silly example, but it makes the logical structure of your code much, much simpler. If you're uncertain about how to break up the code, or if there isn't much repetition to logically separate out, look for your comments. If the comment adds no value, e.g.

# add two numbers
x = y + z

Then just delete it. Similarly, if it explains how the code works (unless horrendously complicated) delete it as well. If it explains what the code does, move that bit of code into a function, and use a good name for that function that explains what it does. Lastly, if it explains why the code is done that way, e.g.

# remove the invalid element that the 3rd-party library always appends
x = x[:-1]

You can either leave the comment in-place (because otherwise no-one would understand why you're doing something) or you can move it into a function (recommended if you have to do the same thing multiple times), e.g.

def remove_invalid_element_from_<mythirdpartylib>(arr):
    return arr[:-1]

I'm not going to go through the entire ~500 lines of code in your example, but I'll highlight a few key points.

Possible places to modularize your code.

Above, when you were describing the work flow, anytime you use the word "Repeat" you can probably put it into a function. For example, the first step has almost no differences between the technical group and policy group handling:

  1. They have different variable names
  2. The colors used are different
  3. The style rules are different
  4. The message levels are different

These are all really easy to address by moving it into a shared function.

  1. You don't need the specific variable names in the function - instead of pol_dirs and tech_dirs just use dirs, and similarly for technical_group and policy_group just use group.
  2. Just pass in the color dictionary (see below) as a parameter and set it appropriately for the group type
  3. Same as 2, except for the style rules tuple
  4. Instead of keeping track of the message level inside of the function, just return whatever the message level should be and outside of the function you can just take the max of the two.

Doing just that, which takes almost no work, saves you almost 100 lines of code. You can do something very similar for step 2 - the code is more distinct between sections, but if you're clever about it you can remove a ton of the duplication.

Here are some other, general pieces of advice.

Don't use pass just to say "do nothing"

Oftentimes you can exclude pass entirely, or slightly restructure your code to avoid it. It adds very little value in 99.9% of cases.

for group in policy_group.children():
    # If it matches, remove from list
    if group.name() in pol_dirs:
        pol_dirs.remove(group.name())
    else:
        pass

Because you aren't doing anything only in the else branch, just delete it.

if not pol_dirs:
    pass
else:
    # Else add group containing folder name and its shapefiles
    for folder_name in pol_dirs:

In this case, just invert your condition

if pol_dirs:
    for folder_name in pol_dirs:

Even easier though, because pol_dirs is a list, you can just do

for folder_name in pol_dirs:

Iterating over an empty iterable using a for-loop just skips the loop.

Safely concat your file paths

You're manually building your file paths, e.g.

if glob.glob(pol_root + "/" + folder_name + "/*.shp"):
for shapefile in glob.glob(pol_root + "/" + folder_name + "/*.shp"):
os.walk(QgsProject.instance().readPath("./") + '/Constraints/Policy constraints').next()

First of all, raw string concatenation tends to be fairly inefficient. Second of all, the file separator isn't guaranteed to be the same for all OS. Although you may not be planning on going to other OS now, why make it harder if you ever want to do so? Furthermore, by using a common idiom, any other Python programmer will easily recognize what the line of code is supposed to do. Just use os.path.join instead. Those lines would become

if glob.glob(os.path.join(pol_root, folder_name, "*.shp")):
for shapefile in glob.glob(os.path.join(pol_root, folder_name, "*.shp")):
os.walk(os.path.join(QgsProject.instance().readPath("./"), "Constraints", "Policy constraints")).next()

While you're at it,

Put values in variables if you use them more than once

In this case, you do glob.glob(pol_root + "/" + folder_name + "/*.shp") at least twice, and I'm sure that there are other examples. Come up with a good variable name for it and use that value instead.

Use dictionaries for some if chains

if layer.minimumValue(idx) == 1 and layer.maximumValue(idx) == 1:
    symbol.setColor(QColor('#dbffdb'))
if layer.minimumValue(idx) == 2 and layer.maximumValue(idx) == 2:
    symbol.setColor(QColor('#f0ab64'))
if layer.minimumValue(idx) == 3 and layer.maximumValue(idx) == 3:
    symbol.setColor(QColor('#963634'))
if layer.minimumValue(idx) == 4 and layer.maximumValue(idx) == 4:
    symbol.setColor(QColor('#1d1b10'))

This would be much cleaner if you encode the values in a dictionary, like so:

color_mapping = { 1: "dbffdb", 2: "f0ab64", 3: "963634", 4: "1d1b10" }
if layer.minimumValue(idx) == layer.maximumValue(idx):
    color = color_mapping.get(layer.minimumValue(idx), "default color")
    symbol.setColor(QColor('#{color}'.format(color=color)))

Move shared code out as many layers as possible

When you're handling the policy folders in step 1, you have a condition like

if idx != -1:
    ...
    iface.legendInterface().refreshLayerSymbology(layer)
    iface.legendInterface().setLayerVisible(layer, True)
else:
    ...
    iface.legendInterface().refreshLayerSymbology(layer)
    iface.legendInterface().setLayerVisible(layer, True)

Just move the repeated code outside of the if/else statements as it happens either way.

\$\endgroup\$

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.