<p>Hi!</p>
<p>Often my clients want to reset social sharing cache of many social networks. I have created a service to simplify this task. I've been wanting to practice in golang and write this service on golang.</p>
<p>Please, who have the opportunity, review my code and let the criticisms: <a href="https://github.com/iamsalnikov/socache" rel="nofollow">https://github.com/iamsalnikov/socache</a>.</p>
<p>Thank you!</p>
<hr/>**评论：**<br/><br/>66a4: <pre><p>First of all please add description to README file.</p></pre>iamchmo: <pre><p>But there is "Readme.md" file. I need to rename it to "README"?</p></pre>66a4: <pre><p>No but it will be nice to have detailed description of your project, goal of the project, motivation, etc. </p></pre>iamchmo: <pre><p>Thank you! I will do it.</p></pre>Ekranos: <pre><p><a href="https://github.com/iamsalnikov/socache/blob/master/server.go#L24" rel="nofollow">https://github.com/iamsalnikov/socache/blob/master/server.go#L24</a></p>
<p>Maybe just call these funcs vk.New() and facebook.New(), you already got the name in the package.</p>
<p><a href="https://github.com/iamsalnikov/socache/blob/master/cleaners/cleaner_interface.go" rel="nofollow">https://github.com/iamsalnikov/socache/blob/master/cleaners/cleaner_interface.go</a></p>
<p>You should rename the file and the interface to clearer/Clearer or (cleaner/Cleaner && rename the func in the interface to Clean). This is according to nearly all single func interfaces in the stdlib, like io.Reader, io.Writer, json.Marshaler etc.</p>
<p>You got a dependency to github.com/asaskevich/govalidator where you in fact just use !govalidator.IsURL. According to idiomatic go, you should replicate small functionality instead of depending on another package, example for this are found in the stdlib.</p>
<p>I don't get the point why easyjson is used, but i haven't looked into it though. Isn't a simple json.Marshal or json.Unmarshal / json.Encoder / json.Decoder sufficient? Maybe in use with <a href="https://mholt.github.io/json-to-go/" rel="nofollow">https://mholt.github.io/json-to-go/</a> to generate the structs automatically.</p>
<p>That's what i found on the first look. In case you want me to look further, i got some time on the weekend.</p></pre>iamchmo: <pre><p>Thank you very much for review! Your feedback is very useful for me!</p></pre>iamchmo: <pre><p>Ekranos, thank you very much again! I have created request with changes, following your review. <a href="https://github.com/iamsalnikov/socache/pull/4/files" rel="nofollow">https://github.com/iamsalnikov/socache/pull/4/files</a></p></pre>k_u_r_o_k_u_s_e: <pre><p>There are 0 tests.</p></pre>iamchmo: <pre><p>Thank you! I Mark it for itself</p></pre>DeedleFake: <pre><p>Here's a list of quibbles and complaints. I'll update it as I find stuff.</p>
<ul>
<li>Why are the flags global variables if they're only ever used in <code>main()</code>? They're polluting the global namespace for no particular reason.</li>
<li><code>cleaners.CleanerInterface</code> is needlessly repetitive. <del>How about just <code>cleaners.Interface</code>?</del> <a href="https://www.reddit.com/r/golang/comments/51xjim/gophers_please_review_my_code/d7frtx9" rel="nofollow">Or not.</a></li>
<li>I'm not sure why the <code>Server</code> struct exists. Go isn't OO, and you don't need to encapsulate everything in types. In particular, there's basically no reason to have a singleton in the <code>main</code> package. Just move everything in <code>main</code> into <code>main.go</code> and make <code>Server</code>'s methods top-level functions.</li>
</ul></pre>iamchmo: <pre><p>Thank you very much! I will fix it.</p></pre>iamchmo: <pre><p>DeedleFake, thank you again! I have created pull request, following your list. I fix first point of it.</p>
<p><a href="https://github.com/iamsalnikov/socache/pull/6" rel="nofollow">https://github.com/iamsalnikov/socache/pull/6</a></p>
<p>Thank you again!</p></pre>Ekranos: <pre><p>cleaners.Interface wouldn't be idiomatic Go though. Take a look at some of the single func interfaces in the stdlib.</p>
<ul>
<li>String -> Stringer</li>
<li>Close -> Closer</li>
<li>Write -> Writer</li>
<li>Read -> Reader</li>
</ul>
<p>etc.</p>
<p>I see nothing wrong with the server struct, it could get pretty handy for future additions. Don't forget, alot of projects start small and get pretty messy when time goes on. I got one such thing to maintain where all the server setup is done in one function.</p>
<p>Here is a paste:
<a href="http://pastebin.com/XCmWE7zk" rel="nofollow">http://pastebin.com/XCmWE7zk</a></p>
<p>That's messy, isn't it? I would be much happier with some structs there.</p>
<p>EDIT: I see pastebin stripped out tons of newlines...</p></pre>DeedleFake: <pre><p>That's true. I was thinking about it more in terms of <a href="https://golang.org/doc/effective_go.html#package-names" rel="nofollow">stutter</a>. Specifically, I was thinking about <code>sort.Interface</code>, but that's a three method interface. On the other hand, I'm not a big fan of <code>cleaners.Clearer</code>. Maybe rename the package and use <code>cleaner.Cleaner</code> with a <code>Clean()</code> method? It's probably a good idea to be consistent with names either way.</p></pre>Ekranos: <pre><p>I can't think of an equal case in the stdlib. That's a pretty ugly corner-case. Haven't found something like that after some googling. But since sort.Interface is there and you got no stutter then...</p>
<p>I would guess either way is okay. Somebody wants to post this case to go-nuts?</p></pre>epiris: <pre><p>I think pkg.Interface is a good/ok name for a package in some scenarios. If the package primarily is a collection of funcs / maybe a single struct which implements the <pkg> default which operate upon a single interface when it's operation is clear by the pkg name. Which I guess isn't a ton of scenarios :) but it comes up.</p>
<p>I.e. it would make sense to me what these interfaces are for:</p>
<ul>
<li>shape.Interface</li>
<li>transport.Interface</li>
<li>event.Interface</li>
<li>service.Interface</li>
<li>server.Interface</li>
</ul></pre>
