Closed Denis535 closed 5 years ago
Hi @Deni35. Thanks for the feedback. It has been a challenge trying to keep this project complete and easy to use (for both experienced and inexperienced developers) ...I'm not sure if I have succeeded either.
From what perspective, specifically, does this project not look simple and friendly? The documentation (on my todo list), the code (can't help that much), the examples (I don't know if this can be simpler), the interface (and comments, which could be better), other? Basically what was your approach and when did you find difficulty (not simple)?
To help more advanced users understand the project I created the architecture diagram with shows where the high and low-level classes exist and their relationship. I don't think separation into separate modules would help much since high-level uses low-level. They are already separated by namespaces, so, I assume this is really a documentation issue (which I know needs work). I did consider separating API and WebSocket into separate libraries due to dependencies, but for no other reason. Also, the thin wrapper is (low-level) BinanceHttpClient
, which returns JSON responses.
Now it's not "designed for ease of use"?)) And why you marked this issue as question?
It's not documentation issue. But to understand your diagram is not easy. Maybe some boundaries could make it easier. Every project has primary (business) and secondary (helper) logic. Business logic interacts directly with binance. Helpers is common for all exchanges and other services. Also Business logic could have its helpers (like your serializers). I don't need to understand and to see your helpers (at least in the beginning). I need to focus on business logic. So, we should highlight primary logic and hide secondary logic. You can do it via namespaces (put all your helpers in Binance.Helpers... folder/namespace) or via projects. In second case we can be sure no dependencies rules violation. You should highlight your gateways (REST, WebSocket), entities (request parameters, responses, misc), architecture components (or interfaces in your case) and other helpers.
I always trying to split my code on buisness logic and helpers to keep my buisness logic thin and clean. It allow me/other to better focus on code/issue i'm working on. I'm doing it on every level: projects, namespaces, folders and class level. If my class is big I'm writing comment // Helpers
at the end of class and then move low-level code in helper-fucntions. I found this a very useful habit.
It would not be correct to say that I am right and you are not. Many things can be a matter of taste. And we can all be wrong. But if you read the book Clean Architecture by Robert C. Martin, it could changed your thinking.
I also have a few more notes. 1) Root namespace has too many items. And items within many folders don't have thair namespaces. Sometimes it makes sense. But looks like not here. 2) Extensions folder contains classes with different namespaces. I understand your thinking. But I think it's evil. If you don't want to put this classes in base folders, you can create folders: WebSocket_Exts, Client_Exts and etc. But better create sub folders: abstraction, implementation, extensions, enums. Like you made Exceptions sub folder. 3) Usings must be within namespace. But this is only your convenience. 4) StyleCop has rule "DoNotUseRegions". And I agree with it. As I understand you are using ReSharper, maybe ReSharper somehow helps you manage the rigions. Split code via comments is much more convenient for me. 5) BinanceApi is good name, it says: i'm gateway, i'm main logic, i'm working with this exhange. But AggregateTradeWebSocketClient not saying this. At first glance it looked as low-level class. Of course the name is and so long. I think you should move it to namespace Binance.Gateway.
I labeled this a question because I wanted more information (specifics) and because of: "Maybe if you decompose project into base/high/low-level logic and helpers/utils, it gets more friendly ?"
[edit] Also, the title has a question mark... "Can you split project into high/low-level and infrastructure modules ?"
I changed the description to hopefully curb some expectations (including your own). I considered removing the "ease of use" part for some time. Long before this issue, it became apparent that easy is relative/subjective and makes for an impossible requirement, like trying to please everyone all the time. Having that as part of the description makes it seem like a requirement. I still, of course, will try to make this library easy to use, regardless.
I will look into adding some boundaries to the architecture diagram. I think I wanted to make the diagram as small as possible at the time. I did add color to highlight which classes are top-level. Regardless, this and the associated text was meant for those getting deep into the code, not really as a starting point.
I know there are many classes in the root namespace. That was intentional to simplify namespace usage. Because some of the simplest examples required including several namespaces and it also seemed at times people were having difficulty finding methods (not easy to use), not knowing what namespace to include. I also, had extensions/helpers behind Binance.Extensions
, but that seemed unnecessary. Keeping in mind, from a user's perspective, they shouldn't care what methods are direct and what are helpers.
Perhaps I took namespace simplification too far, at least in your case; I will look at namespace separations again. However, for me, this seems like just another aspect of trying to please everyone all the time... when I respond to issues like this I also have to make assumptions for all those that haven't said anything and I tend to ping-pong back and forth until hopefully I reach a least painful middle-ground 😄. It would be great if I could ask my entire user base and have them vote on some of the changes, but that doesn't seem feasible.
Yes, the name AggregateTradeWebSocketClient
may be too technical and born from the low-level up (not what I want for an API). Perhaps something like AggregateTradeStream
would be more approachable? Or, better yet, just TradeStream
and make aggregation an option. All good thoughts and I appreciate you sharing your opinions; I wish I had more involvement like this early-on. At the moment, I am trying hard to limit my refactoring/redesign and focus on finalizing 0.2.0 (and stop putting off documentation/testing, etc.).
I agree the root Extensions
folder has too many classes. So, I moved them to their respective namespace Extensions
folders (new folders created, but no namespace changes).
So, I've read the majority of Clean Architecture by Robert C. Martin now, and it hasn't changed my thinking.
Getting back to your original comment/question... I don't plan to separate this library. I consider the use of partial boundaries with recent changes enough. Also, some of the "helper" functionality will eventually be applied to "every exchange" ...as I've mentioned to others, that will be part of the next evolution of this API.
Hi! I think you have done a lot of work. But the design of the project does not look simple and friendly. Because of this, we need to apply a lot of effort to understand this project. Maybe if you decompose project into base/high/low-level logic and helpers/utils, it gets more friendly? I think many users need only a thin wrapper for binance api. Caching, rate limiting, asset/symbol list is good features but I rather want to do it myself and for every exchange.