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 - Nkiru - Adatrader #42

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

Pipes - Nkiru - Adatrader #42

wants to merge 23 commits into from

Conversation

nkiruka
Copy link

@nkiruka nkiruka 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 view is similar to a Rails controller; it helps to coordinate between the data and the DOM.
Did you use jQuery directly in your Views? How, and why? "Thou shalt not use jQuery directly in the view." Backbone uses jQuery behind the scenes intensively but it is all hidden within the view. Instead, a jQuery selection of el ($el), makes jQuery functions available to be be called on that element.
What was an example of an event you triggered? Why did you need to trigger it? In the quote model, I use trigger in the buy and sell functions. When the buy or sell button is clicked, an event is triggered, tradeHistoryView listens for "trade" and then appends the quote to the tradeHistoryView.
In what was is unit testing in JavaScript similar to unit testing in Ruby? In what ways is it different? Similarities: Structure is similar: Both Jasmine and Minitest have describe and it blocks nested in the main describe block, assert statements and matchers. Both testing suites follow the same general pattern: arrange, act and assert. Differences: In old versions of Minitest, "expect" was used but it is not required anymore. In js, the testing has to be surrounded with expect, for instance in the text book curriculum, word.get does not have expectations (i.e to be truthy method).

@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 doesn't remove itself
General
Has separate views for different parts of the app yes
Uses events (listening/handling and triggering) to manage different behavior in views yes - a little too much in places, see inline comment
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! While there's still some functionality missing around limit orders, it seems like you've addressed the core learning goals of the project, using events to manage a large app with multiple related components. Code is generally easy to read and understand, and with a couple of exceptions the design is well organized.

Event-driven programming is an important tool for the modern software engineer, especially in an asynchronous context like front-end JavaScript. It's also very different than the message-driven paradigm we've studied so far Designing such programs is a unique challenge, as is knowing when and when not to use event-driven techniques. Keep studying and thinking about them, always trying to come up with cleaner, more robust designs, and keep up the hard work!

const orderEntryView = new OrderEntryView({
el: '.order-entry-form',
bus: bus,
quotesList: quotes

Choose a reason for hiding this comment

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

I very much like that you pass the quote list directly to the OrderEntryView. Building this connection (as opposed to using the event bus or something else) will make your program's design much simpler.

buy(event) {
event.preventDefault();
console.log('test1');
let orderData = {

Choose a reason for hiding this comment

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

If you take out the console.logs, this function and sell() are almost identical. Could you consolidate the two somehow?

}

this.bus.trigger('newOrder', order);
},

Choose a reason for hiding this comment

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

It seems odd to me that the OrderEntryView and the OrderListView communicate through the bus. It might be cleaner to have them both know about the OrderList, or even for them to be combined into a single view.

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