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

Jessica Owens -- Carets #25

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

Jessica Owens -- Carets #25

wants to merge 14 commits into from

Conversation

vertige
Copy link

@vertige vertige commented Dec 18, 2017

Ada Trader

Congratulations! You're submitting your assignment!

##Note
This is incomplete. Tests haven't been written.

Comprehension Questions

Question Answer
How do Backbone Views help structure your code? They gave me controller actions like triggers and listeners and helped dry up app.js and the models
Did you use jQuery directly in your Views? How, and why? Yes. Mostly I used templates, however I did use this.$('.form-errors ul').append(<h3>${errorsHash[key]}</h3>) in my order_form_view, but it was isolated. I couldn't figure out how to render the errors via template in a timely manner.
What was an example of an event you triggered? Why did you need to trigger it? I triggered 'trade' via a bus in order to have the trade view update when a quote is bought or sold
In what was is unit testing in JavaScript similar to unit testing in Ruby? In what ways is it different? The format is very similar. It seems BDD is different from TDD. I haven't been able to sink my teeth into it yet, but I'm trying to figure out how to handle triggers and bus events.

@tildeee
Copy link

tildeee commented Jan 2, 2018

Ada Trader

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene x
Comprehension questions x
Organization
Models and collections are defined in separate files x
Code that relies on the DOM is located in or called by $(document).ready x
Functionality
Quote prices change when clicking Buy and Sell x
The Trade History updates when buying and selling a quote x
A user can create an open order using the Order Entry Form x
An open order removes itself from the open orders and updates the Trade History when fulfilled x
General
Has separate views for different parts of the app x
Uses events (listening/handling and triggering) to manage different behavior in views x
Practices good standards for views (ES6 syntax, doesn't directly use jQuery, returns this in render) Didn't always follow convention; renderForm instead of render, missed returning a this in one view. Also, your trade list view doesn't have a render; I think this is a code smell to show you that you could've approached the trades without using a separate view.
Error handling for the Order Entry Form x
Testing
Has unit tests for models At least the ones provided passed! :)
Overall

In your order_entry_view, when creating an order you have a generic loop to get your properties from the form into the right format for creating data. It's good and it works! It may be a little bit overengineering though-- In some cases, it may be more valuable to explicitly just find and add the properties so that it's clearer/easier to read, and more intentional for someone to know what is on that order.

Overall, good work! Event-driven programming is always interesting, especially in contrast with the message-driven programming we've done otherwise. Keep in mind that event-driven is not the only solution available. Otherwise, great job!

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