<p>I just published my very first Golang <a href="https://github.com/opengemara/opengemara-compiler" rel="nofollow">project</a>. </p>
<p>The main part of the project is <a href="https://github.com/opengemara/opengemara-compiler/blob/master/server/CompilerLibrary/compiler.go" rel="nofollow">here</a>.</p>
<p>It's a custom markdown->JSON transpiler.</p>
<p>Can anyone please provide some code-review?</p>
<hr/>**评论：**<br/><br/>8lall0: <pre><p>I suggest you this <a href="https://www.youtube.com/watch?v=HxaD_trXwRE" rel="nofollow">video</a>. </p></pre>shmuelbrin: <pre><p>Thanks!</p></pre>allhatenocattle: <pre><p><a href="https://github.com/opengemara/opengemara-compiler/blob/master/compiler/compiler.go#L103" rel="nofollow">https://github.com/opengemara/opengemara-compiler/blob/master/compiler/compiler.go#L103</a></p>
<p>It is sleeping for 200ns not 200ms. You'd want
time.Sleep(200 * time.Millisecond)</p>
<p><a href="https://github.com/opengemara/opengemara-compiler/blob/master/compiler/compiler.go#L107" rel="nofollow">https://github.com/opengemara/opengemara-compiler/blob/master/compiler/compiler.go#L107</a>
The else clause isn't needed, just do the if .. break, and keep the rest inline</p>
<p><a href="https://github.com/opengemara/opengemara-compiler/blob/master/compiler/compiler.go#L122" rel="nofollow">https://github.com/opengemara/opengemara-compiler/blob/master/compiler/compiler.go#L122</a>
This else isn't needed since the if statement returns, keep it inline</p>
<p><a href="https://github.com/opengemara/opengemara-compiler/blob/master/compiler/compiler.go#L133" rel="nofollow">https://github.com/opengemara/opengemara-compiler/blob/master/compiler/compiler.go#L133</a>
fmt.Sprintf would be a cleaner way to construct a string</p>
<p><a href="https://github.com/opengemara/opengemara-compiler/blob/master/compiler/compiler.go#L122" rel="nofollow">https://github.com/opengemara/opengemara-compiler/blob/master/compiler/compiler.go#L122</a>
use resp.StatusCode == http.StatusOK</p>
<p><a href="https://github.com/opengemara/opengemara-compiler/blob/master/compiler/compiler.go#L114" rel="nofollow">https://github.com/opengemara/opengemara-compiler/blob/master/compiler/compiler.go#L114</a>
I don't think that check is necessary, won't it keep in the loop until error free or panicing?</p>
<p><a href="https://github.com/opengemara/opengemara-compiler/blob/master/compiler/compiler.go#L50" rel="nofollow">https://github.com/opengemara/opengemara-compiler/blob/master/compiler/compiler.go#L50</a>
I wouldn't use a channel for passing back errors, it makes the logic less clear. I'd have processFile also return an error and keep track of them in a slice inside <a href="https://github.com/opengemara/opengemara-compiler/blob/master/compiler/compiler.go#L52" rel="nofollow">https://github.com/opengemara/opengemara-compiler/blob/master/compiler/compiler.go#L52</a></p>
<p><a href="https://github.com/opengemara/opengemara-compiler/blob/master/server/server.go#L39" rel="nofollow">https://github.com/opengemara/opengemara-compiler/blob/master/server/server.go#L39</a>
don't you want to return here if err != nil?</p>
<p><a href="https://github.com/opengemara/opengemara-compiler/blob/master/server/server.go#L79" rel="nofollow">https://github.com/opengemara/opengemara-compiler/blob/master/server/server.go#L79</a>
is that the status you want, seems suprising</p>
<p><a href="https://github.com/opengemara/opengemara-compiler/blob/master/server/CompilerLibrary/compiler.go#L388" rel="nofollow">https://github.com/opengemara/opengemara-compiler/blob/master/server/CompilerLibrary/compiler.go#L388</a>
else not needed since if returns</p>
<p><a href="https://github.com/opengemara/opengemara-compiler/blob/master/server/CompilerLibrary/compiler.go#L431" rel="nofollow">https://github.com/opengemara/opengemara-compiler/blob/master/server/CompilerLibrary/compiler.go#L431</a>
string constants should be stored as consts</p>
<p><a href="https://github.com/opengemara/opengemara-compiler/blob/master/server/CompilerLibrary/compiler.go#L504" rel="nofollow">https://github.com/opengemara/opengemara-compiler/blob/master/server/CompilerLibrary/compiler.go#L504</a>
return nil explicitly since err was already checked</p>
<p><a href="https://github.com/opengemara/opengemara-compiler/blob/master/server/CompilerLibrary/compiler.go#L579" rel="nofollow">https://github.com/opengemara/opengemara-compiler/blob/master/server/CompilerLibrary/compiler.go#L579</a>
this func is getting crazy deep, refactor</p></pre>Justinsaccount: <pre><p>What's with this global <code>wordList</code> thing?</p></pre>shmuelbrin: <pre><p>It's a list of words and their definitions. I made it global because it's almost a constant, and I don't want to reload it on every connection.</p>
<p>I would have kept it in a SQL database instead (querying the database instead of the <code>map</code>), but it's much easier to edit flat files than a DB.</p></pre>Justinsaccount: <pre><p>ah, you should probably wrap that up in an struct somewhere.. it's a little weird that other packages inject words into the CompilerLibrary package via InitWordList</p></pre>Thaxll: <pre><p>Some of the functions are big, might be better to break them down a bit!</p></pre>ImLopshire: <pre><p>I think the structure of the project could be improved. Take a look at <a href="https://medium.com/@benbjohnson/structuring-applications-in-go-3b04be4ff091" rel="nofollow">this article</a> for some tips.</p>
<p>I also don’t like that the CLI requires you to spin up the sever locally. If you were to separate the logic from the http handlers, you could use it directly in the CLI.</p>
<p>In general a http handler should only be responsible for transport related logic: reading from a request and writing the response. The actual business logic should exists some where else. <a href="https://gokit.io/examples/stringsvc.html" rel="nofollow">This example</a> from gokit demonstrates this pattern well. </p></pre>Killing_Spark: <pre><p>I second this. For the cli I would prefere a way to only call the compiler once. </p></pre>
