# Composing QGIS expression based on a QGIS layer tree

I am developing a plugin for a GIS (Geographic Information Systems) software which uses Python. I have been using Python for over a year (on-off) and have the following code which works. It basically reads the names of files loaded into the GIS software to compose a QGIS expression for every group of files. Finally it deletes the list, ready for the next group.

The script is quite chunky and uses repetition. I am wondering how would one improve it to make it more compact and run more efficiently?

root = QgsProject.instance().layerTreeRoot()
policy_group = root.findGroup('Names')
formula_score4 = []
formula_score3a = []
formula_score3 = []
formula_score2a = []
formula_score2 = []
formula_score1 = []
for group in policy_group.children():
for layer in group.children():
score4 = '"' + layer.layerName() + '_Score' + '"' + ' = 4 OR '
formula_score4.append(score4)
for layer in group.children():
score3a = 'coalesce(' + layer.layerName() + ' = 3, 0.00)*3 OR '
formula_score3a.append(score3a)
for layer in group.children():
score3 = '"' + layer.layerName() + '_Score' + '"' + ' = 3 OR '
formula_score3.append(score3)
for layer in group.children():
score2a = 'coalesce(' + layer.layerName() + ' = 2, 0.00)*2 OR '
formula_score2a.append(score2a)
for layer in group.children():
score2 = '"' + layer.layerName() + '_Score' + '"' + ' = 2 OR '
formula_score2.append(score2)
for layer in group.children():
score1 = '"' + layer.layerName() + '_Score' + '"' + ' = 1 OR '
formula_score1.append(score1)
formula1 = "CASE WHEN " + "".join(str(x) for x in formula_score4) + "".join(str(x) for x in formula_score3a) + ">=9 THEN 4 " + "WHEN " + "".join(str(x) for x in formula_score3) + "".join(str(x) for x in formula_score2a) + ">=6 THEN 3 " + "WHEN " + "".join(str(x) for x in formula_score2) + "THEN 2 " + "WHEN " + "".join(str(x) for x in formula_score1) + "THEN 1 ELSE 1 END"
formula2 = formula1.replace("OR >=9 THEN 4 ", ">=9 THEN 4 ")
formula3 = formula2.replace("OR >=6 THEN 3 ", ">=6 THEN 3 ")
formula4 = formula3.replace("OR THEN 2 ", "THEN 2 ")
final_formula = formula4.replace("OR THEN 1 ELSE 1 END", "THEN 1 ELSE 1 END")
del formula_score4[:]
del formula_score3a[:]
del formula_score3[:]
del formula_score2a[:]
del formula_score2[:]
del formula_score1[:]


I have included a screenshot of running the above code in the Python Console of QGIS. I believe the result is what I was wanting from the code as it matches the same sql expression I wrote manually which is stored in a text file (instead of hardcoding the names of layers and reading the textfile, I wanted a more dynamic approach). Here is the image:

And here is the output for the first group (formatted for readability):

'CASE WHEN "Layer_1a_Score" = 4 OR
"Layer_1b_Score" = 4 OR
"Layer_1c_Score" = 4 OR
"Layer_1d_Score" = 4 OR
"Layer_1e_Score" = 4 OR
"Layer_1f_Score" = 4 OR
coalesce(Layer_1a = 3, 0.00)*3 OR
coalesce(Layer_1b = 3, 0.00)*3 OR
coalesce(Layer_1c = 3, 0.00)*3 OR
coalesce(Layer_1d = 3, 0.00)*3 OR
coalesce(Layer_1e = 3, 0.00)*3 OR
coalesce(Layer_1f = 3, 0.00)*3 >=9
THEN 4 WHEN
"Layer_1a_Score" = 3 OR
"Layer_1b_Score" = 3 OR
"Layer_1c_Score" = 3 OR
"Layer_1d_Score" = 3 OR
"Layer_1e_Score" = 3 OR
"Layer_1f_Score" = 3 OR
coalesce(Layer_1a = 2, 0.00)*2 OR
coalesce(Layer_1b = 2, 0.00)*2 OR
coalesce(Layer_1c = 2, 0.00)*2 OR
coalesce(Layer_1d = 2, 0.00)*2 OR
coalesce(Layer_1e = 2, 0.00)*2 OR
coalesce(Layer_1f = 2, 0.00)*2 >=6
THEN 3 WHEN
"Layer_1a_Score" = 2 OR
"Layer_1b_Score" = 2 OR
"Layer_1c_Score" = 2 OR
"Layer_1d_Score" = 2 OR
"Layer_1e_Score" = 2 OR
"Layer_1f_Score" = 2
THEN 2 WHEN
"Layer_1a_Score" = 1 OR
"Layer_1b_Score" = 1 OR
"Layer_1c_Score" = 1 OR
"Layer_1d_Score" = 1 OR
"Layer_1e_Score" = 1 OR
"Layer_1f_Score" = 1
THEN 1 ELSE 1 END'
...

