[nanomsg] Re: Review of mangos

  • From: Gonzalo Diethelm <gonzalo.diethelm@xxxxxxxxx>
  • To: "Garrett D'Amore" <garrett@xxxxxxxxxx>
  • Date: Thu, 8 May 2014 16:53:54 -0400

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)
> 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
>
>


-- 
Gonzalo Diethelm
gonzalo.diethelm@xxxxxxxxx

Other related posts: