Idiomatic way to return (potentially shadowed) variables which have been defined as part of a separate multiple variable return

Question

Normal Situation

When writing a function with a named return value, you can typically use naked returns (whether or not you should is a separate discussion). They might look something like the following:

func add(x, y int) (z int) {
    z = x + y
    return
}

return here meaning the same as return z

Problematic Situation

However with the abridged snippet below...

func loadModule(moduleName, fileRoot string) (module []byte) {
    if strings.HasSuffix(moduleName, ".md") {
        module, err := readConvertMDFile(fileRoot + "htdocs/md/" + moduleName)
        if err != nil {
            log.Println(err)
        }
        return module
    } else {
        module = []byte{}
        return
    }
}

(This snippet runs fine, and is my current solution to the problem)

... the compiler will complain that module is shadowed if instead of return module there is just return. This is because module was declared (a second time) along with err, which has to be declared as it doesn't exist yet in this scope.

Possible Solutions

1. Do as I have done and explicitly name the return variable. While this isn't a terrible solution, I feel as though there should be a way to arrange the code so that it runs as it should with a naked return. Others have commented that this explicit return leads to 'code smell'.

2. Add a var err error at the beginning and use a multiple assignment rather than declaration. Probably a better solution, but I would prefer to use implicit assignment where possible for the sake of consistency and to reduce unnecessary lines.

3. Use a temporary moduleT variable then assign module = moduleT... this just feels messy and redundant.

While I can get the compiled result I'm looking for, I'm hoping someone can suggest a clear, idiomatic way of writing this.

Answer 1

At the time I wrote the question, my function looked like the following:

(shown mostly to demonstrate its verbosity)

func loadModule(moduleName, fileRoot string) (module []byte) {
    if strings.HasSuffix(moduleName, ".md") {
        module, err := readConvertMDFile(fileRoot + "htdocs/md/" + moduleName)
        if err != nil {
            log.Println(err)
        }
        return module
    } else if strings.HasSuffix(moduleName, ".html") {
        module, err := ioutil.ReadFile(fileRoot + "htdocs/html/" + moduleName)
        if err != nil {
            log.Println(err)
        }
        return module
    } else if strings.HasSuffix(moduleName, ".js") {
        module, err := ioutil.ReadFile(fileRoot + "htdocs/js/" + moduleName)
        if err != nil {
            log.Println(err)
        }
        return module
    } else if strings.HasSuffix(moduleName, ".css") {
        module, err := ioutil.ReadFile(fileRoot + "htdocs/css/" + moduleName)
        if err != nil {
            log.Println(err)
        }
        return module
    } else {
        module = []byte{}
        return
    }
}

This uses my suggested solution 1. It has a lot of repeated code (I'm still something of a beginner). If I instead use suggested solution 2 (but not in the way I originally thought of it) by putting var err error at the top of then function, the code is improved in two ways:

func loadModule(moduleName, fileRoot string) (module []byte) {
    var err error
    switch {
    case strings.HasSuffix(moduleName, ".md"):
        module, err = readConvertMDFile(fileRoot + "htdocs/md/" + moduleName)
    case strings.HasSuffix(moduleName, ".html"):
        module, err = ioutil.ReadFile(fileRoot + "htdocs/html/" + moduleName)
    case strings.HasSuffix(moduleName, ".js"):
        module, err = ioutil.ReadFile(fileRoot + "htdocs/js/" + moduleName)
    case strings.HasSuffix(moduleName, ".css"):
        module, err = ioutil.ReadFile(fileRoot + "htdocs/css/" + moduleName)
    default:
        module = []byte{}
    }
    if err != nil {
            log.Println(err)
        }
    return
}

There are no shadowed variables any more, and both the error logging and the return can be moved out of each if statement, resulting in much clearer code.

There may well be a way to improve on this. EDIT: ...and there is, following @ANisus' suggestion, the if-else chain has been replaced with a switch statement.

Answer 2

I always use your suggested solution 2 - add an extra var statement.

func loadModule(moduleName, fileRoot string) (module []byte) {
    var err error
    if strings.HasSuffix(moduleName, ".md") {
        module, err = readConvertMDFile(fileRoot + "htdocs/md/" + moduleName)
        if err != nil {
            log.Println(err)
        }
        return
    } else {
        // no need for this as module will be nil if it isn't written to
        // a nil slice is perfectly legal and has len()=0
        // and can be appended to etc
        // module = []byte{}
        return
    }
}

Option 2 is the most efficient solution too. Remember that go returns all values on the stack so a named return value is equivalent to a stack allocated variable.

If in option 1 or option 3 you don't have a naked return, then there is an implicit module = module or module = moduleT statement in there anyway.

Unfortunately variable shadowing is something which bites every Go programmer after a while. I'd quite like the compiler to disallow all shadowing within a function as it is a source of real bugs.