Code Review Request: Golang qBittorrent API wrapper

blov · 2017-04-14 00:00:13 · 803 次点击    
这是一个分享于 2017-04-14 00:00:13 的资源,其中的信息可能已经有所发展或是发生改变。

link to project: https://github.com/jaredlmosley/go-qbittorrent

Hey, I wrote an API wrapper for qbittorrent based on the Python version as a way learn Golang and because I needed it for another project. Since this is my first project in Golang I was hoping I could have someone look over it and let me know if there are any problems or if I did not approach it in an idiomatic way.

I know I don't have any tests yet, I'm still trying to figure out a good way of writing them and will hopefully be adding them soon.


评论:

ar1819:

Okay - so the first thing you wanna do is to move main package into separate directory. Usually it's called %your project github path%/cmd/%you binary name%. This is a must or everyone who would want to use your library will be forced to build the main binary as well on the go get stage.

Second - don't println errors. Return them until you can handle them. Do not continue execution as if nothing happened. This can lead to disasters.

Also do not use error.New if you not storing it in a global variable. The reason for that is that I want to be able to catch your error without comparing strings. Avoid fmt.Errorf in most of the cases.

Do not store methods in separate file of type declaration. This makes navigation harder.

IMHO you should not store .idea directory inside git repo. We are all using different editors - you can use whatever you like - but there is no reason to store this meta inside repo.

Docs - this is super important. Document methods. Document constants. Document variables. Document your thoughts inside the code. This is very important to understand your intentions.

Thats all for now - I'll try to investigate more later.

anmousyony:

For the main.go part, thats only in the project temporarily as I was using it to test my changes. Both that and the .idea directory should be removed soon as I just set up my .gitignore file after moving the repo.

Outside of main.go, I'm not printing any errors I believe (I was printing them in main.go for my personal testing purposes). They should all be being bubbled up to the user for them to handle if I'm not handling them myself. For the errors I create, should I just declare them at the top of the file if I use them or would it be better to just not create any myself?

For the method and type declaration part, are you saying that the models.go file should be combined with the endpoints.go file where the structs are used or that requests.go and endpoints.go should be combined because they both rely on the client struct?

I believe all methods and types are documented according to the GoDocs format. Should I be documenting the intermediary variables even when their names are self-explanatory? I know I need to document the flow of some of the longer methods better.

Thank you very much for taking the time to tell me all this!

ar1819:

You are printing errors in tools package. Didn't look how much it is used around the code.

Declare errors at the top level. Use github.com/pkg/errors if you need to add some text context when error is returned (do not forget to mention this in the docs). For a more complex scenarios write custom types which implement error interface. Prefer to use existing errors and error types but ONLY if they apply to your domain.

Yes, combine requests and endpoints.go. I think most of us can handle 1k code in one file.

Docs - spend more than one sentence explaining what method does. Dont overdo this, tho. Explain behavior if method has side effects (most of them are).

Inside the method code explain the usage of magic constants and some of a control flow if you need it. The good advice is "write the comments about what you thought when you wrote this part of code". This helps a lot to understand your intentions in the future.

anmousyony:

Oh, I didn't notice that in the tools package. I was only using that package when I was first starting to test my requests and responses. Thank you for pointing that out.

The custom errors was exactly because I wasn't sure what was the proper error to return in those cases but I think I can change it so I am not creating any errors myself.

I'm having trouble writing meaningful comments, as most of my methods are just calling get/post/etc with a specific endpoint. Can you please give me an example where there was a method or section of control flow where a comment would have been helpful? (some of the code might have changed/moved around as I have been updating my code with your responses)

ar1819:

Yes - for example you are setting header field in the get method. You actually commented this by writing "add header". But you did not answer - why we are doing this in the first place?)

anmousyony:

Ahhh ok, I was focusing more on the "what" and "how" and I completely forgot the "why". Thank you so much!


入群交流(和以上内容无关):加入Go大咖交流群,或添加微信:liuxiaoyan-s 备注:入群;或加QQ群:692541889

803 次点击  
加入收藏 微博
暂无回复
添加一条新回复 (您需要 登录 后才能回复 没有账号 ?)
  • 请尽量让自己的回复能够对别人有帮助
  • 支持 Markdown 格式, **粗体**、~~删除线~~、`单行代码`
  • 支持 @ 本站用户;支持表情(输入 : 提示),见 Emoji cheat sheet
  • 图片支持拖拽、截图粘贴等方式上传