-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
A few small nits #1
Comments
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. |
sahildua2305
added a commit
that referenced
this issue
Feb 14, 2018
Because well, as per Go docs here(https://golang.org/pkg/log/#Fatalln): "Fatalln is equivalent to Println() followed by a call to os.Exit(1)." Also, thanks to @dgryski for pointing it out in: #1
sahildua2305
added a commit
that referenced
this issue
Feb 14, 2018
This was most probably just a missed error message while refactoring the code around parseEvent. Reported in #1.
sahildua2305
added a commit
that referenced
this issue
Feb 18, 2018
Cases for "F", "U" and "P" in parseEvent method were identical and hence can be combined to avoid code duplication. Reported in #1.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.The text was updated successfully, but these errors were encountered: