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

<Ada-Trader><Pipes><Roxanne Agerone> #47

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

Conversation

RAgerone
Copy link

Ada Trader

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
How do Backbone Views help structure your code? I think of Backbone views as being analogous to Rails Controllers. They help to keep your model functions near each other.
Did you use jQuery directly in your Views? How, and why? Chris said that the first commandment of views is, "Thou shall not put Jquery in your Backbone Views". Backbone has something that is useful. It is ".$" . You use this on your model to isolate something in your backbone view and you DO NOT USE JQUERY.
What was an example of an event you triggered? Why did you need to trigger it? There were many events triggered. Obviously, there were button triggers which make people feel great about interacting with your site. I used triggers to help ensure that functions were called at the proper time. I'm still working on getting the trigger to delete an order once it has been purchased. I ended up using an event bus which I named hamRadio. I imagined a giant game of "Simon Says" for this portion. I think that it would be very helpful if the objects in JavaScript had unique id's so that I could trigger events on them by using their id.
In what was is unit testing in JavaScript similar to unit testing in Ruby? In what ways is it different? I didn't get to go into too much of the JavaScript testing yet. From my cursory inspection, Jasmine isn't nearly as comprehensive as Minitest. For instance, I would like to see if my model was a typeOf that model, but I'm not sure if that's possible. A quote should be an instance of quote, right?

@droberts-sea
Copy link

Ada Trader

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene yes
Comprehension questions yes - Regarding unique IDs, if you need this then you're probably using an event bus to solve a problem it's not well-suited for. If you know exactly which object is going to emit an event, you should listen to that object directly instead of messing with a bus.
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 no
General
Has separate views for different parts of the app yes
Uses events (listening/handling and triggering) to manage different behavior in views yes - perhaps a little too much
Practices good standards for views (ES6 syntax, doesn't directly use jQuery, returns this in render) yes
Error handling for the Order Entry Form Errors are handled but not reported
Testing
Has unit tests for models yes
Overall

This is a good start. You were able to get a lot of the core functionality working, which is no mean feat. However, there's definitely room for improvement here, particularly around building a clean design for an event-driven program.

From my reading, most of your trouble seems to come from an over-reliance on the event bus. I suspect that you started working with the bus before quite understanding what problem it's supposed to solve and so ended up using it for everything, even when it made your design much more complicated. As they say, when you've got a shiny new hammer everything starts to look like a nail. Knowing when not to use an event bus (letting components know about and listen to each other) or when not to use events at all (direct function calls) is just as important as knowing how to use one.

As one concrete example of this, let's talk about your workflow for adding a limit order. To create a limit order, we need to do a few things:

  1. Respond to a user event (button click)
  2. Read form data to get price and symbol
  3. Find the quote that corresponds to the symbol
  4. Determine if the order is valid based on the current quote price
  5. Have the order listen for changes on the quote so it can execute when the price is right

In your code, this begins in LimitOrderListView, which owns both the list of limit orders and the form to add a new one.

The first two steps, listening for the click event and reading the form data, are well done.

Finding the quote that corresponds to the symbol is where things start to get messy. Your code triggers an event on the bus, which is handled in the QuoteListView, triggering another event on the bus that is in turn handled up by the individual LimitOrderView. This workflow is fairly unintuitive - it took me a good bit of head-scratching to figure out what was going on.

The reason event handling doesn't work well here is that you need to invoke some behavior and see the result - in this case, find the quote for a given symbol. With a direct function call you would use a return value, but event handlers don't have a way to return a value to whatever triggered the event. This means you need a complex system of reciprocal events to get the information where you need it.

A cleaner design might be to:

  • Move findQuoteBySymbol from QuoteListView to QuoteList
  • Pass the QuoteList as an extra parameter to the LimitOrderListView
  • In LimitOrderListView.addLimitOrder(), instead of triggering a find_quote_model event, call findQuoteBySymbol directly on the QuoteList
  • Pass the symbol you get back into the newly created LimitOrder
  • In LimitOrder.initialize(), listen to for price changes directly on the quote (rather than using the bus).

Note that we don't use the bus at all - we've replaced one event sequence with a direct function call, and the other with listening to the specific component we're interested in.

Event driven programming is not an easy topic to master, but it's an essential tool for a modern software engineer, especially in an asynchronous context like front-end JavaScript. Keep studying and thinking about it, and let me know if you'd like to sit down together and work through some revisions to this code.

uniqueQuoteSymbols(){
// this.$('#quotes').empty();
let quoteSymbols = [];
this.model.each((quote) => {

Choose a reason for hiding this comment

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

This function doesn't do any view-specific things. To me this means it's probably in the wrong place - perhaps it would be better as a function of QuoteList. This would probably simplify your event handling chain as well.

// dropdownElement.html('');
// console.log(event);
// console.log('this is event');
// event.forEach( (symbol) => {

Choose a reason for hiding this comment

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

I think I like the idea of putting this function here. I would take it a step further and, instead of building it as an event handler, call it directly from $(document).ready() with the list of symbols (or an instance of QuoteList).

this.dropdownTemplate = params.dropdownTemplate;
this.listenTo(this.hamRadio, 'render_order_dropdown', this.renderOrderDropdown);
this.listenTo(this, 'order_purchase', this.addLimitOrder);
this.listenTo(this, 'order_sell', this.addLimitOrder);

Choose a reason for hiding this comment

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

As far as I can tell, you don't have any code to trigger order_purchase or order_sell events.

addQuoteAttribute(quoteModel){
if(!this.quoteModel){
debugger;
}

Choose a reason for hiding this comment

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

This function is redefined below - you should remove this version of it.

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