[nanomsg] Re: Review of mangos

  • From: Garrett D'Amore <garrett@xxxxxxxxxx>
  • To: "nanomsg@xxxxxxxxxxxxx" <nanomsg@xxxxxxxxxxxxx>
  • Date: Tue, 6 May 2014 09:58:19 -0700

Thanks for your review.  I will go through these in detail later tonight when I 
am able to do so on my personal equipment.  

Sent from my iPhone

> On May 6, 2014, at 9:52 AM, Gonzalo Diethelm <gonzalo.diethelm@xxxxxxxxx> 
> wrote:
> 
> Garrett, my apologies for misspelling your name!
> 
> 
> 
>> On Tue, May 6, 2014 at 12:46 PM, 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?
>> 
>> Line 128: isn't it necessary to close the file (maybe in a defer)?
>> 
>> Line 169, 177: should you check for the existence (and readability) of the 
>> file?
>> 
>> Line 273, 281, 290, 317: this code is repeated four times; move it to a 
>> helper?
>> 
>> 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)?
>> 
>> Line 393: maybe add some other special cases, like \t and \'.
>> 
>> 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?
>> 
>> Line 458, 491: this code is repeated twice; move it to a helper?
>> 
>> Line 531: shouldn't the sock.SetOption() be after the if sock == nil?
>> 
>> Line 554: why is it subscribing to every possible message by default? This 
>> could be a bad idea...
>> 
>> -- 
>> Gonzalo Diethelm
>> gonzalo.diethelm@xxxxxxxxx
> 
> 
> 
> -- 
> Gonzalo Diethelm
> gonzalo.diethelm@xxxxxxxxx

Other related posts: