Programming Practices – Is It Good Practice to Always Have Methods Return a Value?

coding-standardscoding-stylecqrsfunctionsprogramming practices

Sorry for the terrible title but hopefully these snippets will give you the gist.

Method 1:

class Person:
    def __init__(self, name):
        self.name = name

    def set_name(self, new_name):
        self.name = ' '.join(s[0].upper() + s[1:] for s in new_name.split(' '))


def __main__():
    me = Person('Foo')
    me.set_name('foo bar')

Method 2:

class Person:
    def __init__(self, name):
        self.name = name

    def set_name(self, new_name):
        return ' '.join(s[0].upper() + s[1:] for s in new_name.split(' '))


def __main__():
    me = Person('Foo')
    me.name = me.set_name('foo bar')

In the relatively simple example, both methods should achieve the same result. My question is which method would be considered better practice? Is there a readability or performance reason or is it simply personal preference?

Best Answer

One of the first languages I learned was Pascal. It had an important distinction that was made. There were procedures, that had no return, and functions that always had a return type.

These are two separate constructs that do separate things.

Functions return values. Always.

Now that that is out of the way, a method (be it function-esque or procedure-esque) should always return something that is meaningful if it is to return anything at all. Returning values that are discarded 99% or 100% of the time is not useful - its a waste. A good compiler could recognize this and may make its minimal if any return cost nothing... but the waste is in human time for the person reading the code. What is it returning? Why is it returning?

Many static analysis tools will (properly) warn the programmer about ignoring the return value of a function call. For example FindBugs

This method ignores the return value of one of the variants of java.io.InputStream.read() which can return multiple bytes. If the return value is not checked, the caller will not be able to correctly handle the case where fewer bytes were read than the caller requested. This is a particularly insidious kind of bug, because in many programs, reads from input streams usually do read the full amount of data requested, causing the program to fail only sporadically.

Or the warnings in g++ (#534)

Ignoring return value of function 'Symbol' (compare with Location)

A function that returns a value is called just for side effects as, for example, in a statement by itself or the left-hand side of a comma operator. Try: (void) function( ); to call a function and ignore its return value. See also the fvr, fvo and fdr flags.

And thus, there is the expectation that you should be looking at return values that are returned. If it has no meaning, it shouldn't be in the first place. If it is an exceptional failure, well, that's what exceptions are for (in many languages).

Now, lets look at something about this code:

class Person:
    def __init__(self, name):
        self.name = name

    def set_name(self, new_name):
        return ' '.join(s[0].upper() + s[1:] for s in new_name.split(' '))

def __main__():
    me = Person('Foo')
    me.name = me.set_name('foo bar')

See those lines I called out? They aren't doing what they say there're doing at all.

The function named set isn't setting. Its formatting, but it isn't setting.

Sure, writing a format_name isn't a bad method to write. And it would be a function - because it returns something. And a (pardon my Java) public void setName(String name) has a void return type - because its not useful to anyone. It clearly communicates its intent - "I am setting this value and its done. Don't worry about it."

Methods should only have useful return values.