Go beginner asks for code review

[Leandro Motta Barros]

Hello,

I hope this is not considered off-topic: I have started learning Go a few weeks ago, and have recently finished writing my first sizable piece of Go code.

I wonder if anyone would be willing to take a look at it, and give some advice on how to be more idiomatic or improve it somehow. Any feedback is welcome! :-)

The code is a hand-written parser for a subset of Markdown, and can be found here: https://github.com/lmbarros/sbxs_go_markydown

Function Parse() on parser.go ( https://github.com/lmbarros/sbxs_go_markydown/blob/master/markydown/parser.go#L7 ) is the main entry point for this code.

Thanks a lot!

LMB

[Sam Whited]

On Thu, Sep 22, 2016 at 10:48 AM, Leandro Motta Barros
<l...@stackedboxes.org> wrote:
> I hope this is not considered off-topic: I have started learning Go a few
> weeks ago, and have recently finished writing my first sizable piece of Go
> code.

Not at all; welcome to the Go community, and congrats on your first
big project! I'm sure you'll get lots of feedback, and please,
continue to ask questions and avail yourself of this list, it's a
great resource if you're just starting out.

Best,
Sam


--
Sam Whited
pub 4096R/54083AE104EA7AD3

[Sam Whited]

On Thu, Sep 22, 2016 at 10:48 AM, Leandro Motta Barros <l...@stackedboxes.org> wrote:

> I wonder if anyone would be willing to take a look at it, and give some
> advice on how to be more idiomatic or improve it somehow.

Not sure how to do this on GitHub if it's not a PR, I don't appear to be able to leave line-level comments on the source, so I've inlined it. Sorry for all the missing context. That being said, here are a few things I noticed:

> parser := &parser{}
> parser.input = document
> parser.processor = processor
> parser.frag = parser.input
> parser.fragEnd = 0
> parser.textStyle = TextStyleRegular
> parser.linkTarget = ""
> parser.linkTargetLen = 0

This could be much more cleanly written with all the fields in the struct literal, eg. the idiomatic way to write this would be:

> parser := &parser{
>    input: document,
>    processor: processor,
>    frag: document,
>    textStyle: TextStyleRegular,
> }

Also, don't forget that in Go all structs are initialized, so eg. linkTargetLen and linkTarget which are 0 and the empty string respectively dont need to be initialized at all since the zero value of an numeric type is 0 and the zero value for a string is "".

Finally, be careful, doing parser := &parser{} is shadowing the struct parser which may (or may not) be what you want.

> p.processor.OnStartDocument()

This is of course completely up to you, but it's idiomatic to just call something like this "StartDocument", the "On" is normally elided.

> github.com/lmbarros/sbxs_go_markydown/markydown

The package would normally just live at the repo root in go; unless you're expecting other subpackages to live in this repo. Right now I'd have to:

    import "github.com/lmbarros/sbxs_go_markydown/markydown"

but it might read better if I could just import:

    import "github.com/lmbarros/go_markydown"

(you can also leave the go_ off if this will be the only thing called markydown on your GitHub account)

if len(p.input) == 0 {
return false
}
// Try parsing each of the "special" paragraph types.
if p.parseHeading() {
return true
}
if p.parseBulletedParagraph() {
return true
}

some people prefer to write long chains of I

[Leandro Motta Barros]

Hi Sam,

Looks like your response got truncated. :-/

Anyway, there is a good deal of nice tips, there. I am updating my code to take your feedback into account. Thanks a lot!

There is one point I still wondering about, however:

On Thu, Sep 22, 2016 at 1:57 PM, Sam Whited <s...@samwhited.com> wrote:

> github.com/lmbarros/sbxs_go_markydown/markydown

The package would normally just live at the repo root in go; unless you're expecting other subpackages to live in this repo. Right now I'd have to:

    import "github.com/lmbarros/sbxs_go_markydown/markydown"

but it might read better if I could just import:

    import "github.com/lmbarros/go_markydown"

(you can also leave the go_ off if this will be the only thing called markydown on your GitHub account)

Yes, I think the directory structure I used sucks, and I'd like to improve it. I'll tell why did so, and would be glad to have some feedback about it:

1. The sbxs_ prefix is there just because I expect this package to be part of a set of related repositories and I wanted to give them some "unity" through naming. Arguably a bad idea, I am not sure about it.

2. I added "go" to the package name because I tend to experiment with different languages from time to time, and therefore I have a reasonable chance to have naming conflicts without it.

3. I added that extra "markydown" directory just because I read on Effective Go that it is a convention to give the package name the same name as the directory, and I thought that "markydown" was a better package name than "sbxs_go_markydown". Maybe I should just use the long package name and let users rename it when importing if they wish? Or is there  some better way?

Thanks again!

LMB

[Chris Manghane]

To this point, it's probably better not expect users to rename your package when importing it. Import paths can be gross, but it should not affect the underlying package name. That is to say, you can keep the long import path and just name the package "markydown". Given your other considerations, the convention in Effective Go doesn't fit very well in this situation.

 

Thanks again!

LMB

--
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+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Diego Medina]

Hi,

First, awesome that you started using Go! some thoughts inline:

 

1. The sbxs_ prefix is there just because I expect this package to be part of a set of related repositories and I wanted to give them some "unity" through naming. Arguably a bad idea, I am not sure about it.

I have seen two ways other Go developers have "solve" this,

1. Create a dedicated organization/github account for this group of repos, like

http://www.gorillatoolkit.org/

if you see that link (you probably are familiar with it already but if not)

you have:

gorilla/context

gorilla/mux

gorilla/reverse

gorilla/rpc 

gorilla/schema

gorilla/securecookie

gorilla/sessions 

gorilla/websocket

and people in the community refers to them as " ... have you tried gorilla mux? ..." , etc 

2. in your github account you create a repo sbxs, and inside there you add a folder go_markydown

if then you have a new component that is related, you create a new folder, go_asciidoc inside sbxs

this is somewhat similar to juju iirc

https://github.com/juju/juju

 

2. I added "go" to the package name because I tend to experiment with different languages from time to time, and therefore I have a reasonable chance to have naming conflicts without it.

this is a personal preference, you have your reason, sounds good to me :)

 

3. I added that extra "markydown" directory just because I read on Effective Go that it is a convention to give the package name the same name as the directory, and I thought that "markydown" was a better package name than "sbxs_go_markydown". Maybe I should just use the long package name and let users rename it when importing if they wish? Or is there  some better way?

skipping this one.

Thanks!

 

Thanks again!

LMB

[Leandro Motta Barros]

Just leaving my final tank you for everyone! You all really helped!

Cheers,

LMB