Hacker News Reader

Comment by ImJasonH | Hacker News Reader

Some Go API style nits:

- it's unusual to pass a map with expected keys like this. Normally you'd pass a struct like: p := New(Options{ BaseColor: "...", Color: "...", Generator: "...", })

- You could define the known patterns as constants so they're documented and so users don't have to worry about typos

- Field names are normally camel-case without underscores, e.g., "Base_color" should be "BaseColor"

- You could use Go's build-in image/color package instead of accepting RGB strings, but that's more of a judgement call.

If you're accepting pull requests I might send you some, but they're breaking API changes so it's up to you.

All-in-all though it's a cool project and I'm glad to see it in Go :)

View on HN

Hi @ImJasonH, I Developed this, let me answer you points :)

I believe these points are not contributing in breaking API :D so lets talk on them first.

2. You mean instead of an array I'm using here [1], We should add them as constants and write separate documentation for each? This sounds nice as it will help obviously like you said.

3. Ohh, I missed this convention, thanks for pointing this out. :)

4. As you said its more of a judgement call, I believe that RGB strings are fine and easy for users. You find any wrong in RGB strings?

So, I'll be very happy to co-operate if you are doing pull request covering points 2 and 3 :) And I'll try to cover your point 1 on my own :)

Cheers \o/

[1] https://github.com/pravj/geo_pattern/blob/master/pattern/pat...

Regarding 4, I personally tend to be distrustful of what is often described as Stringly Typed code. To me, statically constraining your API to only accept valid inputs seems like superior design.

If you're reworking your API, you may want to consider functional options:

http://dave.cheney.net/2014/10/17/functional-options-for-fri...

I haven't attempted it myself, but it looks very interesting.

Another minor nitpick, here [1] you are using reflection. You can pretty easily convert it to a type switch [2], which is more idiomatic.

[1] https://github.com/pravj/geo_pattern/blob/master/svg/svg.go#...

[2] https://golang.org/doc/effective_go.html#type_switch

Reply on news.ycombinator.com