Is it considered good practice to always have methods return a value?

Question

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?

Answer 1

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.

Answer 2

me.name = me.set_name('foo bar')

Don't do this.

If you want to know what is better practice, just read through your examples. The first:

1. Make a person, with name of Foo
2. on that person, set the name to 'foo bar'

Now with the second:

1. Make a person, with name of Foo
2. Set the person's name to a method setting the person's name to 'foo bar'
   • It's a really bad idea to use "set" as part of your method here because it makes your examples impossible to process, since it more is a "format name"

The point of all this is that the problem here is not about the "does a method return?" or not question, it's about "does what you are doing make any sense at all." A void/no return function makes complete sense if the function doesn't return anything.

So you need to either make the set_name method differently named, so it makes sense, or get rid of it. Right now the reason you have a problem is that set_name is a really poor function.

Your examples would be better if either:

• Your __init__ formatted the name when setting it
• You didn't allow a name to be passed to the constructor and the current set_name actually set the formatted name

An example like:

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

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

def __main__():
    me = Person('Foo')

makes sense. I would question why you allow both setting a name and then requiring someone to specifically format the name. You might also do:

class Person:
    def __init__(self):
        pass

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

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

Answer 3

Method 2 is abhorrent. You are making the user do extra work for no real purpose. As others have noted, you are not setting the name; you are formatting it. At the very least, it should be a class method:

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

    @classmethod
    def format_name(new_name):
        return ' '.join(s[0].upper() + s[1:] for s in new_name.split(' '))

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

But even so, the problem here is that .name is an entirely public property. Why bother if you are forcing users to know about name formatting.

For getters/setters, if your issue is that you think not returning a value is ugly, then consider returning self. This allows chaining:

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

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

        return self

    def set_age(self, age):
        self.age = age

        return self

def __main__():
    me = Person('Foo', 50)
    me.set_name('foo bar').set_age(42)