Open dgryski opened 6 years ago
Thanks a lot for reviewing and listing improvements @dgryski ❤️
I'll get to fixing/improving the things you mentioned one at a time soon.
EDIT: I took the liberty to modify the issue description into a checklist so that I can easily keep track of things to fix.
I've only quickly glanced over the code but I've noticed a few things:
log.Printf
and thenos.Exit
. You can just calllog.Fatalf
instead, which does the same thing.r.ReadString('\n')
, use abufio.Scanner
: https://golang.org/pkg/bufio/#Scanner , it's a nicer API.io.EOF
and data that was partially read. In the case ofReadString
, you're assured thaterr != nil
iff the data doesn't end in the delimiter, which in your case means likely an incomplete line was read, but it's still good to document that you're discarding a potentially incomplete line.parseEvent
returns an empty string for the error when the broadcast packet doesn't have two fieldscase "F", "U", "P": ...
recover()
directly. In order to catch a panic,recover()
needs to be in adefer
and you need to check the return value fromrecover
to see if you're actually panicking or not.os.Kill
, which isSIGKILL
, which you can't catch. You probably meantSIGTERM
(but there's no constant for that in theos
package, just pullsyscall.SIGTERM
directly.[]byte
->string
->[]byte
. It's probably worth profiling to see if you can do away with the extra string conversion which will involve both a memory allocation and a data copy.