-
Notifications
You must be signed in to change notification settings - Fork 43
Pipes - Kee - Ada Trader #36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Ada TraderWhat We're Looking For
|
| el: $('#order-workspace'), | ||
| model: orders, | ||
| template: orderTemplate, | ||
| quoteList: quotes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that the OrderListView knows about the QuoteList. I've seen several students attempt to achieve the same thing using the bus as an intermediary, but the code ends up being much more complex.
| this.addOrder(event, 'sell'); | ||
| }, | ||
|
|
||
| addOrder(event, action) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work using a helper method to DRY up your code here!
|
|
||
| if (order.get('buy') == 'buy' && order.get('targetPrice') >= quote.get('price')) { | ||
| let tradeItem = { | ||
| price: order.get('targetPrice'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bit of logic inside the forEach loop would work well as a function defined on Order.
| } | ||
| this.bus.trigger('boughtOrSold', tradeItem) | ||
| order.destroy(); | ||
| quote.sell(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the workflow for executing an open order looks like this:
- Quote price changes
quote_price_changeevent on the bus- Handled here by
OrderListView.buySellQuote() - Find
Orders matching the quote - For each one, determine if it needs to be sold
But we can simplify this! The key observation is that when you create the Order you must know which Quote it's for - you need this information to validate the Order. Instead of working through the intermediaries of the bus and the OrderListView, each Order could listen directly to its Quote for price changes. Then the workflow would look like:
- Quote price changes
- Event on the quote
- Handler in
Order - Determine if the order needs to be sold
This eliminates dependencies on the bus and the OrderListView. It also simplifies the code, since we don't need to work with a list of Orders. If there are multiple Orders corresponding to the same quote each will have a separate handler registered, and each will run when the event occurs (steps 3-4).
| initialize(params) { | ||
| this.bus = params.bus; | ||
| this.template = params.template; | ||
| this.listenTo(this.bus, 'boughtOrSold', this.render); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good use of the bus. There are many sources for the same type of event (quotes being immediately bought or sold, and orders executing when the price is right) and we want to treat them all the same, so we have them all trigger events on the same object. This greatly simplifies the design.
Ada Trader
Congratulations! You're submitting your assignment!
Comprehension Questions