> > I agree even though I've never thought like this. For my own curiosity, > what gain do you see by embedding structs like this?
Now when making calls you don’t have to refer to the field to call the methods; the embedded type’s methods are promoted to the struct type but still use the field value in the method. In my projects I’ve liked this as it has the code use less words (t.Lock() instead of t.m.Lock()), although I can see that there may be cases where referring to the struct field explicitly is preferable. This is to satisfy interfaces indeed. If you have an alternative solution, > I'd be happy to hear it. My thought is that the interface itself should be changed since the methods it calls have no need for data in the underlying type. Here’s an alternative representation I suggested in another code review: https://play.golang.org/p/F_f0hLNsmSW What do you mean by that? I may have misunderstood the use of the types as inputs to other pieces, but perhaps refactoring the types could simplify the library. Same, what do you mean by that? A concise README.md may help people use the library. Moving some of the documentation to sub-folders like demo and leaving a reference in the readme may be as effective without so much scrolling. Thanks for sharing Bob here. Matt On Tuesday, January 30, 2018 at 2:31:06 AM UTC-6, Asticode wrote: > > First off, cheers for the code review :) > > >> Since type ability is not exported, why not embed sync.Mutex? > > I agree. > > >> What does o stand for? > > I named this variable "isOn" first, but then I named the function to > access it the same way, therefore I had to find another name. I do agree > "o" sucks, I'll try to find another clearer way. > > >> I’d say that comments like “// newAbility creates a new ability” are > unnecessary. The identifiers are documentation. There are many unnecessary > comments. > > I agree. > > >> For read-only methods like *ability.isOn() perhaps a sync.RWMutex may > make sense. > > I agree. > > >> type brain could have sync.Mutex and *astiws.Client embedded. type > brains, type dispatcher, type interfaces could have sync.Mutex embedded. > type server could have everything besides name embedded. type brainsServer > could have everything embedded. type clientsServer could have everything > besides stopFunc embedded. The sub-packages have more cases like this. > > I agree even though I've never thought like this. For my own curiosity, > what gain do you see by embedding structs like this? > > >> There are no tests. > > I agree :( > > >> The horizontal dependency of go-astibob/abilities to go-astibob/brain > is a red flag for me. Why not define the abilities in package astibob? > > I can't see any reasons why not, therefore that may be a good idea. > > >> I don’t understand the pkg directory, why not have these as regular > sub-packages? > > I wanted to keep packages using CGO in a separate place since astibob, > astibrain and abilities use interfaces to interact with features. Having > one root folder for those packages was more convenient for me. But you may > be right. > > >> In go-astibob/pkg/portaudio, robotgo, I’m not sure what an alternative > would be, but defining methods on a struct{} type seems unnecessary > compared to just having functions. Elsewhere I’ve suggested defining a > struct type with function fields instead. If this is to satisfy an > interface, perhaps there is another approach where the interface is > unnecessary. > > This is to satisfy interfaces indeed. If you have an alternative solution, > I'd be happy to hear it. > > >> The godoc is a reasonable size but there are a lot of types without any > functionality > > What do you mean by that? > > >> The README.md seems long, perhaps examples/demo could contain some of > that information. > > Same, what do you mean by that? > > Again, cheers for the code review :) > > > > > Le lundi 29 janvier 2018 17:39:27 UTC+1, matthe...@gmail.com a écrit : >> >> Hi Quentin, here’s a code review. >> >> Since type ability is not exported, why not embed sync.Mutex? What does o >> stand for? >> >> I’d say that comments like “// newAbility creates a new ability” are >> unnecessary. The identifiers are documentation. There are many unnecessary >> comments. >> >> For read-only methods like *ability.isOn() perhaps a sync.RWMutex may >> make sense. >> >> type brain could have sync.Mutex and *astiws.Client embedded. type >> brains, type dispatcher, type interfaces could have sync.Mutex embedded. >> type server could have everything besides name embedded. type brainsServer >> could have everything embedded. type clientsServer could have everything >> besides stopFunc embedded. The sub-packages have more cases like this. >> >> There are no tests. >> >> The horizontal dependency of go-astibob/abilities to go-astibob/brain is >> a red flag for me. Why not define the abilities in package astibob? >> >> To me the demo examples show a clean interface for interacting with the >> library. >> >> I don’t understand the pkg directory, why not have these as regular >> sub-packages? >> >> In go-astibob/pkg/portaudio, robotgo, I’m not sure what an alternative >> would be, but defining methods on a struct{} type seems unnecessary >> compared to just having functions. Elsewhere I’ve suggested defining a >> struct type with function fields instead. If this is to satisfy an >> interface, perhaps there is another approach where the interface is >> unnecessary. >> >> These are the most complex html templates I’ve seen, I may refer back to >> these sometime. >> >> The godoc is a reasonable size but there are a lot of types without any >> functionality. The README.md seems long, perhaps examples/demo could >> contain some of that information. >> >> Matt >> >> On Monday, January 29, 2018 at 2:13:24 AM UTC-6, Asticode wrote: >>> >>> Heys guys, >>> >>> I'm happy to announce Bob, a framework to create a distributed AI that >>> can learn to understand your voice, speak back, interact with your mouse >>> and keyboard, and anything else you want: >>> https://github.com/asticode/go-astibob. >>> >>> Let me know what you think. >>> >>> Cheers >>> >> -- You received this message because you are subscribed to the Google Groups "golang-nuts" group. To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
