'Resume' Code Review


(Jaron R. Swab) #1

Hi everyone,
I am looking for a code review to help find where my gaps in knowledge are currently.

I would like to (someday) get a job developing with Go and want to make sure the code I have hosted on GitHub is not an eyesore to look upon.

Any feedback is appreciated as I want to get better and be able to show the knowledge I have with the Go programming language.

My repo can be found at:

Thanks!


(catacombs) #2

Nice job. I, too, wrote a version of a dwm status bar in go :).

A couple things I noticed:

I think you can do var i uint16 = 0 as i := 0. A little shorter and easier to read. No need to expliclity say its a unsigned 16-bit integer.

I don’t think you need a timer in this script if you’re displaying the status.

DWM offers an autostart patch that can initialize the statusbar exectuable via bash. All you’d need to do is set a while loop to a however long you want – maybe half a second – and have that call your executable.

No need to do this:

cat := []string{"", // do not delete or comment out this line
			"RAM:", ramFree, "free", "|",
			weather, "|",
			hTime,
			""} // do not delete or comment out this line

		// concatinate all strings to one line for output
		status := strings.Join(cat, " ")

Instead, you could do something like this:

ram := fmt.Sprintf("RAM: %d free", ramFree)
weather := fmt.Sprintf("%s", weather)
time := fmt.Sprintf("%s", hTime)

status := fmt.Sprintf("%s | %s | %s", ram, weather, time)

cmd := exec.Command("xsetroot", "-name", status)
cmd.Run()

(Jaron R. Swab) #3

Thanks for all the tips!

I was under the impression that using i := 0 would default to an int64 since my system is 64 bit (not sure where I heard that) and wanted to keep the int as small as possible. If that is not the case then yeah, calling var is too verbose.

Awe yes! Sprintf totally overlooked that! hahaha

Thanks and I will look into DWM the DWM auto start patch. I have been running patch-less for a while to see what I need/miss from i3wm.


(catacombs) #4

i := 0 defaults to a 64-bit int because, as you said, your system is 64 bits. Why do do you want to keep the i as small as possible? Memory management isn’t really an issue in Go, especially with its garbage collection. For the statusbar, I am confident int is more than sufficient for your needs.


(Jaron R. Swab) #5

Good point, forgot about that.

I changed the code to use Go Routines (for example purposes) and no longer need the int in main. I want to pass the pointer through the channel but that was giving me issues so I’ll have to read up on that in tho morning.

Thanks again :grin:


(Ivan Matmati) #6

Hi jsrwab,

I didn’t have time to get into the details of your implementation, so my feedback is more a feeling than anything else. I think you should use differently your channels. In main.go, you should use a for select block. It would be nicer and more standard. The loop in wttr.go is puzzinlig me. Why not use a Ticker instead of this complex management of i ?
Don’t forget that your defer will be executed at the return of your function ! In case of a loop, it’s never executed. You’ve got a potential leak : https://golang.org/ref/spec#Defer_statements
Have a nice day.


(Jaron R. Swab) #7

Thanks I’ll look into all this after work and see if I can clean it up.


(George Calianu) #8

A small observation, is no need to explicit initialize variables to their zero values in Go.

https://tour.golang.org/basics/12


(Jaron R. Swab) #9

Awe yes, I remember reading that now. Thanks @geosoft1!


(Jaron R. Swab) #10

I moved to a ticker.

I also removed the defer and just have it close the https request. In doing this will it reopen the next time the go routine for that lib is called or did I just block any updates? (Luckily the temperature does not change that often anyway lol)


(Ivan Matmati) #11

Hi,

Your goroutines launches every five seconds and every hour could benefit from ticker too.
In fact, these tickers seems to be movable into the goroutines themselves. In the new versions you wrote without the loop, now the defer would be perfect because some errors could happen or premature return … You’ve got two choices :

  • for loop -> no defer inside
  • no for loop -> defer is perfect
    They are mutually exclusive.

The resp.Body.Close is actually closing the response not the request. And yes, not closing must have some bad effects somewhere as it is required from the documentation of Body field : It is the caller’s responsibility to close Body. If you don’t close the stream of the response it could mean that the connection is hold opened. Then at a certain moment, you will run out of available connections because of resource starving.


(Jaron R. Swab) #12

Good to know! I’ll do some more playing around and see what I can come up with.

Thanks again for all the help! Having you all look at this code is allowing me to learn more of the ins and outs Go.

By chance do you mean to defer the closing of the HTTP request as follows instead of manually closing it after the channel passes the data?

func Local(cWttr chan string) {
        // get temp(%t) and wind direction/speed (%w)
        // for exact location add postal code - wttr.in/~15222?format...
        // for more wttr options see https://wttr.in/:help
        resp, err := http.Get("https://wttr.in/?format=%t+%w")
        if err != nil {
                errMessage := "wttr connection issue"
                cWttr <- errMessage
        }
        defer resp.Body.Close() // close http request

        // convert responce to string for return
        bodyData, _ := ioutil.ReadAll(resp.Body)
        weather := fmt.Sprintf("%s | ", strings.TrimSpace(string(bodyData)))
        cWttr <- weather
}

(Jaron R. Swab) #13

Update:
I chose implement tickers into the libs running as go routines and it works like a charm.

Thanks again to everyone that added suggestions on what I could do to improve my Go example. Can’t wait to tackle another project :smiley:

PS: I am still open to suggestions on this repo so feel free to add share your ideas. It’s on both gitlab and github.

:pray::blue_heart:


(catacombs) #14

Bit of a pain to keep the code in two different places, no?


(Jaron R. Swab) #15

I just commit to master on Github and only put stuff there since so many people use it. All my work in progress and branches only get pushed to Gitlab.


(Onezino Moreira) #16

Some tips regarding your project structure:

1 - Create using the deps. Sometimes you’ll need dependencies.
2 - Add tests, if you are familiar with golang tests you are one step ahead.
3 - Instead of use plain string channel return, you can type your own. type weather string.
5 - Use error chan and not a only one chan to data/error.
4 - Add interfaces (you must be familiar with) and work with it.

5 - Most libraries use the cmd/binname/main.go as a entry point to compile and generate a executable file.


(Jaron R. Swab) #17

Thank you! I’ll implement these features next.

The goal here is to prove to any future interviewer that I can program in Go. Plus practicing these parts of Go will only.help me in nhe long run.


(catacombs) #18

Can’t stress this enough. Testing in Go is dead simple and will make your programs work infinitely better.


(Ivan Matmati) #19

Hi,

defer should happen as soon as possible, before an error may occur preventing your deferred code from executing. So in your example, I’ll put it right after your Get call.
After all, you could send on a close channel and get an error !


(Jaron R. Swab) #20

@catacombs & @heatbr I added to simple tests to my custom packages and set up two new channels for error messages (I think I want to out put any errors to a log file at some point). The new code is up loaded and in the master branch.

@imatmati That makes sense, no reason to leave something open if something else breaks.

Thanks again everyone for the help!
I’ll be implementing more of heatbr’s suggestions this weekend.