4

For the same task, I have coded two different functions. I would like to know which one is more elegant to use.

The task is to check a pydot object if it beholds a requested node, and if so, to return the node and the graph object back. If the node doesn't exist, the node needs to be created.

To get the names of the nodes, I use the pydot objects get_nodes() function. However, if there aren't any nodes introduced yet, this function returns an empty list. Hence, before iterating over the values, I make an empty list check.

The first variant (’variant1') is easy to understand. After the length check, which is necessary because of node.get_name(), it loops over to node names and once the node that is being searched for is found, the node and the graph is returned. If not, it calls a function that creates the node and updates the graph. Although this function is easy to understand, IMHO it is not elegant. It beholds two ’return’ statements:

def variant1(node_name, graph):

    if len(graph.get_nodes()) > 0:

        for node in graph.get_nodes():
            if node_name == node.get_name():
                return node, graph

    return create_node(node_name, graph)

The second variant, is quite more complicated. Once the node is found in the graph, it breaks and jumps directly to the last row (’return node, graph’). This variant has only one return statement.

def variant2(node_name, graph):

    if len(graph.get_nodes()) > 0:

        for node in graph.get_nodes():
            if node_name == node.get_name():
                break

        else:
            # the node doesnt exist. create it and update the graph
            node, graph = create_node(node_name, graph)

    else:
        # create the first node in the graph
        node, graph = create_node(node_name, graph)

    return node, graph

My question is, which one should I prefer to use, according to the ’The Zen of Python’, ’PEP8’ or ’Google Python Style Guide’?

jonrsharpe
  • 115,751
  • 26
  • 228
  • 437
T-800
  • 1,543
  • 2
  • 17
  • 26
  • In the former I'd just `return create_node(node_name, graph)` - why assign for one line? – jonrsharpe Aug 13 '15 at 15:09
  • @jonrsharpe you're right! I'll update it. Thanks! – T-800 Aug 13 '15 at 15:11
  • Does `create_node` return a node and a graph as well? Ideally, your function will return a node/graph pair whether or not a new node is created. – chepner Aug 13 '15 at 15:18
  • I usually try to have as few return statements as possible, and have them as close to the bottom of the function as possible, without sacrificing readability. – Cyphase Aug 13 '15 at 15:22
  • _"The second variant, is quite more complicated."_ This says it all. – ivan_pozdeev Aug 13 '15 at 20:15

1 Answers1

9

Here's how I'd write it:

def variant1a(node_name, graph):
    """Include a docstring, too!"""
    for node in graph.get_nodes():
        if node.get_name() == node_name:
            return node, graph
    return create_node(node_name, graph)

This means:

  • the comparison operation is in a more readable order;
  • only one line calls create_node (DRY!);
  • although there are two return lines, both are at the bottom of the function;
  • no for: else: (which some people dislike - I think it's useful in general, but unnecessary here); and
  • no needless length checking (if len(graph.get_nodes) is zero the loop is skipped anyway).

PEP-8 doesn't prohibit multiple return lines (indeed, some of the examples in the style guide do have them). I couldn't see a reference to it in Google's style guide, but I didn't expand all of the sections!

jonrsharpe
  • 115,751
  • 26
  • 228
  • 437
  • Thanks for your response, but we need to check the length: if the `graph.get_nodes()` returns an empty list, you'll get error while getting the name of the node (`node.get_name()`) – T-800 Aug 13 '15 at 15:23
  • 1
    @T-1000 no, because if it returns an empty list **you never enter the loop**. Try it at home: `for _ in []: print("Did this appear?")` – jonrsharpe Aug 13 '15 at 15:24
  • you're right. I have somehow overseen it. I've updated the question and would like now to see what the general opinion is. – T-800 Aug 13 '15 at 15:37
  • 2
    @T-1000 it's pretty bad form to change the question after people have answered. – jonrsharpe Aug 13 '15 at 15:41