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

Bianca Fernandez | Pipes | Ada Trader #24

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

Conversation

biciclista22
Copy link

Ada Trader

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
How do Backbone Views help structure your code? Views help move some of the event calling that we put in the app.js file into the views, that serve as controllers for our MV app. It allows us to create varying areas of scope and create interaction between and among different views.
Did you use jQuery directly in your Views? How, and why? If I remember correctly, Chris stressed not to use jQuery in our Views. In looking at the views, I may have used it in my render methods, where I empty and then append view elements into the DOM.
What was an example of an event you triggered? Why did you need to trigger it? In the buy and sell functions in the quote.js file, I triggered a 'trade' event that would be picked up by the trade history view.
In what was is unit testing in JavaScript similar to unit testing in Ruby? In what ways is it different? I was not able to get to the testing for this project, unfortunately. Some similarities are the arrange, act, assert format, because it is a behavior driven testing language. The describe and it blocks also feel very reminiscent of MiniTest. The nested functions, though, are a visual difference. The expect portion of the test, though is different. There are actually 2 functions that are being called - the expect and matcher (each taking one argument).

@Hamled
Copy link

Hamled commented Jan 2, 2018

Ada Trader

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene
Comprehension questions
Organization
Models and collections are defined in separate files
Code that relies on the DOM is located in or called by $(document).ready
Functionality
Quote prices change when clicking Buy and Sell
The Trade History updates when buying and selling a quote
A user can create an open order using the Order Entry Form
An open order removes itself from the open orders and updates the Trade History when fulfilled
General
Has separate views for different parts of the app
Uses events (listening/handling and triggering) to manage different behavior in views
Practices good standards for views (ES6 syntax, doesn't directly use jQuery, returns this in render)
Error handling for the Order Entry Form No. The order entry form does not validate the order details (such as requiring a symbol and price).
Testing
Has unit tests for models No. There are no tests for the Order model validations (because there is no validation logic).
Overall Looks good! There's a little bit missing around the Order validation and associated tests, and some pretty minor bugs. Otherwise, I think this is a well done project and I like the clean approach you took for the event system without an event bus.

},
cancel(event) {
if (confirm("Are you sure?") === true){
this.model.destroy();
Copy link

Choose a reason for hiding this comment

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

We also need to call this.remove() here, as we do in completeOrder().

What's currently happening is that we destroy the Order model, which triggers update on the OrderList collection it was in, and that results in OpenOrderListView calling its render() function, which creates new OpenOrderView instances for each order remaining in the collection. However, it does not get rid of the old view instances.

As a result, when we cancel an order it is no longer listed in the open orders list but the view does stick around in memory -- and it's still listening to the Quote model for any updates. Thus, the cancelled order could still execute after it is no longer displayed. You can repro this by creating an order and clicking cancel immediately, then waiting until the relevant quote's price shifts enough that the cancelled order would have executed. You can see in the trade history that in fact the quote does get executed even after being cancelled.

// this.trades = []; // took this out because don't need to store this data anymore
this.template = params.template;

// this.listenTo(this.model, 'update', this.bind);
Copy link

Choose a reason for hiding this comment

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

I think there's a slight change that could be made to sort of "combine" this line and the bind() function below, to achieve the same result in a simpler way:

this.listenTo(this.model, 'trade', this.render);

The trick here is that while this.model is referring to the QuoteList collection, the way that Backbone works is that if any model inside of a collection triggers an event it "bubbles up" through the collection. So you can listen to the collection directly for any model-specific events, such as trade.

A secondary benefit you get from this approach is that you don't need to call bind() again if the collection of quotes is expanded.

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