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 - Sairagul - AdaTrader #45

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

Conversation

sairagula
Copy link

Ada Trader

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
How do Backbone Views help structure your code? They were similar to Rails controllers, they were used for coordination between data and DOM
Did you use jQuery directly in your Views? How, and why? We shouldn't use Jquery directly in views, instead we can use this.$()
What was an example of an event you triggered? Why did you need to trigger it? When user presses one of the buy/sell buttons, event will get triggered and tradeview that was listening for the event will add quote to its list
In what was is unit testing in JavaScript similar to unit testing in Ruby? In what ways is it different? I didn't finish some features yet including testing. I'll add as I work on them. But testing looks very similar in Ruby

@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 some - works for buy orders but not for sell
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 no
Testing
Has unit tests for models no
Overall Good work overall! For the most part your app is DRY, well-organized and easy to read. There are a couple places where you over-use events, where a regular method call would be simpler, but in general I am quite happy with this submission. Keep up the hard work!

// console.log('target price should be higher than current price');
// Do validation on the order
//orders.append(order);
this.bus.trigger('newOrder', order);

Choose a reason for hiding this comment

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

It seems a little round-about to trigger an event on the bus here. Might be cleaner for the OrderFormView to know about the OrderList, and to add the new orders directly.

initialize(params){
this.template = params.template;
this.bus = params.bus;
this.quotesList = params.quotesList;

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 QuoteList so it can associate the quote with a new order.

else{
if(this.model.sellIt()){
if(this.model.sellIt()){
this.model.destroy();

Choose a reason for hiding this comment

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

You have the same condition here twice! I suspect this is why sell orders don't clear themselves.

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