[nanomsg] Re: Review of mangos

  • From: Garrett D'Amore <garrett@xxxxxxxxxx>
  • To: Gonzalo Diethelm <gonzalo.diethelm@xxxxxxxxx>
  • Date: Thu, 8 May 2014 15:56:00 -0700

Yeah, sure, pull requests are welcome. :-)  Bitbucket has a similar workflow to 

Garrett D'Amore
Sent with Airmail

On May 8, 2014 at 1:53:55 PM, Gonzalo Diethelm (gonzalo.diethelm@xxxxxxxxx) 

Glad to be of (minor) help.

I have been reviewing other parts of mangos, but I have not had as much time as 
I thought; what's new, right?

Anyway, I was wondering if you would be willing to take patches in case I find 
anything worthy; I am assuming the answer is yes, since you did include some of 
my suggestions into macat.  If yes, should I just clone your repo, patch away 
in my clone and then send a pull request?  I have followed this workflow with 
github, I am assuming it would be similar with bitbucket. Is that correct?

On Thu, May 8, 2014 at 4:29 PM, Garrett D'Amore <garrett@xxxxxxxxxx> wrote:
On May 6, 2014 at 9:47:38 AM, Gonzalo Diethelm (gonzalo.diethelm@xxxxxxxxx) 
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" {



        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

Other related posts: