10

Be consistent with your method naming; don't mix camelCase and snake_case, pick one and stick with it. I would recommend picking camelCase as there are a lot of JavaScript projects out there which use it as a convention, but feel free to go with whatever is most readable to you. I would define your extras object as so; falsey values shouldn't be picked up ...


7

First of all, Backbone views already have a cached version of $(this.el) in this.$el so you should use this.$el instead of $(this.el). I find that things work better when views only touch their own el and leave it up to someone else to put that el into the DOM. In your case, that would mean that you shouldn't do this: $('#showit').append($(this.el)); in ...


7

Interesting questions. Is setting 'style' still considered bad practice when creating elements through templating? Only if you cannot reasonably use css classes (which is your case) Should I be setting the style using jQuery after I generate the HTML from the template? Since you know the height and size prior to generating, you should make this part of ...


5

From a once over: Collection You could merge those 2 if statements into 1 You should remove commented out code You should remove console.log code You should try to have 1 chained var block You should use lowerCamelCasing consistently This makes the code look a little tighter: var Tweets = Backbone.Collection.extend({ initialize : function(options) { ...


5

There are few ways to solve your problem with Backbone.js + Require.js and executing functions or changing routes from view. 1. Using Backbone.Router Here is approach I've used multiple times: 'use strict'; // Your view -------------- define([ 'jquery' 'backbone' 'common/views/view', 'common/helpers/misc' ], function ($, Backbone, View, ...


5

Interesting code. As you might have guessed, there's a lot wrong with it. Events instead of chaining. Makes it harder to follow the code, makes it harder on IDE's that know how to jump to a function, no added benefits. You have this basically as a holder of global variables instead of calling functions with parameters Comments -> I like it, but be careful ...


4

Reviewing code for coolness is a first ;) Some observations: initialize : if really all you want to do there is Hello World, then you should take it out. initialize is meant for initializing your model with answers. getCorrect and getWrong are short one-liners that are called once, you should inline them arr1, arr2, coll and even result are pretty terrible ...


4

Can't say much about the javascript in general as I don't use it but: Please do not comment every line of code. This means you write your program twice: once in the comments and once in code. They are bound to drift apart and get out of sync and all of a sudden you have code which does the opposite of what the comment says and then you are in trouble. Bad ...


4

One quick note (I'm so sorry I have no chance to read the full code) please be careful when using reference values (i.e object and arrays) in defaults, let me show you one example of how this could be turned into an unexpected behavior: A = Backbone.Model.extend({defaults:{arr: ['a', 'b']}}) a = new A() a.get('arr').push('c') a.get('arr') >> ["a", "b",...


4

As this question was asked nearly four years ago it is likely the case that you have learned more about JavaScript since then and/or changed this code. Nonetheless perhaps the info below will help you and/or somebody else. It is interesting that The BackBoneJS documentation uses some valid JSON for specifying the keys of objects but not all. The keys in the ...


3

DetailsView You should pass jquery selector to the el — Backbone will handle it. Also, note that you should pass DOM element to the el property. Injecting one view into another is often a bad practice (if relation is not like parent and subview): is.options.dataEditorView.$el.show(); this.options.finalView.render(); Consider triggering event like ...


3

I would always much rather use the Backbone event listeners where possible rather than use callback hoooks. For example in MasterView, where you are fetching the tweets, a tidier approach might be to bind to the collection sync event, rather than use a callback: var MasterView = Backbone.View.extend({ initialize : function() {}, events : { "...


3

About Backbone I could add some suggestions: Pass selector string to the el property — Backbone will handle it. You can change initializing block to be more "Backbonish": $(function() { $('#btnSignup').on('click', function () { if ( typeof wndSignUp == "undefined" ) { // Initialize view var wndSignUp = ...


3

If your Person was initialized with a name other than "fetus", your test would fail.. but your test has several reasons for failing and that's what makes it not-so-good: that same test would also fail if a Person was "born" with a child (!) or if age defaulted to 1, or if the welcome to this world string was different. I've never written a unit test with JS,...


3

I don't see major refactorings in your code. However: Use triple-equals (see here for more) when doing comparisons with zero or one (and probably the part with a == b, unless you know you want otherwise) Pre-compile templates rather than doing them on-the-fly for better performance You are using Backbone, jQuery, and Underscore (or lodash) - re-examine ...


3

I like the code as is, Use of 'use strict' JsHint cannot find anything Readable, well named variables No obvious copy pasting or repeated code As for events, I would Wait until everything is rendered Add a $.click() to each menu item, item by selecting on a classname used only by those menu items, or you could re-build the id's of each menu item and ...


3

I'm not sure what seems convoluted to you. It's a simple piece of code, and if you remove anything from it, it will fail to do what you want, as you noted yourself: If you remove the debounce, the method will be fired 3 times: once for the single click + twice for the double-click. So you need that. If you remove the double click handler, then it will be ...


2

Answerifying what Joseph the Dreamer commented on, there is really not enough code to build a general routine out of, the code is fine as is. Though ideally you could add comments.


2

Interesting question, MVC It seems, you did not nail the MVC concept perfectly. In essence, you should have 1 single render routine for both RGB, HSV and CMYK since all these are just outputs of the same value from the model. It does not make sense to split that out. ( If you do this exercise you will notice the removal of a ton of copy pasted code). ...


2

From a once over: The trigger name 'loaded' should be a constant, declared on the userIdKey level You should not be using console.log, at the very least you should consoledate those calls into a function. It seems that if the user needs to be created thru .save(), that you are not storing the userIdKey neither in localStorage nor chrome.storage, rendering ...


2

I would approach that like this: Have an array with all the fields you var mandatoryFields = ['firstAttr','secondAttr']; Then have a validate function that takes attrs, mandatoryFields and errors function validate( o , mandatoryFields, errors ) { for( var i = 0 , i < mandatoryFields.length ; i++ ) { var field = mandatoryFields[i]; if(!o[...


2

You code looks good, I only can give a few suggestions: I find it more convenient to use ...bind(this) instead of var self = this; - in this case the extra variable is not necessary the code looks better. Maybe it is better to use an index within the array - in this way the array will not be copied several times. Of course, the JS is highly optimized ...


2

Generally it looks like your know what your doing, maybe you could be more efficient by removing the blur event since you wouldn't need to save/validate if the input is already populated or left empty, this will also save you calling onInputChange twice when an input is changed. Backbone will automatically create this.$el from the el property so you can ...


2

Work In Progress, interesting code, I had to try a few times to understand how to win the game. Please dont change the code again, since I made a copy and I am working on that copy. UI While not typically part of CodeReview, the UI should give you all the hints needed to play the game. The fact that 'R' should be pressed should be mentioned, even more ...


2

This is not the simplest way. There seems to be no point in instance, if there can be only 1 instance, why have it all ? Why have initialize at all ? Why set foo to 0, I would set it undefined If I were to keep the instance concept, I would write this: _instance: null, get instance() { if (this._instance === null) { this._instance = new ...


2

jQuery Your code uses jQuery, but you don't seem to be using any features of jQuery that offer any advantages over the DOM - unless you need to target older browsers you can remove your jQuery dependency and thus make your code better: faster (less abstraction, fewer dependencies to download), more portable, more future-proof, etc). Simply replace $("#id") ...


2

As jQuery is loaded, why not taking advantage of it. This will make the code short. In the OP code, Vanilla JS is used and that object is wrapped in jQuery which is not necessary. Just use jQuery with selector. Use an object, that'll be used to keep the mappings of the values and their corresponding configuration of moment. This will be easy to add new ...


1

I'm not very familiar with Backbone, however I do have a couple of comments: To simplify your life, check out Marionette Forms. It can handle quick validation of forms. This is an alternative to Backbone Forms, which is generally more demanding in terms of code needed. The following snippet: if(!validUser){ $(e.target).find('#userName').select(); } ...


1

Changing model attributes within an click handler is okay and very common, but I would suggest keeping your rendering code away from event handlers. Backbone supports event mechanism that can help implementing Observer pattern, so it's unnecessary to explicitly write renderOne() everywhere after you change the model. In your case, this.model.set({likes:++...


Only top voted, non community-wiki answers of a minimum length are eligible