• I am skeptical that this produces the final_formula that you intend. Could you edit the question to include some sample output? – 200_success May 10 '16 at 22:55
• @200_success - Thank you, I've edited the question to include a screenshot of executing the code and the printout obtained =) – Joseph May 12 '16 at 9:33
• @200_success - Funnily enough, there is a mistake. The coalesce()functions should be added together coalesce(Layer_1a = 2, 0.00)*2 + coalesce(Layer_1b = 2, 0.00)*2 + .... However, using both methods answered, this was easily solvable but thanks for making me check the code again ;) – Joseph May 12 '16 at 12:57
• That makes a bit more sense than before. But do you actually mean coalesce(Layer_1a_SCORE = 2, 0.00)*2 + coalesce(Layer_1b_Score = 2, 0.00)*2 + ...? – 200_success May 12 '16 at 18:48
• @Joseph Agreed. Fixed. Feel free to ask a new question with the improved code, linking back to this question. Welcome to Code Review! – Mast May 13 '16 at 11:46

You do quite a lot or repetition. Using str.format you should be able to make your code smaller by using a comprehensions or map. Doing these inside a ''.join can allow you to reduce a lot of your code. And if you make a list to iterate over you can do this all in two comprehensions. I also think you should make a comprehension to get the layer name from your group.children(). Finally you will want to build formula1 from this which can be done with another string with format.

As all the strings used to format the data are constant, you can use UPPER_SNAKE_CASE variable names to say it's a constant. And should result in something like:

SCORE_FORMATS = [
'"{}_Score" = 1 OR ',
'"{}_Score" = 2 OR ',
'"{}_Score" = 3 OR ',
'"{}_Score" = 4 OR ',
'coalesce({} = 2, 0.00)*2 OR ',
'coalesce({} = 3, 0.00)*3 OR ']
FORMULA1 = "CASE WHEN {3}{5}>=9 THEN 4 WHEN {2}{4}>=6 THEN 3 WHEN {1}THEN 2 WHEN {0}THEN 1 ELSE 1 END"


Finally to make the main part of your code, formula1 to final_formula, you can use the following as I explained above:

for group in policy_group.children():
group_children = [layer.layerName() for layer in group.children()]
formulas = [
''.join(map(formula_format.format, group_children))
for formula_format in SCORE_FORMATS
]
formula1 = FORMULA1.format(*formulas)
formula2 = formula1.replace("OR >=9 THEN 4 ", ">=9 THEN 4 ")
formula3 = formula2.replace("OR >=6 THEN 3 ", ">=6 THEN 3 ")
formula4 = formula3.replace("OR THEN 2 ", "THEN 2 ")
final_formula = formula4.replace("OR THEN 1 ELSE 1 END", "THEN 1 ELSE 1 END")


The output of this function only uses final_formula where I was originally unsure if you were using all the formulas. You also used the wrong delimiter for the coalesce.

To improve on the above I'd change SCORE_FORMATS to be tuples of (delimiter, format). This will allow you to easily change both for all your different functions. This will also change the ''.join(map(...)) to use your delimiter and will remove the need for all the replace calls.

This unfortunately makes the FORMULA a bit longer, but give us an excuse to use implicit string concatenation which can make it much easier to read.

