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 | tanisha | ada trader #31

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

pipes | tanisha | ada trader #31

wants to merge 10 commits into from

Conversation

tanham
Copy link

@tanham tanham commented Dec 18, 2017

Ada Trader

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
How do Backbone Views help structure your code? backbone views removes code that deals with user interface from the app.js to allow for cleaner code
Did you use jQuery directly in your Views? How, and why? no, anytime I wanted to grab a DOM element, I used this. before it.Using this puts the jQuery selector in the context of that particular view. Without that context, the selector would be look through the entire project and so you would need to use ids to be more specific.
What was an example of an event you triggered? Why did you need to trigger it? in my quote view, I trigger a tradeUpdate in my buy and sell functions. the quote list view is listening for the trade update which is triggered in the buy or sell functions.
In what was is unit testing in JavaScript similar to unit testing in Ruby? In what ways is it different?

@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 no
General
Has separate views for different parts of the app yes
Uses events (listening/handling and triggering) to manage different behavior in views yes
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 no :(
Overall This is a good start. Your code for handling quotes and market orders looks good, and you're very close to having limit orders working. There's just a little more plumbing that needs to be done to ensure data is available where you need it. Keep up the hard work!

let formData = this.getFormData();
formData.buy = buy;
const newOrder = new Order(formData);
this.model.add(newOrder);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now you only have the symbol for this quote, since that's what you get when you read the form. However, in order to connect all the pieces, you really need the Quote itself, not just the symbol. That will let the Order listen to events on the quote, validate the initial buy/sell price, etc.

One way to do this would be for the OrderListView to know about the QuoteList collection, passing it in as an extra property to the constructor from app.js. Then in QuoteList you could write a function that, given a symbol, returns the corresponding quote.

},
buy() {
this.model.set('buy', true);
this.trigger('tradeUpdate', this);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I like the attribute name buy. A more descriptive way to go might be to call the attribute 'lastTradeType' and set it to 'buy' or 'sell'.

Since the type of the most recent trade isn't really relevant for the quote, an even better strategy would be to not set it on the model at all, but provide it as an extra parameter when you trigger the event:

this.trigger('tradeUpdate', this, true);

And then the event listener in QuoteListView:

trade: function(quoteView, buy) {
  const tradeData = quoteView.model.toJSON();
  tradeData.buy = buy;
  const compiledTradeTemplate = this.tradeTemplate(tradeData);
  // ...
}

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