Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I want to simplify an if: .. else: .. construct:

def find_eulerian_tour(graph):
    available_edges, path = initial_state(graph)
    while incomplete(available_edges):
        edge = propose_extension_edge(available_edges, path)
        if extension_is_possible(edge):
            extend_path(available_edges, path, edge)
        else: 
            node = propose_node_for_reorder(available_edges, path)
            if reorder_is_sensible(node):
                path = reorder_path(available_edges, path, node)
            else:
                return None
    return path

It seems unnecessarily complicated, because the idea can be expressed much simpler.

def find_eulerian_tour(graph):
    available_edges, path = initial_state(graph)
    while incomplete(available_edges):
        if extension_is_possible(available_edges, path):
            extend_path(available_edges, path)
        elif reorder_is_sensible(available_edges, path):
            path = reorder_path(available_edges, path)
        else:
            return None
    return path

But now I have to do the calculations from

edge = propose_extension_edge(available_edges, path)

and

node = propose_node_for_reorder(available_edges, path) 

twice because I can't keep the results in variables (edge = .., node = ..).

I am looking for an idea to keep code as simple as it is with the elif construction while doing the propose_node..-calculation only once.

def find_eulerian_tour(graph):
    available_edges, path = initial_state(graph)
    while incomplete(available_edges):
        edge = propose_extension_edge(available_edges, path)
        if extension_is_possible(edge):
            extend_path(available_edges, path, edge)
        else: 
            node = propose_node_for_reorder(available_edges, path)
            if reorder_is_sensible(node):
                path = reorder_path(available_edges, path, node)
            else:
                return None
    return path

def initial_state(graph):
    available, path = graph[:], []
    if len(available) > 0:
        path.append(available[0][0])
    return available, path

def incomplete(available_edges):
    return len(available_edges) > 0

def propose_extension_edge(available_edges, path):
    last_node = path[-1]
    for edge in available_edges:
        if last_node in edge:
            return edge
    return None

def extension_is_possible(edge):
    return edge

def extend_path(available_edges, path, edge):
    if edge[0] == path[-1]:
        path.append(edge[1])
    else:
        path.append(edge[0])
    available_edges.remove(edge)

def propose_node_for_reorder(available_edges, path):
    if reorder_is_possible(path):
        return find_split(available_edges, path)
    return None

def reorder_is_possible(path):
    return is_tour(path)

def find_split(available_edges, path):
    options = edgenodes(available_edges)
    for node in options:
        if node in path:
            return node
    return None

def reorder_is_sensible(node):
    return node

def is_tour(path):
    return path[0] == path[-1]

def edgenodes(edges):
    result = []
    for edge in edges:
        for node in edge:
            if not node in result:
                result.append(node)
    return result

def reorder_path(available_edges, tour, split):
    split_location = tour.index(split)
    return tour[split_location:-1] + tour[0:split_location+1]

Example use:

graph1 = [(1, 2), (2, 3), (3, 1)]
graph2 = [(0, 1), (1, 5), (1, 7), (4, 5), (4, 8), (1, 6), (3, 7), (5, 9), (2, 4), (0, 4), (2, 5), (3, 6), (8, 9)]
graph3 = [(1, 13), (1, 6), (6, 11), (3, 13), (8, 13), (0, 6), (8, 9),(5, 9), (2, 6), (6, 10), (7, 9), (1, 12), (4, 12), (5, 14), (0, 1), (2, 3), (4, 11), (6, 9), (7, 14), (10, 13)]

print find_eulerian_tour(graph1)
print find_eulerian_tour(graph2)
print find_eulerian_tour(graph3)
share|improve this question
1  
I rarely use python. I do a course on algorithms with exercises in python. And I see this as chance to improve the readability of my python code by the way. –  ovhaag Jun 10 at 23:53
add comment

2 Answers

