Skip to content
This repository was archived by the owner on Oct 19, 2023. It is now read-only.

Conversation

@jonboulle
Copy link
Contributor

h/t @runcom

@runcom
Copy link

runcom commented Jun 17, 2016

👍

@jonboulle
Copy link
Contributor Author

took the liberty of fixing travis too

main.go Outdated
if err != nil {
return nil, err
}
defer f.Close()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for this to be a defer. Save the results of ReadAll, close, then return.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't it the same but w/o having to store the result of ReadAll and check for the error? meh though it's less code like this

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also you need to make sure to Close on ReadAll failure as well no?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

b, err := ioutil.ReadAll(f)
if err != nil {
    f.Close()
   return nil, err
}
f.Close()
return b, nil

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defer is a slow operation, and so shouldn't be used if there's only one return path. There's no difference in the return values if ReadAll returns an error, so the extra logic you have above doesn't add any value.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand, if readall fails you still have to close the file

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

b, err := ioutil.ReadAll(f)
f.Close()
return b, err

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok i see nvm

@jonboulle
Copy link
Contributor Author

@mjibson updated

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants