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 - Lindsey - Ada Trader #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

VirtualLindsey
Copy link

Ada Trader

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
How do Backbone Views help structure your code? It provided a consistent place to interact with specific DOM elements
Did you use jQuery directly in your Views? How, and why? I used jQuery to read fields, and select elements
What was an example of an event you triggered? Why did you need to trigger it? I triggered an event after a stock order was completed. I needed to have the quote and order views communicate as to prevent conflicting events.
In what was is unit testing in JavaScript similar to unit testing in Ruby? In what ways is it different? Model validations are basically the same. How you test events are very different. It reminds me of the VCR unit testing.

@droberts-sea
Copy link

Ada Trader

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene I only see one commit - looks as though your git setup is still giving you trouble. If this persists, you might sit down with an instructor for a more thorough investigation. We wouldn't want you to accidentally loose a bunch of work on your capstone.
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 - the bus is a little over-used in some places, but in general this is good
Practices good standards for views (ES6 syntax, doesn't directly use jQuery, returns this in render) yes
Error handling for the Order Entry Form yes
Testing
Has unit tests for models yes
Overall Good job overall! 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!

initialize(attributes) {
this.buy = attributes.buy;
this.targetPrice = attributes.targetPrice
this.quote = attributes.activeQuote;

Choose a reason for hiding this comment

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

I like that the Order knows which quote it's for. It's possible to achieve the same behavior through the bus, but the resulting code ends up being much more complex and hard to read.


quoteCheck: function(e) {
if (this.buy && parseFloat(this.targetPrice) > parseFloat(this.quote.get('price'))) {
this.bus.trigger(`buyMe${this.quote.attributes.symbol}`, this.quote.attributes);

Choose a reason for hiding this comment

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

Good use of a model function.

Since you know what quote you need to buy/sell, it might be cleaner to call the this.quote.buy() or this.quote.sell() directly here rather than passing messages through the bus.

buyOrder: function(event) {
event.preventDefault();
const orderData = {};
orderData['symbol'] = this.$(`[name=symbol]`).val();

Choose a reason for hiding this comment

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

This function and sellOrder are almost identical. Could you consolidate the two?

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