Pipes - angela - ada-trader

[awilson2017]

Ada Trader

Congratulations! You're submitting your assignment!

Comprehension Questions

Question	Answer
How do Backbone Views help structure your code?	it provided controller actions like the listenTo and trigger functions. Plus it moved out code from app.js to appropriate views.
Did you use jQuery directly in your Views? How, and why?	NO. Thou' shall not use jquery directly in views. If I did the scope of the selection would be broader than within the views. We want to be limited to view's scope only.
What was an example of an event you triggered? Why did you need to trigger it?	a custom trigger was 'makeTrade' in quoteView. This made it possible to pass a trade from the quoteview to the tradeHistoryView.
In what was is unit testing in JavaScript similar to unit testing in Ruby? In what ways is it different?	it does the same thing eg comparing the desired outcome with the actual outcome. The difference is the setup: Ruby (A must equal B) and JS (expect A to equal B)

Form error handling is not completely set up to appear in the DOM and all associated spec tests pass.

[Hamled]

Ada Trader

What We're Looking For

Feature	Feedback
Core Requirements
Git hygiene	✅
Comprehension questions	✅ For question 1, I want to make clear that listenTo and trigger are more part of Backbone's Events system, and are not restricted to Views -- Models and Collections also have those functions.
Organization
Models and collections are defined in separate files	✅
Code that relies on the DOM is located in or called by $(document).ready	Mostly. There's one piece of code that should happen in $(document).ready which is happening before (setting up the dropdown selection options).
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	Half. Orders are removed from the list of open orders, but don't create trade history entries. There also appear to be some issues with the execution logic, but they're not consistent enough for me to identify a particular cause or set of repro steps.
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	Mostly. As you mentioned in your PR description, there is model validation logic for the Order model, and that code is being checked when the form is used to create a new order. There is some kind of bug in the code for presenting the error message on-screen, however.
Testing
Has unit tests for models	✅
Overall	Looks good! There are some issues with the order execution logic, although some of that might be due to changing the simulator code to shift prices in 1-cent increments rather than 10-cent increments? (This is the effect of changing maxChange = 1.00 to maxChange = 0.10).

[Hamled]

This code should really be within the $(document).ready(...) callback function.

The reason for that is because we're using jQuery to find the select tag on the page with ID dropdown, and then manipulating it by appending option tags inside of it. Both of these things are only guaranteed to work properly if we wait until the entire page has been loaded -- which is what the $(document).ready(...) is for.

We can see the problem manifest if we change index.html to have the script tag for app.bundle.js inside of the head tag after the meta tags, instead of at the bottom of the body tag. By moving the JS code to the top of our HTML file, the browser will load it and run the JS code in app.js first, before, loading the actual HTML content of the page.

In that case the drop down selection does not have any options added to it, because when this line of code runs jQuery cannot find the select tag with ID dropdown (because it hasn't been loaded yet from the HTML file).

[Hamled]

It doesn't look like this instance of Order is being used anywhere, so we could probably remove it?

[Hamled]

In general it's totally okay to have multiple callbacks for a single event (this is especially useful when the callbacks are doing completely unrelated stuff), however in this case it's not necessary.

What will happen hear is that this.addOrder() will be called first when the newOrder event is triggered. When that method runs it calls this.model.add(), which puts the new order into the OrderList collection... which itself triggers the update event on the collection.

And we already have a listener for the update event which runs this.render(), so we're effectively calling this.render() twice. Once as a result of update and then again as a result of newOrder.