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 - Sara Frandsen - Ada Trader #29

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

Conversation

frankienakama
Copy link

Ada Trader

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
How do Backbone Views help structure your code? They organize code based on where/what specific pieces you want logic to work on.
Did you use jQuery directly in your Views? How, and why? I used it to render where the error messages would appear.
What was an example of an event you triggered? Why did you need to trigger it? I triggered a custom 'addTrade' event that would add either a quote or an order to the trade list.
In what was is unit testing in JavaScript similar to unit testing in Ruby? In what ways is it different? The basic layout was very similar--you describe what you are testing, and what you are testing for, then you arrange, act, and assert. The biggest differences were syntactic and learning different assertions ('expect..toEqual' instead of '.mustEqual').

Sara Frandsen added 30 commits December 12, 2017 15:44
@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 - nicely organized, good use of the bus
Practices good standards for views (ES6 syntax, doesn't directly use jQuery, returns this in render) yes
Error handling for the Order Entry Form some - detects an empty price, but does not check that the buy / sell price is above / below the current market price
Testing
Has unit tests for models yes
Overall Great work overall! This code is for the most part DRY, well-organized and easy to read. Keep up the hard work!


bySymbol: function(symbol) {
const filtered = this.filter(function (quote) {
return quote.get('symbol') === symbol;

Choose a reason for hiding this comment

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

I like that you broke this out as a separate function on the collection. Good organization!

initialize(params) {
this.buy = params.buy;
this.targetPrice = params.targetPrice;
this.symbol = params.symbol;

Choose a reason for hiding this comment

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

Backbone should copy all this data into the attributes automatically, which you can then access with this.get('targetPrice').

initialize(params) {
this.symbol = params.symbol;
this.buy = params.buy;
this.price = params.price;

Choose a reason for hiding this comment

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

Again, should not need to explicitly copy attributes here.

if (this.model.targetPrice < matchingQuote.attributes.price) {
matchingQuote.buy();
this.bus.trigger('addTrade', trade); // newTrade() in trade_list_view
this.trigger('deleteOrder'); // deleteOrder() above ^^

Choose a reason for hiding this comment

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

Why not call this.deleteOrder() directly?

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