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

Stef -- Carets #32

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

Stef -- Carets #32

wants to merge 20 commits into from

Conversation

SesameSeeds
Copy link

Ada Trader

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
How do Backbone Views help structure your code? Organization of the where and what. However, it's a bit confusing with so many and understanding the best places to put things. I felt there was a lot of trail and error.
Did you use jQuery directly in your Views? How, and why? Yes, here and there, such as; this.$('css.selector') and $el.
What was an example of an event you triggered? Why did you need to trigger it? Click functions for buy/sell so the appropriate things can be listened for and return the appropriate bought/sold stock.
In what was is unit testing in JavaScript similar to unit testing in Ruby? In what ways is it different? No idea if mine pass or not because of time, but I took the tests available in quote and made one for orders and wrote some out. The layout is similar to Ruby, you still do the same describing of what you are testing, testing for, and the three a's. Syntax is different, which is expected in a new language.

@SesameSeeds
Copy link
Author

SesameSeeds commented Dec 18, 2017

WIP, still not working properly with trade costs, ran out of time, will fix this asap.

@CheezItMan
Copy link

Ada Trader

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Decent number of commit and good commit messages, but a lot were added this weekend, what's keeping things from getting done in the week?
Comprehension questions Using this.$ doesn't use jQuery directly, instead it uses a jQuery selection within the view. This prevents you from causing a problem outside your view's area of concern. And no your tests don't pass.
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, but only for buy sell doesn't work because some of the placeholder values you put in for buy don't exist in sell.
An open order removes itself from the open orders and updates the Trade History when fulfilled The order isn't listening for changes on the Quote model, so it isn't done yet. I assume you ran out of time.
General
Has separate views for different parts of the app Check
Uses events (listening/handling and triggering) to manage different behavior in views It works for changes in Quotes updating the Quote view and additional orders updating OrderListView, but changes in quote models don't trigger a response in Order.
Practices good standards for views (ES6 syntax, doesn't directly use jQuery, returns this in render) note in quote_view.js line 29, you're using jQuery instead of this.$(. That's a no-no.
Error handling for the Order Entry Form You have a start on it with validations in Order, but currently it's not being used.
Testing
Has unit tests for models Some tests are written, they aren't passing.
Overall You did a pretty good job getting Wave 1 & 2 done, there are issues, as I've documented in your code, but they work and you have the general structure working. Wave 3 is where it comes apart. You need to study how backbone objects can communicate together with events. That's clearly where you need to review. Consider passing the QuoteList into the OrderListView and using it to create orders who then know what quote to watch, and can listen to change events. This resource may help a bit. You can also check the textbook curriculum or myself or Charles or Dee.

buy: true,
symbol: this.$('select[name=symbol]').val(),
targetPrice: parseFloat(this.$('input[name=price-target]').val()),
currentPrice: 0,

Choose a reason for hiding this comment

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

How is the order to get the current quote price? Maybe this view needs to have an instance variable referring to the Quotelist.

targetPrice: 50.00,
});

expect(order.isValid()).toEqual(true);

Choose a reason for hiding this comment

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

Validation failing

action: true,
});

expect(order.isValid()).toEqual(false);

Choose a reason for hiding this comment

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

Validation failing

else if (!this.buy && this.targetPrice <= this.currentPrice) {
error.targetPrice = ['Price is lower than market, aim higher!'];
}
else if (!attributes.targetPrice) {

Choose a reason for hiding this comment

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

You also need (based on your tests) to check for numeric value for price, boolean value for buy and maybe check if (!attributes.targetPrice) first.

const orderData = {
buy: false,
symbol: this.$('select[name=symbol]').val(),
targetPrice: parseFloat(this.$('input[name=price-target]').val()),

Choose a reason for hiding this comment

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

No currentPrice

sellQuote: function() {
this.model.set('buy', false);
let tradeTemplate = _.template($('#trade-template').html());
$('#trades').prepend(tradeTemplate(this.model.attributes));

Choose a reason for hiding this comment

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

You should not directly access jQuery in a view. Instead use this.$ to select things within the view. Since the trades section is not within the view you can can trigger an event and let the larger view handle the prepending.

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