I made this. And I'm looking for your opinion! [GOlang]

Development Web Development
1

I’m a pretty experienced PHP developer, working on switching to golang.
I’ve made a small scoped project, both to learn more, and to generate some code I’d be able to share.

I’m very aware that there are numerous things wrong with the code, but I’d like to learn about what else is wrong and get an opportunity to make a case for choices I’ve made.

Link to repository:

Feel free to chime in even if you’re not an expert, I might be able to answer your questions or explain something :wink:

2 Likes
2

I’ll be terse, to the point, and opinionated, apologies in advance if this sounds rude, do take these with a grain of salt. This looks OK overall.

  • .htm (why not .html?)
  • I prefer flags to environment variables for configuration (defaults look nicer)
  • … similarly, replace :9000 with a string typed flag that defaults to :9000 – that way you can more easily run it locally for experimentation.
  • func render(... calls log.Fatal(err) … I would perhaps return a 500 and not crash.
  • everything is in package main … don’t do this. instead put everything into package researchr except main.go. rename main.go into researchr.go. That way your executable will be called researchr for easy go install and you’ll be able to refactor code more easily in the future if you want to share stuff cross package, and you probably won’t need a Makefile, except for docker stuff.
  • instead of make file, write a docker-compose.yml
  • your handlers are too short to be separate files - merge
  • you have methods on Env … I’d rename that to Researchr and provide a New (so that you have e.g. func (r *Researchr) handleReceive(w http.ResponseWriter, req *http.Request))… and so that you can do from within main the following r, err : = Researchr.New(dbhandle, http srv instance, otherdeps);
  • instead of parseExperimentFormData . I’d make func (e *Experiment) populateFromRequest(req *http.Request)
  • the r.FormValue look repetitive, you can wrap it into func formBool(r *http.Request, field string, trueValue string) bool { return r.FormValue(field) == trueValue) and then just:
    Experiment{ formBool("responsive", "yes"), formBool("head", "on"), ...}
  • rename DataStore and data_store.go into Store and store.go
  • RemainedResponsive0MissingPercent … looks like it should be an array of 8 members, instead of 8 separate members

That’s probably enough from me for first round of review, … possibly needs looking at is the tests / database code (because… security)

2 Likes
3

Thank you for feedback!

I’ll try to address it, and explain my choices. While I’m not always agreeing I appreciate all of it. My reply would make a better read from the bottom up.

  • While I don’t really care either way, I tend to prefer .htm over .html, and .jpg over .jpeg, tbf never thought about it
  • I didn’t know that flag package provided a nice, validated interface! That’s certainly something I’ll be using. That being said, I’d rather pass sql connection string via env than parameter
  • (Replacing :9000 with flag) valid point
  • (Rendering templates calling log.Fatal(err)) My rationale is: if error is temporary (lost connection to db due to DNS being busy eating glue) I can return a 500. Errors caused by programmer are a valid crash reason. It’s a rule of thumb at best, and definitely scale dependant.
  • (single package) I’ve thought about, but I couldn’t find anything longer than few lines that has any reuse potential or doesn’t share a logical scope. I’ll give it another shot in future. I don’t feel the value of having everything in package researchr instead of package main. I might be missing the intent, but I’d use go install pretty much only for tools I’m developing locally for project/myself.
  • (docker-compose over Makefile) I feel like those two have very different use cases. I usually put all the development “command snippets” I’m using in a given project into the Makefile. Due to small scope and compatibility with two container systems there’s disproportional amount of podman/docker stuff. Also I wouldn’t force docker-compose (dependency) on anyone, unless mandated by production environment :stuck_out_tongue_closed_eyes:
  • (short handlers) Most of them only render template, and I’d definitely put them in a single file, tho there is one at about 100 lines handler/test and I felt like splitting it. I mostly wanted to keep the tests separated :wink:
  • (renaming Env to Researchr ) I think that the idea here is to be able to “compose” main’s http server from multiple “projects”. It’s an interesting idea, but I’ll have to try it in different scenarios to get a feel for it.
  • ( parseExperimentFormData ) I do agree that the body of the function is related to the Experiment struct more than to the handler itself. My rationale for putting it in handler was related to the flow of control. Details of the form request are tied to the handler, and the handler dictates how to map it to a simple entity. Entity itself doesn’t contain any knowledge of logic “above” it. Another aspect is mutability: I could either create and return new instance, or create it then pass it to a method and modify it. While it’s nitpicking, when considering this aspect alone I think that the former is “correct”.
  • ( r.FormValue ) You’re absolutely correct, it’s needlessly long and repetitive. It definitely skipped at least one refactoring round, I kind of missed having ternary operators here. I’ve left “explicit” field names, I felt like they could easily shift on struct edit as there’s a lot of them, but that’s most likely baseless.
  • ( rename ) Agreed :wink:
  • ( RemainedResponsive0MissingPercent ) Right. At this point it looks like array, acts like array… so it needs to be an array :stuck_out_tongue_winking_eye:
2 Likes
4

with packaging and compose … in general with go you’re expected to go install url.to/a/package and go tool will fetch the code all the deps build the binary and put it somewhere correct. It’s strange than main packages are big, even though at the end of the day linker doesn’t care.

1 Like
5

I don’t see anything glaringly wrong from my quick skim.

High level:
If you haven’t already I’d take a look at:

  1. Effective Go - The Go Programming Language
  2. Tutorial: Create a Go module - The Go Programming Language
  3. And, although overkill for a small personal project it can give a good overview of some go project layouts GitHub - golang-standards/project-layout: Standard Go Project Layout

Codewise:
The only thing code specific things I have to say are:

  • As @risk said I would definitely consider moving your non-main functions to their own module/package (see #2) or if you want to be fancy, having one for your HTTP and one for your store.
  • You have an unreachable return at store.go:77

Edit: Just to clarify modules can be in the same git repo. Example:

// main.go
// Sudo code of course

import (
  "net/http"
  "github.com/mjw6i/researchr/handlers"
)

func main() {
  http.HandleFunc("/", handlers.Base)
  http.HandleFunc("/submit", handlers.Submit)
}

// handlers/base.go

func Base(w http.ResponseWriter, r *http.Request) {
  // ...
}
1 Like