SCORE_FORMATS = [
(' OR ', '"{}_Score" = 1'),
(' OR ', '"{}_Score" = 2'),
(' OR ', '"{}_Score" = 3'),
(' OR ', '"{}_Score" = 4'),
(' + ', 'coalesce({} = 2, 0.00)*2'),
(' + ', 'coalesce({} = 3, 0.00)*3')]
FORMULA = ('CASE WHEN {3} OR {5} >= 9 THEN 4 '
'WHEN {2} OR {4} >= 6 THEN 3 '
'WHEN {1} THEN 2 '
'WHEN {0} THEN 1 '
'ELSE 1 END')

for group in policy_group.children():
group_children = [layer.layerName() for layer in group.children()]
formula = FORMULA.format(*[
delimiter.join(map(formula_format.format, group_children))
for delimiter, formula_format in SCORE_FORMATS
])

• Wow, that makes quite a difference! Thank you very much for this, I will implement this and see how it goes :) – Joseph May 12 '16 at 9:35
• Wish I could accept both answers but will accept this one as you answered first with very readable code =) – Joseph May 12 '16 at 14:30
• @Joseph Thank you, :) Just wondering, do you only want final_formula? Cause you can make the algorithm much better, and from your examples that you added you seem to only use it... (I didn't know, so I tried to keep everything) – Peilonrayz May 12 '16 at 14:45
• Most welcome, I appreciate you keeping everything in the code! At the moment, it is just the final_formula I need but I'm curious as to what you mean by making the "algorithm much better"? :) – Joseph May 12 '16 at 14:54
• @Joseph "please don't feel obligated to improve it", it's too late for that now. It will be improved. – Peilonrayz May 12 '16 at 15:11

I'd like to approach this answer as a refactoring exercise. I'm going to iteratively improve your code little-by-little until the final result.

## 1. Remove unnecessary declarations and deletions

The declarations before the for group in ... of formula_score4, etc., and the del formula_score4[:], etc., at the end, are unnecessary. The formula_scoreX variables are temporary variables, rewritten each time through the loop. Python will handle their allocation and cleanup.

## 2. Create a temporary layers list

To reduce the number of times group.children() and layer.layerName() are called, let's define a layers list at the top of the outer for loop:

for group in policy_group.children():
layers = [layer.layerName() for layer in group.children()]
...


Now your inner for loops will just iterate over layers as such: for layer in layers. This is a minor improvement now, but it pays off in readability. And it's possible that there is a performance gain by eliminating multiple redundant calls to group.children() and layer.layerName() (not very likely for small sets of data, but it can add up as the sizes of the loops get very large).

## 3. Recognize the opportunity for Python's string .format() method

The 6 for loops inside the main for loop do similar things. There are 2 types of strings being generated: one that needs to look like "{layerName}_Score" = {N}; and one that should look like coalesce({layerName} = {N}, 0.00)*{N} (where {layerName} is the name of the layer, and {N} is the score "type" (i.e., 4, 3, 2, 1).

If you define

formatstr  = '"{layerName}_Score" = {N} OR '
formatstrA = 'coalesce({layerName} = {N}, 0.00)*{N} OR '


Then your inner loops would look similar to this:

for layer in layers:
score4 = formatstr.format(layerName = layer, N = 4)
formula_score4.append(score4)


## 4. Utilize Python's List Comprehensions

The loop structure at the end of the previous section is just a wordy version of Python's list comprehension. Instead of iterating one-by-one over a list of layer strings, and appending strings one-by-one to an output list, Python's list comprehensions do the same thing, without the verbosity. The Pythonic way to write the inner loops is: formula_score4 = [formatstr.format(layerName = layer), 4) for layer in layers]. Thus, we can reduce the top part of the outer loop to:

for group in policy_group.children():
layers = [g.layerName() for g in group.children()]
formula_score4 = [formatstr.format(layerName = layer, N = 4) for layer in layers]
formula_score3 = [formatstr.format(layerName = layer, N = 3) for layer in layers]
formula_score2 = [formatstr.format(layerName = layer, N = 2) for layer in layers]
formula_score1 = [formatstr.format(layerName = layer, N = 1) for layer in layers]
formula_score3a = [formatstrA.format(layerName = layer, N = 3) for layer in layers]
formula_score2a = [formatstrA.format(layerName = layer, N = 2) for layer in layers]
# ...


## 5. Break up long lines

This is a very simple suggestion, and doesn't do anything other than improve readability. But please, don't make your lines span so many columns. This only applies to your formula1 assignment, but it is so egregious. Also, there is no need to break up string constants in that assignment (i.e., ... + ">=9 THEN 4 WHEN " + ... instead of ... + ">=9 THEN 4 " + "WHEN " + ...).

Cleaning up formula1 = ... by surrounding the right-hand side in parentheses so it can be broken up with requiring trailing backslashes:

    formula1 = ("CASE WHEN " +
"".join(str(x) for x in formula_score4) +
"".join(str(x) for x in formula_score3a) + ">=9 THEN 4 WHEN " +
"".join(str(x) for x in formula_score3) +
"".join(str(x) for x in formula_score2a) + ">=6 THEN 3 WHEN " +
"".join(str(x) for x in formula_score2) + "THEN 2 WHEN " +
"".join(str(x) for x in formula_score1) + "THEN 1 ELSE 1 END")


I think you'll agree this is so much more readable and understandable.

## 6. Use delimiter.join() to join strings together

Your formula score strings all end in " OR " because you are building up your final query. But then you have to use .replace() to remove the final " OR " from each formula score type.

Say you have a short list of strings, l = ['abc', 'def', 'ghi'], that you wanted to join together, separated by delimiter (such as ' OR '). Then the best choice is the .join() method, but using a delimiter:

>>> l = ['abc', 'def', 'ghi']
>>> print ' OR '.join(l)
abc OR def OR ghi


You can see where I'm going here. Instead of appending ' OR ' to every layer name formula score string, and then having to search and replace the final ' OR ' before the value comparison, just .join() the strings together using a delimiter. Bonus: you can remove the potentially expensive .replace() calls.

1. Change the format strings (remove the trailing ' OR ' from them):

formatstr  = '"{layerName}_Score" = {N}'
formatstrA = 'coalesce({layerName} = {N}, 0.00)*{N}'

2. Change the formula_scoreX variables to be your built-up strings, instead of lists of strings that have to be joined later:

formula_score4  = ' OR '.join(formatstr.format(layerName = layer, N = 4) for layer in layers]
# similarly for formula_score3, formula_score2, formula_score1
formula_score3a = ' OR '.join(formatstrA.format(layerName = layer, N = 3) for layer in layers]
# and similarly for formula_score2a

3. Delete the formula2 = ..., formula3 = ..., formula4 = ..., and final_formula assignments. You no longer need the calls to .replace().

4. Fix the formula1 assignment, rename it to just formula :

formula = ("CASE WHEN " +
formula_score4 + formula_score3a + ">=9 THEN 4 WHEN " +
formula_score3 + formula_score2a + ">=6 THEN 3 WHEN " +
formula_score2 + "THEN 2 WHEN " +
formula_score1 + "THEN 1 ELSE 1 END")


At this point, the code has been reduced to the following (in its entirety):

root = QgsProject.instance().layerTreeRoot()
policy_group = root.findGroup('Names')

formatstr  = '"{layerName}_Score" = {N}'
formatstrA = 'coalesce({layerName} = {N}, 0.00)*{N}'

for group in policy_group.children():
layers = [g.layerName() for g in group.children()]

formula_score4  = ' OR '.join(formatstr.format(layerName = layer, N = 4) for layer in layers]
formula_score3  = ' OR '.join(formatstr.format(layerName = layer, N = 3) for layer in layers]
formula_score2  = ' OR '.join(formatstr.format(layerName = layer, N = 2) for layer in layers]
formula_score1  = ' OR '.join(formatstr.format(layerName = layer, N = 1) for layer in layers]

formula_score3a = ' OR '.join(formatstrA.format(layerName = layer, N = 3) for layer in layers]
formula_score2a = ' OR '.join(formatstrA.format(layerName = layer, N = 2) for layer in layers]

formula = ("CASE WHEN " +
formula_score4 + " OR " + formula_score3a + ">=9 THEN 4 WHEN " +
formula_score3 + " OR " + formula_score2a + ">=6 THEN 3 WHEN " +
formula_score2 + "THEN 2 WHEN " +
formula_score1 + "THEN 1 ELSE 1 END")


Now we're getting somewhere. This is now very readable, and good enough to ship (in my opinion). However, with one final refactoring, we can eliminate 6 intermediate variables, and really tighten up the code...

## 7. DRY — Don't Repeat Yourself (using dict to reduce duplication)

While the formula_scoreX variables above are much improved, and the explicit loops eliminated, there's still a lot of repetition. Sure, just 2-4 copies of those list comprehensions isn't really so bad; however, it doesn't scale well if your business logic needs to expand to, say, 20 or 50 score groupings.

Let's use dict to map the score type (1, 2, 3, or 4) to the appropriate strings. We need 2 dictionaries (one for the "{layerName}_Score" strings, one for the 2 "coalesce" strings):

fscorestrs  = {k:" OR ".join(formatstr.format(layerName = layer, N = k) \
for layer in layers) for k in range(1,5)}
fscorestrsA = {k:" OR ".join(formatstrA.format(layerName = layer, N = k) \
for layer in layers) for k in range(2,4)}


There's a lot to unpack in those 2 statements, but they look worse than they are. Here's a simple dictionary creation (with list comprehension to help create it):

>>> d = {k:'string ' + str(k) for k in range(1,4)}
>>> for (key, value) in d.items():
...     print key, '=', value
...
1 = string 1
2 = string 2
3 = string 3


Notice that range(start, end) creates a list from start (inclusive) until end (not inclusive).

These two dictionaries now contain all your formula strings, indexed by "type" (i.e., 1, 2, 3, and 4). Also, these 2 lines are the only references to the format strings formatstr and formatstrA. In the final code below, I have eliminated the format string assignments, and just placed the format strings directly in the dictionary creation statements. It's probably a bit less readable that way, but in my mind it's worth the tradeoff in order to have locality of reference (keeping use close to definition and in the same scope).

## The final refactored code

root = QgsProject.instance().layerTreeRoot()
policy_group = root.findGroup('Names')
delim = ' OR '

for group in policy_group.children():
layers = [g.layerName() for g in group.children()]

fscorestrs  = {k:delim.join('"{layer}_Score" = {N}'.format(
layer = l, N = k) for l in layers) for k in range(1, 5)}
fscorestrsA = {k:delim.join('coalesce({layer} = {N}, 0.00)*{N}'.format(
layer = l, N = k) for l in layers) for k in range(2, 4)}

formula = ("CASE WHEN " +
fscorestrs[4] + " OR " + fscorestrsA[3] + " >=9 THEN 4 WHEN " +
fscorestrs[3] + " OR " + fscorestrsA[2] + " >=6 THEN 3 WHEN " +
fscorestrs[2] + " THEN 2 WHEN " +
fscorestrs[1] + " THEN 1 ELSE 1 END")

• You really went above and beyond the call of duty here, it is greatly appreciated. Thank you very much! Read through this a few times and got the "Ahh, that makes sense!" moments, will take this onboard and implement it :) – Joseph May 12 '16 at 9:37
• Wish I could accept both answers but I accepted the other answer. However, I will award you a bounty for providing a clear step-by-step tutorial =) – Joseph May 12 '16 at 14:30
• @Joseph That's very kind of you, but don't worry. I enjoy refactoring exercises, and I like practicing my technical writing. I hope there's something you can use, if not now, then in the future. – scottbb May 12 '16 at 14:41
• Actually I have used the code from JoeWallis and yourself! Not in the same function ofcourse but in different parts of the plugin so don't worry, it is being used =) – Joseph May 12 '16 at 14:49
• Edited your final code to take into account a couple of things I didn't put into the original question =) – Joseph May 13 '16 at 9:21

