Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Golang Version #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ddbullfrog
Copy link

Add Golang Version

@ocram
Copy link
Contributor

ocram commented Jul 14, 2017

Thanks!

There has already been an existing pull request for a Go implementation here: #19

There are a few minor differences, though. Could you explain them perhaps?

For example, why does your code not make use of a Codec which is suggested in the other implementation? Is it not necessary? And why do you buffer in a byte array first and only convert to a string at the end? Is this better for performance? Or just a personal preference?

@bgadrian
Copy link

If I can respond, as a Go newbie myself:

  • it is Not optimal to use String = string + string (like in any language), the go docs says to use a bytes buffer or in the recent version they added a Strings Builder. None of the pull requests use these methods.

  • encoders/decoders, from what I can see what is Go idiomatic, in the builtin libraries they have 2 interfaces (Encode and decode, marshall and unmarshall), and they work with bytes), everything is a []byte. https://golang.org/pkg/encoding/#pkg-overview

With my limited knowledge I would say that none of these PR are idiomatic Go.

@ocram
Copy link
Contributor

ocram commented Jun 14, 2018

Thanks, @bgadrian!

We now have a third candidate for the Go implementation: #32

@ocram
Copy link
Contributor

ocram commented Jun 14, 2018

This one here is currently the shortest and simplest implementation, which is great.

It does not use a string builder, but it does not simply concatenate strings, either. Instead, it uses a byte array that acts as a buffer. Shouldn’t this be fine, @bgadrian?

@ocram ocram mentioned this pull request Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants