golint if/else stmt and early returns

[mhh...@gmail.com]

Hi,

golint will report

if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (golint)   

for this code,

// Load returns the list of partition found and their properties.
func (l *LinuxLoader) Load() ([]*Properties, error) {
    //-
    ret := []*Properties{}

    if temp, err := runDf(); err != nil {
        return ret, err
    } else {
        ret = PropertiesList(ret).Append(PropertiesList(temp))
    }
    //-
    if temp, err := runLsLabel(); err != nil {
        return ret, err
    } else {
        ret = PropertiesList(ret).Append(PropertiesList(temp))
    }
    //-
    if temp, err := runLsUsb(); err != nil {
        return ret, err
    } else {
        ret = PropertiesList(ret).Merge(PropertiesList(temp), "IsRemovable")
    }
    //-
    if temp, err := runMount(); err != nil {
        return ret, err
    } else {
        ret = PropertiesList(ret).Merge(PropertiesList(temp), "Label")
    }
    //-
    return ret, nil
}

Does it mean i should nest those stmts and let it be 4 level deep ?
Is it the reco ?

Is there something wrong about early returns ?

thanks

[Ian Davis]

It's saying you don't need the else clauses since you have returned in the if clause.

--

You received this message because you are subscribed to the Google Groups "golang-nuts" group.

To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Paweł Kopiczko]

Early returns are fine. You don't need all those else blocks.

// Load returns the list of partition found and their properties.
func (l *LinuxLoader) Load() ([]*Properties, error) {
//-
ret := []*Properties{}

if temp, err := runDf(); err != nil {
return ret, err
}

ret = PropertiesList(ret).Append(PropertiesList(temp))
//-
if temp, err := runLsLabel(); err != nil {
return ret, err
}

ret = PropertiesList(ret).Append(PropertiesList(temp))
//-
if temp, err := runLsUsb(); err != nil {
return ret, err
}

ret = PropertiesList(ret).Merge(PropertiesList(temp), "IsRemovable")
//-
if temp, err := runMount(); err != nil {
return ret, err
}

ret = PropertiesList(ret).Merge(PropertiesList(temp), "Label")
//-
return ret, nil
}

[roger peppe]

I'd be inclined to write it something like this:

https://play.golang.org/p/_CRQ86vHfq

[mhh...@gmail.com]

I d like to, but when I write,

// Load returns the list of partition found and their properties.
func (l *LinuxLoader) Load() ([]*Properties, error) {
    //-
    ret := []*Properties{}

    if temp, err := runDf(); err != nil {
        return ret, err
    }
    ret = PropertiesList(ret).Append(PropertiesList(temp))

...

# github.com/mh-cbon/disksinfo/diskinfo
/tmp/go-build658543611/github.com/mh-cbon/disksinfo/diskinfo/_test/_obj_test/linux.go:25: undefined: temp

FAIL github.com/mh-cbon/disksinfo/diskinfo [build failed]

:X

Otherwise i have to work with shadowed variable.

[mhh...@gmail.com]

yeah, i did write it like it before,
but i want stop to shadow my variables as much as possible,
i found mistakes happens too easily with that writing.

[Nigel Tao]

This is tangential, but if we're talking about style, you might be
able to simplify this line
ret = PropertiesList(ret).Append(PropertiesList(temp))
to be
ret = PropertiesList(ret).Append(temp)
if the PropertiesList underlying type and the temp variable's type are
what I'm guessing they are: []*Properties. The relevant sections of
the spec are https://golang.org/ref/spec#Calls and the link to
"assignable".

In a similar fashion, you could probably further simplify to:
ret = ret.Append(temp)
if you replaced
ret := []*Properties{}
with either
ret := PropertiesList(nil)
or
var ret PropertiesList

[jmont...@dyn.com]

While there may be better ways to express what this logic, for clarity, here is the change that golint is actually suggesting:

// Load returns the list of partition found and their properties.
func (l *LinuxLoader) Load() ([]*Properties, error) {
    //-
    ret := []*Properties{}

    if temp, err := runDf(); err != nil {
        return ret, err
    }

    ret = PropertiesList(ret).Append(PropertiesList(temp))
    //-
    if temp, err := runLsLabel(); err != nil {
        return ret, err
    } else {
        ret = PropertiesList(ret).Append(PropertiesList(temp))
    }
    //-
    if temp, err := runLsUsb(); err != nil {
        return ret, err
    } else {
        ret = PropertiesList(ret).Merge(PropertiesList(temp), "IsRemovable")
    }
    //-
    if temp, err := runMount(); err != nil {
        return ret, err
    } else {
        ret = PropertiesList(ret).Merge(PropertiesList(temp), "Label")
    }
    //-
    return ret, nil
}

The change is at line 9.

[mhh...@gmail.com]

Indeed, thanks!