I was writing a review along the lines of what @scottbb has suggested when I noticed that the formula that your code produces didn't make sense:

CASE
WHEN "Layer_1a_Score" = 4 OR
"Layer_1b_Score" = 4 OR
"Layer_1c_Score" = 4 OR
"Layer_1d_Score" = 4 OR
"Layer_1e_Score" = 4 OR
"Layer_1f_Score" = 4 OR
coalesce(Layer_1a = 3, 0.00)*3 OR
coalesce(Layer_1b = 3, 0.00)*3 OR
coalesce(Layer_1c = 3, 0.00)*3 OR
coalesce(Layer_1d = 3, 0.00)*3 OR
coalesce(Layer_1e = 3, 0.00)*3 OR
coalesce(Layer_1f = 3, 0.00)*3 >=9 THEN 4
WHEN …


As you noted in a subsequent comment, what you had intended was:

CASE
WHEN "Layer_1a_Score" = 4 OR
"Layer_1b_Score" = 4 OR
"Layer_1c_Score" = 4 OR
"Layer_1d_Score" = 4 OR
"Layer_1e_Score" = 4 OR
"Layer_1f_Score" = 4 OR
coalesce(Layer_1a = 3, 0.00)*3 +
coalesce(Layer_1b = 3, 0.00)*3 +
coalesce(Layer_1c = 3, 0.00)*3 +
coalesce(Layer_1d = 3, 0.00)*3 +
coalesce(Layer_1e = 3, 0.00)*3 +
coalesce(Layer_1f = 3, 0.00)*3 >=9 THEN 4
WHEN …


