Skip to content
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

Pipes - Kee - Ada Trader #36

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

anemonekey
Copy link

Ada Trader

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
How do Backbone Views help structure your code? Views helped take out the bulk of coding logic out from app.js and into separate files. This made code easier to read and organize.
Did you use jQuery directly in your Views? How, and why? Instead of using jQuery directly, I used $. for selecting purposes.
What was an example of an event you triggered? Why did you need to trigger it? An event I needed to trigger was buying/selling so that the information could be recorded in the trade history section.
In what was is unit testing in JavaScript similar to unit testing in Ruby? In what ways is it different? The main difference feels like syntax. In terms of logical structure, it seems very similar to testing in Ruby with nests.

@droberts-sea
Copy link

Ada Trader

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene yes
Comprehension questions yes
Organization
Models and collections are defined in separate files yes
Code that relies on the DOM is located in or called by $(document).ready yes
Functionality
Quote prices change when clicking Buy and Sell yes
The Trade History updates when buying and selling a quote yes
A user can create an open order using the Order Entry Form yes
An open order removes itself from the open orders and updates the Trade History when fulfilled yes
General
Has separate views for different parts of the app yes
Uses events (listening/handling and triggering) to manage different behavior in views yes - bus is a little overused, see inline comments
Practices good standards for views (ES6 syntax, doesn't directly use jQuery, returns this in render) yes
Error handling for the Order Entry Form no
Testing
Has unit tests for models yes
Overall Good job overall! Event-driven programming is an important tool for the modern software engineer, especially in an asynchronous context like front-end JavaScript. It's also very different than the message-driven paradigm we've studied so far. Designing such programs is a unique challenge, as is knowing when and when not to use event-driven techniques. Keep studying and thinking about them, always trying to come up with cleaner, more robust designs, and keep up the hard work!

el: $('#order-workspace'),
model: orders,
template: orderTemplate,
quoteList: quotes,

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) {

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'),

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();

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:

  1. Quote price changes
  2. quote_price_change event on the bus
  3. Handled here by OrderListView.buySellQuote()
  4. Find Orders matching the quote
  5. 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:

  1. Quote price changes
  2. Event on the quote
  3. Handler in Order
  4. 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);

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants