some comments as I attempt to understand what's going on - #1
JonathanQuang wants to merge 2 commits into
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
| 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
| } | ||
|
|
||
| /** | ||
| * Returns shart ptr to the order that has the highest range.upper value for |
| 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.
| } | ||
|
|
||
| /** | ||
| * @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
| } | ||
|
|
||
| /** | ||
| * @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?
OhadRau
left a comment
OhadRau
left a comment
There was a problem hiding this comment.
This stuff looks good, i still want to improve comments before merging but Doxygen is a good step towards making this PR ready to merge
| @@ -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
2 participants
ohad pls add some javadoc comments