I suspect that what you really intended was:

CASE
WHEN "Layer_1a_Score" = 4 OR
"Layer_1b_Score" = 4 OR
"Layer_1c_Score" = 4 OR
"Layer_1d_Score" = 4 OR
"Layer_1e_Score" = 4 OR
"Layer_1f_Score" = 4 OR
coalesce(Layer_1a_Score = 3, 0.00)*3 +
coalesce(Layer_1b_Score = 3, 0.00)*3 +
coalesce(Layer_1c_Score = 3, 0.00)*3 +
coalesce(Layer_1d_Score = 3, 0.00)*3 +
coalesce(Layer_1e_Score = 3, 0.00)*3 +
coalesce(Layer_1f_Score = 3, 0.00)*3 >=9 THEN 4
WHEN …


But that is just a convoluted way of saying "When any layer's score is 4, or if three of the layers' scores are 3, then the final score is 4".

You don't need to bother looking for scores that are 1, since that is the default case anyway.

The fundamental reason for the complexity of your expression is that you have given names to many numbers that should be considered interchangeable. In SQL, data that are meant to be "treated the same" should be stored in the same column, rather than each in its own column.

I assume you are using PostGIS, in which case you can use the handy UNNEST() PostgreSQL function to create a literal table. You can then use SUM(bool_expr::INT) to count the number of true values.

SELECT CASE
WHEN SUM((score = 4)::INT) > 0 OR SUM((score = 3)::INT) >= 3 THEN 4
WHEN SUM((score = 3)::INT) > 0 OR SUM((score = 2)::INT) >= 3 THEN 3
WHEN SUM((score = 2)::INT) > 0 THEN 2
ELSE 1
END
FROM UNNEST(ARRAY[
"Layer_1a_Score", "Layer_1b_Score", …, "Layer_1f_Score"
]) AS score


The italicized text is the only part of that expression that changes. It should be easy to construct that list of layer names using a ', '.join(layer.layerName() for layer in group.children()) in Python.

• Thanks for your answer, you are correct! At the moment, I am not using PostGIS although I will at some point soon. The sql filter is used for the QgsExpression class so not sure if the code will work (will test it). However, it will be useful when I transfer over to PostGIS so thank you again =) – Joseph May 13 '16 at 9:25
• Now that it has been clarified that this is a QGIS expression, I suggest that you write a custom Python function that can be incorporated into the expression. It would be less nasty doing the count in Python than to generate a nasty-looking CASE expression. – 200_success May 13 '16 at 16:50