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.
Change the format strings (remove the trailing ' OR '
from them):
formatstr = '"{layerName}_Score" = {N}'
formatstrA = 'coalesce({layerName} = {N}, 0.00)*{N}'
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
Delete the formula2 = ...
, formula3 = ...
, formula4 = ...
, and final_formula
assignments. You no longer need the calls to .replace()
.
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")
final_formula
that you intend. Could you edit the question to include some sample output? \$\endgroup\$coalesce()
functions should be added togethercoalesce(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 ;) \$\endgroup\$coalesce(Layer_1a_SCORE = 2, 0.00)*2 + coalesce(Layer_1b_Score = 2, 0.00)*2 + ...
? \$\endgroup\$