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

Amy Cash - Pipes - Ada Trader #41

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

Amy Cash - Pipes - Ada Trader #41

wants to merge 28 commits into from

Conversation

cashmonger
Copy link

Ada Trader

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
How do Backbone Views help structure your code? They mediate between the presentation in the browser and the data/models that you are presenting.
Did you use jQuery directly in your Views? How, and why? Mainly for emptying and filling sections of HTML and appending generated data.
What was an example of an event you triggered? Why did you need to trigger it? A record trade event, so that, when a trade happens, the current info can be displayed in the trade history view.
In what was is unit testing in JavaScript similar to unit testing in Ruby? In what ways is it different? It's similar in terms of Describe and It blocks, but some of the specific syntax for setting up the tests is different.

@Hamled
Copy link

Hamled commented Dec 20, 2017

Ada Trader

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene
Comprehension questions
Organization
Models and collections are defined in separate files
Code that relies on the DOM is located in or called by $(document).ready
Functionality
Quote prices change when clicking Buy and Sell Mostly. This functionality does work, but the automatic price changes from the simulator are not working.
The Trade History updates when buying and selling a quote
A user can create an open order using the Order Entry Form
An open order removes itself from the open orders and updates the Trade History when fulfilled No. The Order model is not connected to the Quote model by listening to its change event, so it can never execute when the price is correct.
General
Has separate views for different parts of the app
Uses events (listening/handling and triggering) to manage different behavior in views Mostly. There are some unnecessary event triggers, and some missing event connections for wave 3.
Practices good standards for views (ES6 syntax, doesn't directly use jQuery, returns this in render)
Error handling for the Order Entry Form
Testing
Has unit tests for models No, only the provided tests for the Quote model's custom methods.
Overall There are a few pieces missing from waves 1 & 3, and likely some confusion around how models should connect to each other through events.

Otherwise, the project seems solid. I think your understanding of how to use Backbone Views to manage rendering and DOM events is good.

@@ -7,11 +7,15 @@ const Quote = Backbone.Model.extend({
},

buy() {
// Implement this function to increase the price by $1.00
let buyPrice = this.get('price');
let newPrice = buyPrice + 1.00;
Copy link

Choose a reason for hiding this comment

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

We could probably use const here instead of let.

let stringPrice = this.$(`#order-entry-form input[name=price-target]`).val();
orderData["targetPrice"] = parseFloat(stringPrice);
orderData["buy"] = buy;
this.bus.trigger('get_quote', this.model);
Copy link

Choose a reason for hiding this comment

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

It looks like triggering this event isn't doing anything, because there are no objects that are listening for it.

this.quoteList.each((quote) => {
if (orderData['symbol'] == quote.get('symbol') ) {
orderData['quote'] = quote;
}
Copy link

Choose a reason for hiding this comment

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

This could also be written as:

orderData.quote = this.quoteList.findWhere({ symbol: orderData.symbol });

this.recordTrade(false)
this.model.sell();
},
recordTrade(buy) {
Copy link

Choose a reason for hiding this comment

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

I think the logic in this function could be moved into the Quote model, and have it be run as part of buy() or sell().

This would allow it to be tested more easily, and reduces the coupling between this view and its model.


const quoteListView = new QuoteListView({
el: '#quotes-container',
model: quoteList,
Copy link

Choose a reason for hiding this comment

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

By passing quoteList here we are creating a view for a collection of quotes that is not connected to the simulator at all (and thus there appears to be no automatic change in prices).

If we insted write model: quotes, for this line then we'll be connecting this QuoteListView to the same collection of quotes as the simulator receives, which should solve the issue.

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