Code Review - SimpleEventBus

<p>Hi! I have a project that is working cool, but I will like to expand. To start I am opening for code review.</p> <p><a href="https://github.com/aristofanio/eventbus" rel="nofollow">GoEventBus</a></p> <hr/>**评论：**<br/><br/>CHAOSFISCH: <pre><p>Hey, there&#39;s a lot which can be improved; of course take everything with a grain of salt, maybe you had a specific reason to do it this way; I&#39;ll just leave some open questions upon seeing your code:</p> <ul> <li>Why JSON and not something like Protobuf?</li> <li>Code mapping a struct to/from JSON: You could use json.Marshal and json.Unmarshal if you insist on using JSON. Additionally, if you need to do some specific mapping then take a look at the example <a href="https://golang.org/pkg/encoding/json/#Marshal" rel="nofollow">https://golang.org/pkg/encoding/json/#Marshal</a></li> <li>The used singleton patterns are probably not what you want. You make it impossible to change any Repository/Storage implementation like that.</li> <li>e.g. in storage.go you access a map (concurrently). Is it thread-safe. This smells like a data-race you should check.</li> <li>the code uses log.Print.. pretty much everywhere. What if I want to use a different logger or not log at all?</li> <li>0 tests</li> <li>frame.go looks &#34;complicated&#34;, i.e., is there a reason you chose these specific constants? Did you port the code and kept that for compatibility to a different library? Additionally, in this file variable naming does not follow the go specification, i.e., get rid of all the underscores.</li> <li>Any reason for defining the type EventType? Why not use a string instead?</li> </ul></pre>aristofanio: <pre><p>Thanks! </p></pre>aristofanio: <pre><blockquote> <ul> <li>Why JSON and not something like Protobuf?</li> <li>Code mapping a struct to/from JSON: You could use json.Marshal and json.Unmarshal if you insist on using JSON. Additionally, if you need to do some specific mapping then take a look at the example <a href="https://golang.org/pkg/encoding/json/#Marshal" rel="nofollow">https://golang.org/pkg/encoding/json/#Marshal</a></li> </ul> </blockquote> <p><em>Add as new feature: protobuf (as optional)</em></p> <blockquote> <ul> <li>The used singleton patterns are probably not what you want. You make it impossible to change any Repository/Storage implementation like that.</li> <li>e.g. in storage.go you access a map (concurrently). Is it thread-safe. This smells like a data-race you should check.</li> <li>Any reason for defining the type EventType? Why not use a string instead?</li> <li>Additionally, in this file variable naming does not follow the go specification, i.e., get rid of all the underscores.</li> </ul> </blockquote> <p><em>These was decisions in java-thinking style (thanks! this require more experience in go. putting in the next review)</em></p> <blockquote> <p>0 tests frame.go looks &#34;complicated&#34;, i.e., is there a reason you chose these specific constants? Did you port the code and kept that for compatibility to a different library? </p> </blockquote> <p><em>The docs and tests was removed</em></p> <blockquote> <ul> <li>the code uses log.Print.. pretty much everywhere. What if I want to use a different logger or not log at all?</li> </ul> </blockquote> <p><strong>Please, explain more!</strong></p></pre>CHAOSFISCH: <pre><p>I hope you only refer to the last part where you want more explanation. Consider three example situations that might come up if someone is using your library:</p> <ul> <li>You don&#39;t want to have any log output written by your library. To achieve this right now, the developer has to step through all lines of code and remove any call to log.Print/log.Printf and so on. There&#39;s no way for the developer to disable logging entirely. </li> <li>A developer wants to have log output but the format used by the standard go log package is different from the one used in the remaining application. Again, to fix this behavior the developer needs to check all lines and replace each with his own logger implementation.</li> <li>A third developer wants to leave everything as is, but instead of logging to stdout he wants to log to a specific file. Again, this could involve changing the code.</li> </ul> <p>There&#39;s a simple fix to this problem. Instead of depending on a concrete logger, you use an interface. Read this and have a look at the third code example. <a href="https://dave.cheney.net/2017/01/23/the-package-level-logger-anti-pattern" rel="nofollow">https://dave.cheney.net/2017/01/23/the-package-level-logger-anti-pattern</a> By using an interface, the developer can achieve any of the 3 scenarios without any changes to your library.</p></pre>aristofanio: <pre><p>Thanks! Amazing observations!</p></pre>