def find_eulerian_tour(graph):
    available_edges, path = initial_state(graph)
    while incomplete(available_edges):
        edge = propose_extension_edge(available_edges, path)
        if extension_is_possible(edge):
            extend_path(available_edges, path, edge)
        else: 
            node = propose_node_for_reorder(available_edges, path)
            if reorder_is_sensible(node):
                path = reorder_path(available_edges, path, node)
            else:
                return None
    return path

Since you are inside a while block, an if-else can be turned into if->continue. The inner if-else can be turned into if not->return:

def find_eulerian_tour(graph):
    available_edges, path = initial_state(graph)
    while incomplete(available_edges):
        edge = propose_extension_edge(available_edges, path)
        if extension_is_possible(edge):
            extend_path(available_edges, path, edge)
            continue
        node = propose_node_for_reorder(available_edges, path)
        if not reorder_is_sensible(node):
            return None
        path = reorder_path(available_edges, path, node)
    return path

Now it reads like a checklist:

  • Extension possible? Do that and continue.
  • Reordering not sensible? Quit.
  • Reorder.

Is it an improvement? On the original: I think so. Nested logic is generally more difficult to follow. On the if-elif-else? Not really, but that's the price to pay for caching the intermediary values.

share|improve this answer
    
It is an improvement on the original. Thanks –  ovhaag Jun 11 at 7:54
    
The if not idea is good. I think I can expand it. Bad luck, I must make a break now. I am back in ca. 3 hours and will then check if it works out. –  ovhaag Jun 11 at 8:09
add comment

With the if not proposed by otus and with the shortcut logic of or I can write

def find_eulerian_tour(graph):
    available_edges, path = initial_state(graph)
    while incomplete(available_edges):
        if not (extension_success(available_edges, path) or
                reorder_success(available_edges, path)):
            return None
    return path

The .._success functions are new. They calculate a proposal and, if this is successful, they mutate the status, i.e. path and available_edges. I am aware that mutation can be difficult to follow too. But as I already use mutation in the extend.. case, it is only consistent to also use mutation in the reorder.. case.

At least at this moment I am happy with this refactoring.

def find_eulerian_tour(graph):
    available_edges, path = initial_state(graph)
    while incomplete(available_edges):
        if not (extension_success(available_edges, path) or
                reorder_success(available_edges, path)):
            return None
    return path

def initial_state(graph):
    available, path = graph[:], []
    if len(available) > 0:
        path.append(available[0][0])
    return available, path

def incomplete(available_edges):
    return len(available_edges) > 0

def extension_success(available_edges, path):
    edge = propose_extension_edge(available_edges, path)
    if not extension_is_possible(edge): return False
    return extend_path(available_edges, path, edge)

def propose_extension_edge(available_edges, path):
    last_node = path[-1]
    for edge in available_edges:
        if last_node in edge:
            return edge
    return None

def extension_is_possible(edge):
    return edge

def extend_path(available_edges, path, edge):
    if edge[0] == path[-1]:
        path.append(edge[1])
    else:
        path.append(edge[0])
    available_edges.remove(edge)
    return True

def reorder_success(available_edges, path):
    node = propose_node_for_reorder(available_edges, path)
    if not reorder_is_sensible(node): return False
    path[:] = reorder_path(available_edges, path, node)
    return True

def propose_node_for_reorder(available_edges, path):
    if reorder_is_possible(path):
        return find_split(available_edges, path)
    return None

def reorder_is_possible(path):
    return is_tour(path)

def find_split(available_edges, path):
    options = edgenodes(available_edges)
    for node in options:
        if node in path:
            return node
    return None

def reorder_is_sensible(node):
    return node

def is_tour(path):
    return path[0] == path[-1]

def edgenodes(edges):
    result = []
    for edge in edges:
        for node in edge:
            if not node in result:
                result.append(node)
    return result

def reorder_path(available_edges, tour, split):
    split_location = tour.index(split)
    return tour[split_location:-1] + tour[0:split_location+1]
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.