[nanomsg] Re: Review of mangos
- From: Garrett D'Amore <garrett@xxxxxxxxxx>
- To: Gonzalo Diethelm <gonzalo.diethelm@xxxxxxxxx>, nanomsg@xxxxxxxxxxxxx
- Date: Thu, 8 May 2014 13:29:43 -0700
On May 6, 2014 at 9:47:38 AM, Gonzalo Diethelm (gonzalo.diethelm@xxxxxxxxx)
wrote:
Hello Garret,
I am going over your mangos code in order to improve my go-fu. Great job!
I started with macat.go, to make things easier for me, and I have several
questions / suggestions for you, which you will find below. I can provide
patches for anything you agree should be modified, but remember some / all of
these may be wrong, due to my ignorance of go.
Thanks and best regards.
---
# macat.go
Line 73: are you missing a check for if err != nil?
Good catch.
Line 128: isn't it necessary to close the file (maybe in a defer)?
A deferred close is a good idea. I’ve added it. (Technically probably not
100% necessary since macat is unlikely to be lived long enough for this to
matter, but it does leak a descriptor until exit.)
Line 169, 177: should you check for the existence (and readability) of the file?
No, that’s handled later. This is just getopt style validation.
Line 273, 281, 290, 317: this code is repeated four times; move it to a helper?
Possibly. Of questionable value as the block is only a couple of lines long.
Critically the variable being set is different each time, so handling that may
add more complexity than than its worth. I’m going to pass for now.
Line 376: you are creating the Writer even if it will not be used (when format
is "no"). Additionally, you are creating the Writer each time you call
printMsg(); can that be optimized (and is it necessary)?
Yes it can be, and I’ve done it like this, although admittedly its probably not
much difference:
if printFormat == "no" {
return
}
bw := bufio.NewWriter(os.Stdout)
switch printFormat {
…
Line 393: maybe add some other special cases, like \t and \'.
Actually this follows the same specification as nanocat. I agree that those
other cases might be useful, but that’s not the purpose here.
Line 457: I don't understand why there is a loop in there; are you sending many
copies of sendData? Or is it because the other side may not be up yet?
There may be many copies — see the sendInterval. :-) If that’s non zero, then
we keep doing it every interval.
Line 458, 491: this code is repeated twice; move it to a helper?
Not worth it.
Line 531: shouldn't the sock.SetOption() be after the if sock == nil?
Yes, good catch. Fixed.
Line 554: why is it subscribing to every possible message by default? This
could be a bad idea...
As discussed, this is by design. It follows nanocat(1).
Thanks for your review. I’ve pushed the changes noted above to my master repo.
- Garrett
--
Gonzalo Diethelm
gonzalo.diethelm@xxxxxxxxx
Other related posts:
