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

Kimberley Zell - PIPES - Ada Trader #30

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

Conversation

kimpossible1
Copy link

Ada Trader

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
How do Backbone Views help structure your code? They allow you to select which container should be listened to and elements should be displayed in that are included in that view.
Did you use jQuery directly in your Views? How, and why? I used it to get the order form data in the OrderFormView
What was an example of an event you triggered? Why did you need to trigger it? I triggered an event call trade that was part of an event bus that would then enact the renderTrade method in the TradeHistoryView.
In what was is unit testing in JavaScript similar to unit testing in Ruby? In what ways is it different? The describe and it blocks are similar and the expect is different.

@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 executes but does not appear in the trade history
General
Has separate views for different parts of the app yes
Uses events (listening/handling and triggering) to manage different behavior in views yes
Practices good standards for views (ES6 syntax, doesn't directly use jQuery, returns this in render) yes
Error handling for the Order Entry Form Errors are handled, so you can't do a bad thing, but they are not reported to the user. This makes for a bad user experience: "It doesn't work and I don't know why"
Testing
Has unit tests for models no new tests
Overall Very good job overall - I'm quite happy with this submission!

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 orderFormView = new OrderFormView({
el: '#order-entry-form',
orderList: orders,
quoteList: quotes

Choose a reason for hiding this comment

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

I like that the OrderFormView knows about the orderList and the quoteList. This makes sense - these are the things it needs in order to do its job. I saw many students attempt to use the bus to get this information around, which ends up being a little clunky.


executeOrder() {
console.log('execute order is being called');
let quote = this.model.get('quote');

Choose a reason for hiding this comment

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

I think this might make more sense as a function defined on the Order model, since it only does data manipulation, and doesn't touch the DOM at all. To make sure the view is properly removed, you could add the following line to initialize() above:

this.listenTo(this.model, 'destroy', this.remove);

this.model.destroy();
this.remove();
}
}

Choose a reason for hiding this comment

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

The last thing that needs to happen here is to add a trade to the trade history. Based on your code elsewhere, it looks like triggering a trade event on the bus would set things in motion properly.

buy(event) {
console.log(event);
this.bus.trigger('trade', {
//this is what trade is

Choose a reason for hiding this comment

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

The code here in buy() is almost identical to what you've written in sell() below. Could these be consolidated somehow?

},
buy(event) {
console.log(event);
this.bus.trigger('trade', {

Choose a reason for hiding this comment

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

This is a good use of the bus. Many different sources (the different quotes) can produce the same event, and you want to be able to listen for them in one central place.

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