some comments as I attempt to understand what's going on #1
Conversation
|
Haven't looked thru the PR yet but 2 questions -- is this Doxygen compatible right now? and can you add a Doxygen rule to the makefile so we can auto-generate the documentation? |
There was a problem hiding this comment.
Maybe this is pretty nit-picky but I think if we add this kind of user-facing documentation (as in doxygen/javadoc stuff) I want to see a really high quality of documentation because it's describing the API rather than the internals. This is a good start but I'd like to see these fleshed out or become more ya'know "professional" before I merge this.
As a sidenote, two things we can do to really improve the state of documentation:
- some kind of overview (markdown) file that talks about how this works at a high level, what files implement what portions, etc.
- comments in complicated methods like order matching and book matching
| @@ -3,6 +3,12 @@ | |||
|
|
|||
| namespace tagt::xchg::agent { | |||
|
|
|||
| /** | |||
| * Prints out XCHG=> followed by whatever string data is associated with agent ID (uint_16) | |||
There was a problem hiding this comment.
I think this comment is one of those comments that talks too much about implementation rather than purpose. Right now this is temporary behavior and in the future agent::reply is going to send a message to the agent over TCP, so I think the documentation should reflect that. Something like:
/**
* Sends a reply to a client (agent)
* @param agent The ID for the client to contact
* @param response The packet to send to the client
| @@ -16,6 +21,13 @@ inline std::shared_ptr<const order::Order> best<order::Direction::BUY> | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Returns shart ptr to the order that has the highest range.upper value for | |||
| @@ -24,8 +24,8 @@ namespace tagt::xchg::book { | |||
| struct Book { | |||
| size_t next_order_id { 0 }; | |||
| std::vector<order::Id> order_ids; | |||
| std::unordered_map<order::Id, std::shared_ptr<order::Order>> orders; | |||
| std::unordered_map<order::Ticker, order::Price> last_price; | |||
| std::unordered_map<order::Id, std::shared_ptr<order::Order>> orders;/**maps Id to shared_ptr*/ | |||
There was a problem hiding this comment.
Probably more useful that this is mapping to the Order rather than the shared_ptr, no?
| std::unordered_map<order::Id, std::shared_ptr<order::Order>> orders; | ||
| std::unordered_map<order::Ticker, order::Price> last_price; | ||
| std::unordered_map<order::Id, std::shared_ptr<order::Order>> orders;/**maps Id to shared_ptr*/ | ||
| std::unordered_map<order::Ticker, order::Price> last_price;/**maps Ticker to Price */ |
There was a problem hiding this comment.
Also, don't mean to get repetitive but I feel like this is one of those comments that doesn't really tell you anything new other than looking at the code. I'd like to explain the why here rather than the what. I.e. why am I tracking the last price? How does the code take care of that? Why are we using these data structures? etc.
| Range Range::overlap(const Range& other) const { | ||
| return { | ||
| .lower = std::max(this->lower, other.lower), | ||
| .upper = std::min(this->upper, other.upper) | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * @return constant Range with the smallest possible price and largest possible price |
There was a problem hiding this comment.
Q: Why does it have that behavior?
A: Something to do with how market orders are infinitely elastic (is that the right word?) to price
| @@ -48,6 +67,10 @@ Range StopLimit::range() const { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * @param other pointer to another order | |||
There was a problem hiding this comment.
Again, what is the point of this method? A: Check to see whether two orders "match", i.e. can they trade against each other?
| @@ -1,5 +1,5 @@ | |||
| CXX := g++ | |||
| CXXFLAGS := -std=c++2a -O3 -Wall -Wpedantic -MMD | |||
| CXXFLAGS := -std=c++17 -O3 -Wall -Wpedantic -MMD | |||
There was a problem hiding this comment.
obv discussion we had about this before, but i'll enumerate here so it's in writing on the repo:
C++17 makes some stuff not possible rn, i.e. we can't do struct initialization like {.x = 5}. Easy fix, we can just use constructors until this gets added in C++20. Might be useful to make a quick PR to get that & we can merge that before this PR just so we don't get warnings when we merge this. Happy to take care of that myself
ohad pls add some javadoc comments