0

What is the best way to express this code? (It was difficult to phrase in words)

if cond1 or cond2:
    # shared code
    if cond1:
        # cond1 code
    else:
        # cond2 code

The above obviously duplicates the check

if cond1:
    # shared
    # cond1
elif cond2:
    # shared
    # cond2

This repeats the shared code

I am writing an implementation of Dijkstra's, the question arises within the relax function

def dijkstra(s):
def relax(w, v, edge_cost):
    w_via_v = distance[v] + edge_cost
    if w not in distance:
        distance[w] = w_via_v
        toexplore.push(w)
    elif w_via_v < distance[w]:
        distance[w] = w_via_v
        toexplore.update(w)

distance = {s: 0}
toexplore = PriorityQueue([s], sortkey=lambda v: distance[v])

while not toexplore.is_empty():
    v = toexplore.popmin()
    # Assert: v.distance is distance(s to v)
    # Assert: thus, v is never put back into toexplore
    for w, edge_cost in v.neighbours:
        relax(w, v, edge_cost)

Another alternative is

def relax(w, v, edge_cost):
    if w not in distance:
        distance[w] = ∞
        toexplore.push(w)
    w_via_v = distance[v] + edge_cost
    if w_via_v < distance[w]:
        distance[w] = w_via_v
        toexplore.update(w)

But this unnecessarily pushes a node into the priority queue only to update it again

hl5619
  • 9

5 Answers5

1

There is no singular best way to write such an if-elif statement where some of the paths have partially shared code. It depends too much on the specifics of the code you are writing.

What can be said in general is that your first priority is to write code that can be easily understood. After that you can start looking at principles like DRY and, for example, refactor duplicated blocks of code into a function.

The reasoning behind DRY is that when you have to make a change/correction to duplicated code, you have to make the change in N spots and you probably forget K of them. But that reasoning only holds if the process of removing duplicates does not introduce extra complexities to the code that make it harder to understand and maintain.

In your specific example of the relax function, your first version with the duplicated distance[w] = w_via_v is much easier to understand and check against the official description of the algorithm. The duplicates of that line are also close enough together that you instantly see all instances that need to be updated if/when a change is needed.
Even though there is a duplication in it, that version is way better than the one where you tried to remove the duplication.

1

It depends on context and what makes the most sense reading.

In general, for most use cases I can think of, I would favor:

if cond1 or cond2
  shared

if cond1 logic1

if cond2 logic2

Because every unique if condition (i.e. the whole condition that is part of that if) tends to be the meaningfully readable indication of why you want to hit that block.

Additionally, it's usually better to avoid repetition with code blocks instead of evaluations (if you have to choose and can't do both), because code blocks are lengthier to write and more likely to introduce bugs by forgetting to alter each copy/pasted version when making changes.

Flater
  • 58,824
0

In the general case I would favor:

if cond1:
    # shared
    # cond1 code
elif cond2:
    # shared
    # cond2 code

Simply because the logic is easier to follow - it's obvious what conditions need to be true to execute each block.

You are likely asking the question, because you are worried about the duplicated shared code. In this specific case, it's a one line assignment, as such I wouldn't worry about the duplication - it is unlikely it is going to confuse anyone.

If the shared code was large/larger I would consider pulling it out into a method. That may or may not be a practical option, if pulling out a method isn't practical (results in a mess) then frankly my best advice is to try writing the code a couple of different ways and pick the one you think is more readable.

DavidT
  • 4,601
0

What it is described in the description it is a use case suitable for a flavour of strategy design pattern. Define a strategy with two methods (1) a method that returns true for the case the implementation supports and (2) a method that implements the behaviour to run in case first method returns true and with two implementations (1) an implementations with the first method returning true for w not in distance and the second method doing:

distance[w] = w_via_v
toexplore.push(w)

(2) a second implementation with the first method returning true for w_via_v < distance[w] and the second method doing:

distance[w] = w_via_v
toexplore.update(w)

then pass distance[w] first method of each strategy implementation and call the second method is first method returns true.

-1

It's always a good idea to avoid repeating identical blocks of code.

  • To avoid repeating the computation for cond1 and cond2, you can always compute both conditions once, assign them to flags and then check the flags repeatedly for no extra cost.
  • And if the code in shared is nontrivial, you should probably move it into a subroutine so that the duplication is minimized.
Kilian Foth
  • 110,899