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

Jan Edrozo | Carets #27

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

Jan Edrozo | Carets #27

wants to merge 17 commits into from

Conversation

JNEdrozo
Copy link

@JNEdrozo JNEdrozo 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 helped me structure my code my allowing me to separate which code related to DOM events stay in the view and move business logic (e.g. order model's custom priceCheck function) to the models while keeping the app.js file lean.
Did you use jQuery directly in your Views? How, and why? I used this.$('css-selectors') when appending to specific elements, and $el in each view's render function to add its respective compiled template html content.
What was an example of an event you triggered? Why did you need to trigger it? In the quote's view, there are 2 buttons: buy and sell. On click, the quote's buy and sell methods are executed and in both cases, a custom event (clickedBuyOrSellQuote) is triggered. This event is triggered so the quote_list_view can execute its showTrade function which appends the trade transaction in the trade history.
In what way is unit testing in JavaScript similar to unit testing in Ruby? In what ways is it different? Both are similar in that they have describe, before, and it blocks and testing is ideally suited for business logic in the model. JS testing is different in the syntax and model business logic can include events (e.g. triggers and listeners -- would like more assistance on specific testing of triggers and event listeners, please).

…r functions; Updated app.js(added a quoteListView variable to render)
…js Update: Added buyQuote & sellQuote button click events
…s so trade history entry records correct price (price before trade entry is logged)
… in order model, order_list collection, and their respective views
…l methods in quote model and changed showTrade function to pass tradeData instead of a quoteView in quote_list_view.js
…Order method; NOTE: priceCheck() is the callback function for an event listener that each order model has to monitor the changes in the Quote price to match the target price conditions
…o invoke the matchedQuote methods: buy() or sell()
@CheezItMan
Copy link

Ada Trader

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good number of commits and good commit messages
Comprehension questions Check
Organization
Models and collections are defined in separate files Check
Code that relies on the DOM is located in or called by $(document).ready Check
Functionality
Quote prices change when clicking Buy and Sell Check
The Trade History updates when buying and selling a quote Check
A user can create an open order using the Order Entry Form Check
An open order removes itself from the open orders and updates the Trade History when fulfilled Check
General
Has separate views for different parts of the app Check
Uses events (listening/handling and triggering) to manage different behavior in views Check
Practices good standards for views (ES6 syntax, doesn't directly use jQuery, returns this in render) Check
Error handling for the Order Entry Form Great work here!
Testing
Has unit tests for models Nice work, I have small criticisms, but a very good first jump into testing!
Overall Really good work. You hit all the requirements. I have a few minor criticisms, most seriously that the OrderListView and QuoteListView instances overlap, and you can read my in-code notes. Very impressive work!

// console.log(newOrder.matchedQuote);
if (newOrder.isValid()) {
this.model.add(newOrder);
newOrder.listenTo(newOrder.get('matchedQuote'), 'change', newOrder.priceCheck);

Choose a reason for hiding this comment

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

Nice! 🥇

'click button.btn-buy': 'buyOrder',
'click button.btn-sell': 'sellOrder',
},
buyOrder: function(event) {

Choose a reason for hiding this comment

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

I can't help but think you could refactor these methods to reuse the code.


it("buys if quote's price is equal to or below order's targetPrice", () => {
listener.listenTo(buyOrder, 'destroy', () => {
expect(true).toBeTruthy();

Choose a reason for hiding this comment

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

This part doesn't actually test anything since it only runs the test if destroy was called. You'd instead probably want to use a spy (part of Jasmine) and check to see if the event handler had been called.

Good work trying to test these however.

});
sellOrder.get('matchedQuote').set('price', 101);
sellOrder.priceCheck();
expect(testOrderList.length).toEqual(1);

Choose a reason for hiding this comment

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

This is a good place to do the testing for destroying models however, nice work!

model: quotes,
template: _.template($('#quote-template').html()),
tradeTemplate: _.template($('#trade-template').html()),
el: 'main',

Choose a reason for hiding this comment

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

This is a bit of a problem, it leads your QuoteListView instance and OrderListView instance to overlap on the DOM, which means they could interfere with each other.

Best practices in Backbone is for independent views to NOT overlap in the DOM. You can have subviews, but views that don't know about each other (one isn't managing the other) should not overlap.

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