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

Carets- Julia Meier- Ada Trader #22

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

Conversation

julmeier
Copy link

@julmeier julmeier commented Dec 18, 2017

Ada Trader

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
How do Backbone Views help structure your code? It helps to separate the user interface from the model.
Did you use jQuery directly in your Views? How, and why? Yes. For example, in the OrderListView in the render method, I use JQuery to clear the existing html list, and rerender the list with the updated model data. JQuery is useful in identifying where html can be inserted.
What was an example of an event you triggered? Why did you need to trigger it? In the OrderView, the evaluateOrder method determines if the current values of the stocks meet the requirements of current orders. If so, the method triggers the 'tradeMe' event, which is being listended for in the QuoteListView. When the QuoteListView hears this trigger event, it calls the tradeOrders method which adds the order to the trades log.
In what was is unit testing in JavaScript similar to unit testing in Ruby? In what ways is it different? I wasn't able to get to the testing portion of this project in the time alloted.

…n just returning it. the two initial tests now pass
…d forgetting to put back the original class value of btn-buy...
…e quotes collection in order_list_view.js. Identified the current market price for the symbol selected in the Order Entry Form, and adds this market price to the orderData hash to be validated in the Order model. Order model sends appropriate message to the view for target prices over or equal to the market price.
…incorrectly completes orders for sell order in Open Orders View, and incorrectly doesn't complete order when user manually changes price to meet order requirements
@CheezItMan
Copy link

Ada Trader

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Decent number of commits, good commit messages, except don't refer to waves for commit messages, focus on functionality.
Comprehension questions Check, however you don't use jQuery directly. Instead you use this.$ to limit selections to only within the View.
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, however you have a bug which limits you to buy or sell orders on a particular quote, you can't have a mix.
General
Has separate views for different parts of the app Check
Uses events (listening/handling and triggering) to manage different behavior in views Check, nice work with the custom events!
Practices good standards for views (ES6 syntax, doesn't directly use jQuery, returns this in render) Check, well done!
Error handling for the Order Entry Form Nice work here!
Testing
Has unit tests for models MISSING
Overall Really nice work, you had almost all the required functionality and did a nice job.

errors.targetPrice = ["Cannot enter a zero value for the target price."];
}

if (NaN(attributes.targetPrice)) {

Choose a reason for hiding this comment

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

Should be: if (isNAN(attributes.targetPrice)) {


//finds the current market price and adds it to the orderData hash, to be validated in the model.
const currentQuoteModel = this.quotes.find({symbol: symbol});
currentQuoteModel.set('buy', buyIsTrue);

Choose a reason for hiding this comment

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

Why add a buy attribute to Quote? Instead why not let the Order keep track of if it's buying or selling? Your solution limits you to only having buy orders or sell orders not a mix.


//TODO - need to add a validation
const newOrder = new Order(orderData)
if (newOrder.isValid()) {

Choose a reason for hiding this comment

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

Nice work with the messages!

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