Idiomatic Go code

[Christian von Kietzell]

Hello Gophers,

since I obviously don't have enough Twitter followers to get more than
one answer to my Go question I'm posting it here ;-)

Would you consider this idiomatic Go code?

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

I'm talking specifically about returning a cleanup function. My
reasoning for it goes kinda like this:

- doSomething doesn't care whether the output is a file or a buffer or
os.Stdout, so it should just get an io.Writer.
- The cleanup work for using os.Stdout (or a buffer in tests) is
different than for *os.File.
- Who's responsible for doing that work? If I return *os.File directly
from getFile(), there's an implicit assumption that the file needs to be
closed by the caller. But what if it's os.Stdout?
- The alternative of putting all that in run() is kinda ugly because
run() potentially has to do a lot more.

A potential problem I see? The error from os.File.Close() is never
checked, neither is the error from bufio.Writer.Flush(). In this small
example that wouldn't bother me because it means the program ends
anyway. But in other cases it might become more of a problem.

What do you think?

Kind regards,
Chris

[burak serdar]

The problems you noted aside, this is similar to context.WithCancel()
that returns a cancel() func. It is a pattern I've seen and used in
the field. In my opinion, it is idiomatic Go code.

>
> What do you think?
>
> Kind regards,
> Chris
>

> --
> 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.
> To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/7baf0ec35696fc5501dfbe6ce8f161f8%40vonkietzell.de.

[Axel Wagner]

I don't think it's inherently *un*idiomatic to return a cleanup function. But in this case, it seems unnecessarily complicated (and buggy).

On Thu, Mar 4, 2021 at 2:19 PM Christian von Kietzell <chri...@vonkietzell.de> wrote:

- doSomething doesn't care whether the output is a file or a buffer or
os.Stdout, so it should just get an io.Writer.

I definitely agree with this.

 

- The cleanup work for using os.Stdout (or a buffer in tests) is
different than for *os.File.

- Who's responsible for doing that work? If I return *os.File directly
from getFile(), there's an implicit assumption that the file needs to be
closed by the caller. But what if it's os.Stdout?

One possibility that we need to mention is that it would be possible to return a new file, connected to stdout, for example by using syscall.Dup. You could also wrap `os.Stdout` in a new type, with a no-op `Close` method.

 

- The alternative of putting all that in run() is kinda ugly because
run() potentially has to do a lot more.

Nevertheless, personally, I think it's the right choice, ultimately.

A potential problem I see? The error from os.File.Close() is never
checked, neither is the error from bufio.Writer.Flush(). In this small
example that wouldn't bother me because it means the program ends
anyway. But in other cases it might become more of a problem.

Yes, at the very least, your cleanup function should return an `error`. That `error` might just be statically `nil`, but the failure when flushing/closing (FTR, I don't think you need to flush, if you close the file anyway) should definitely be handled some way. That's why I think your code as-is is actually buggy. "The program will end anyway" isn't a good reason to silently leave a corrupt file behind.

 

So, I would probably be fine accepting this in a code-review (as long as the error is checked) but I would prefer to just inline the function into `run`.

What do you think?

Kind regards,
Chris

[Christian von Kietzell]

Hi,

thank you for the helpful comments. That's what I love about the Go community.

On Thu, Mar 04, 2021 at 03:14:04PM +0100, 'Axel Wagner' via golang-nuts wrote:
> - Who's responsible for doing that work? If I return *os.File directly
> from getFile(), there's an implicit assumption that the file needs to be
> closed by the caller. But what if it's os.Stdout?
>
>
> One possibility that we need to mention is that it would be possible to return
> a new file, connected to stdout, for example by using syscall.Dup. You could
> also wrap `os.Stdout` in a new type, with a no-op `Close` method.

I actually hadn't thought of that. So instead of a Writer and a cleanup function
I could just as easily return an io.WriteCloser. This would make it explicit,
that the caller is supposed to call Close. A custom type (wrapping the actual
writer) with a Close method can then just return nil in case there's nothing to
be done.

> - The alternative of putting all that in run() is kinda ugly because
> run() potentially has to do a lot more.
>
> Nevertheless, personally, I think it's the right choice, ultimately.

In small programs I'd actually do that. But in the real case that inspired this
small example run() has to do a lot of setup work and I'd rather like to keep it
clean by encapsulating the various steps in separate functions.

> A potential problem I see? The error from os.File.Close() is never
> checked, neither is the error from bufio.Writer.Flush(). In this small
> example that wouldn't bother me because it means the program ends
> anyway. But in other cases it might become more of a problem.
>
>
> Yes, at the very least, your cleanup function should return an `error`. That
> `error` might just be statically `nil`, but the failure when flushing/closing
> (FTR, I don't think you need to flush, if you close the file anyway) should
> definitely be handled some way. That's why I think your code as-is is actually
> buggy. "The program will end anyway" isn't a good reason to silently leave a
> corrupt file behind.

I agree. By using a wrapping type and returning an io.WriteCloser that's
actually possible. Side note about flushing: I'm flushing the wrapping
bufio.Writer there. I'd be surprised closing the underlying os.File would be
enough to ensure all buffer contents have been written.


Thanks for the feedback.


Chris

[Axel Wagner]

On Thu, Mar 4, 2021 at 4:28 PM Christian von Kietzell <chri...@vonkietzell.de> wrote:

Side note about flushing: I'm flushing the wrapping
bufio.Writer there. I'd be surprised closing the underlying os.File would be
enough to ensure all buffer contents have been written.

True, I didn't read very carefully :)

 

Thanks for the feedback.


Chris

--
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.

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/YED8ezxQZPxB6hls%40junction.dune.